Fix regression with UDPMuxDefault and NAT1To1IPs

[aler9]

Description

PR #496 added a mechanism to close connections of duplicate candidates and avoid memory leaks. When UDPMuxDefault and NAT1To1IPs are used together, candidates may share the same connection, and candidates may be equal, since their IPs are overridden by the one specified in NAT1To1IPs. When a duplicate candidate is found, its connection is closed, making impossible to use any other candidate that shares the same connection.

This PR fixes the issue as it prevents duplicate candidates from creating connections and getting their connection closed.

Reference issue

Fixes #505

Related to bluenviron/mediamtx#1680

[codecov]

Codecov Report

Patch coverage: 75.00% and project coverage change: +0.15 🎉

Comparison is base (90f8411) 78.00% compared to head (ce83595) 78.16%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   78.00%   78.16%   +0.15%     
==========================================
  Files          39       39              
  Lines        4283     4291       +8     
==========================================
+ Hits         3341     3354      +13     
+ Misses        732      728       -4     
+ Partials      210      209       -1     

Flag	Coverage Δ
go	78.16% <75.00%> (+0.15%)	⬆️
wasm	25.16% <0.00%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
gather.go	66.60% <75.00%> (+1.03%)	⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[stv0g]

Thanks @aler9! Thats great :) I have two smaller comments on your PR.

And, I've noticed that the PR is based on an outdated master branch.
Could you rebase your changes on the latest master?

Thanks a lot :)

[stv0g]

I also assume that this will fix #505?

@aler9 could you confirm this?

[aler9]

Hello @stv0g, i have applied all the requested changes and i can confirm that this fixes #505. Furthermore, the PR adds a unit test with the exact use case (UDPDefaultMux + NAT1To1).

The question is that UDPMux is used in two ways:

• with NewMultiUDPMuxFromPort like livekit and @cnderrauber do. This caused the memory leak that was solved with Close connection if candidates already exist #496
• with NewUDPMuxDefault like mediamtx and i do. This has the advantage of not having to scan interfaces manually, but stopped working after Close connection if candidates already exist #496

Personally i don't have a preference and it's not my role to decide, but i'd like for both methods to continue working in order not to break existing code.

[stv0g]

Thanks @aler9,

It looks good now. We can ignore the failing CI tests for Go 1.18.

I will @cnderrauber some time to also review the change.
But from my side, we could merge it 👍

[cnderrauber]

Thanks for improving this!

[aler9]

i applied all the suggested changes.