Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispose UdpClient instances when disposing MulticastService #94

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fgheysels
Copy link

@fgheysels fgheysels commented Jan 27, 2020

Since UdpClient implements IDisposable, and unicastClientIp4 and unicastClientIp6 are members of the MulticastService class, these instances must be disposed when disposing the MulticastService instance.

See issue 93

…hen disposing MulticastService

Since UdpClient implements IDisposable, and unicastClientIp4 and unicastClientIp6 are members of the MulticastService class, these instances must be disposed when disposing the MulticastService instance.
@codecov-io
Copy link

Codecov Report

Merging #94 into master will decrease coverage by 0.54%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage   91.11%   90.57%   -0.55%     
==========================================
  Files          10       10              
  Lines         518      520       +2     
==========================================
- Hits          472      471       -1     
- Misses         46       49       +3
Impacted Files Coverage Δ
src/MulticastService.cs 91.66% <100%> (+0.09%) ⬆️
src/MulticastClient.cs 82.78% <0%> (-2.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9f2f81...c6349ff. Read the comment docs.

Instead of disposing the UpdClient instances in the Stop method, they've been moved to the Dispose method.  Reasoning here is that it should be possible to Stop and Start the MulticastService instance again.
@fgheysels
Copy link
Author

Kind reminder @richardschneider

@buildYourOwnGolfSimulator

I think the NetworkAddressChanged callback needs to be removed as well in the Stop() method:

#if NET461
if (Environment.OSVersion.Platform.ToString().StartsWith("Win"))
#else
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
#endif
{
NetworkChange.NetworkAddressChanged -= OnNetworkAddressChanged;
}

@buildYourOwnGolfSimulator

To me it also looks like the NetworkAddressChanged callback will update the clients to send/receive upd on, but it will not update the address list used in the response. Those addresses can be specified when creating a ServiceProfile to Advertise, and ends up in the NameServer.Catalog.
So only solution I found is to subscribe to NetworkAddressChanged myself, and call Unadvertise,and then Advertise again with a new ServiceProfile. But seems like memory isn't handled properly then...

@buildYourOwnGolfSimulator

Since I subscribe to NetworkAddressChanged myself, and re-advertise, the solution for the memory leakage was to remove all use of NetworkAddressChanged from the mdns code. Be aware that NetworkAddressChanged is called twice for every change if you have both ipv4 and ipv6 enabled, so make sure you don't re-advertise too much, or from different threads...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants