Fix UDPMuxDefault not reflecting network changes#926
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (67.64%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #926 +/- ##
==========================================
- Coverage 88.35% 88.19% -0.16%
==========================================
Files 46 46
Lines 5969 5973 +4
==========================================
- Hits 5274 5268 -6
- Misses 481 490 +9
- Partials 214 215 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
The idea of removing the cache interface looks good to me, maybe we need to make a small helper for detecting network changes across different systems in pin/transport, so we don't have to relay on library logic or polling, this was the plan for nomination anyways.
@cnderrauber this reverts some of a931199 does it look good for you?
| @@ -61,50 +61,24 @@ type UDPMuxParams struct { | |||
| } | |||
|
|
|||
| // NewUDPMuxDefault creates an implementation of UDPMux. | |||
| func NewUDPMuxDefault(params UDPMuxParams) *UDPMuxDefault { //nolint:cyclop | |||
| func NewUDPMuxDefault(params UDPMuxParams) *UDPMuxDefault { | |||
| if params.Logger == nil { | |||
| params.Logger = logging.NewDefaultLoggerFactory().NewLogger("ice") | |||
| } | |||
|
|
|||
| var localAddrsForUnspecified []net.Addr | |||
| if udpAddr, ok := params.UDPConn.LocalAddr().(*net.UDPAddr); !ok { //nolint:nestif | |||
| var isUnspecified bool | |||
| if udpAddr, ok := params.UDPConn.LocalAddr().(*net.UDPAddr); !ok { | |||
| params.Logger.Errorf("LocalAddr is not a net.UDPAddr, got %T", params.UDPConn.LocalAddr()) | |||
| } else if ok && udpAddr.IP.IsUnspecified() { | |||
| // For unspecified addresses, the correct behavior is to return errListenUnspecified, but | |||
There was a problem hiding this comment.
There is another cache layer in pion/transport that caches the network interfaces https://github.com/pion/transport/blob/1bb98ade0d7759304c0b88bd33914b42bc7f481b/stdnet/net.go#L70-L72 so localInterfaces will just return the same cached interfaces.
maybe we should share the refresh logic with nomination
Lines 1402 to 1405 in bd194f6
|
@JoTurk i did an assessment on the network change issue some time ago that confirms what you wrote about the two separate cache layers. As you mentioned there's also a refresh mechanism that is unfortunately placed in the wrong component, which is the agent inside each In my opinion, the right place in which to put the refresh mechanism is inside a shared instance of Anyway, a common step to any solution is to remove the cache layer inside |
aler9
force-pushed
the
fix/udp-localaddrs
branch
5 times, most recently
from
11fd566 to
34702fb
Compare
UDPMuxDefault was internally caching the list of local IPs, making connectivity impossible after a network change or a local IP change. This was also redundant given the fact that IPs and interfaces are already cached by their provider, which is stdnet.Net / transport.Net. This patch removes this cache layer, allowing UDPMuxDefault to receive upstream interface and IP changes, and fixing support for network changes.
3 participants
Description
UDPMuxDefault was internally caching the list of local IPs, making connectivity impossible after a network change or a local IP change. This was also redundant given the fact that IPs and interfaces are already cached by their provider, which is stdnet.Net / transport.Net.
This patch removes this cache layer, allowing UDPMuxDefault to receive upstream interface and IP changes, and fixing support for network changes.
Reference issue
bluenviron/mediamtx#5097