richardschneider / net-mdns

Simple multicast DNS
MIT License
227 stars 79 forks source link

Dispose UdpClient instances when disposing MulticastService #94

Open fgheysels opened 4 years ago

fgheysels commented 4 years ago

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

codecov-io commented 4 years ago

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%) :arrow_up:
src/MulticastClient.cs 82.78% <0%> (-2.46%) :arrow_down:

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.

fgheysels commented 4 years ago

Kind reminder @richardschneider

buildYourOwnGolfSimulator commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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...