Use UDPMux for Server reflexive candidates #419
Conversation
udp_mux.go
Outdated
| @@ -16,6 +19,7 @@ type UDPMux interface { | |||
| io.Closer | |||
| GetConn(ufrag string) (net.PacketConn, error) | |||
| RemoveConnByUfrag(ufrag string) | |||
| GetXORMappedAddr(serverAddr net.Addr, deadline time.Duration) (*stun.XORMappedAddress, error) | |||
There was a problem hiding this comment.
Adding GetXORMappedAddr breaks the public API.
Does this matter, does anyone implement? If everyone uses our Default I think it is ok.
There was a problem hiding this comment.
Good point and a good question :)
The thing is that if we want to be universal and support all the types of candidates and not only host candidates then this interface should change.
Also, we will be adding GetRelayedAddr function in the next steps to support relay candidates (I will be starting to work on this soon).
One option we could choose not to break the interface is to introduce a new interface UniversalUDPMux which will eventually replace the original UDPMux (which can be deprecated at this point).
I'm hoping that everyone uses the default UDPMux :)
What are your thoughts on that @Sean-Der @davidzhao @boks1971 ?
There was a problem hiding this comment.
At least I see that it won't break things in pion/webrtc cuz the default one is used
https://github.com/pion/webrtc/blob/master/icemux.go#L22
There was a problem hiding this comment.
this probably should use a different interface. it doesn't feel that the simple UDPMux should be concerned with STUN or XORMappedAddr.
There was a problem hiding this comment.
Got you,
To keep muxing logic separate I think we could then add a layer (struct) that reads from the UDPConn and handles STUN packets before the muxer. But then, this struct has to always wrap the muxer and should be exposed to the agent user.
I'm happy to jump in a call and discuss this further. We are very much looking forward to improving this part further and making pion/ice more efficient.
There was a problem hiding this comment.
Agreed with @davidzhao that we'll create a separate SRFLXUDPMux that would embed a UDPMux instance.
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
==========================================
- Coverage 79.28% 79.17% -0.12%
==========================================
Files 33 34 +1
Lines 3650 3860 +210
==========================================
+ Hits 2894 3056 +162
- Misses 590 625 +35
- Partials 166 179 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
lgtm! This is very nice!
@davidzhao would be the best person to catch any gotchas here. I am not fully familiar with this module.
gather.go
Outdated
|
|
||
| conn, err := a.udpMuxSrflx.GetConn(a.localUfrag) | ||
| if err != nil { | ||
| a.log.Warnf("could not find local connection in UDPMux %s %s: %v\n", network, url, err) |
There was a problem hiding this comment.
Maybe log it as UDPMuxSrflx to avoid any confusion.
There was a problem hiding this comment.
I feel a bit unease with GetXORMappedAddr in the UDPMux interface. It feels very specific to Srflx and it would complicate UDPMux for those that don't require this functionality. (Since most are running Pion in the server environment, they don't have to worry about srflx).
gather.go
Outdated
| return | ||
| } | ||
|
|
||
| conn, err := a.udpMuxSrflx.GetConn(a.localUfrag) |
There was a problem hiding this comment.
wouldn't this mean all of the srflx candidates would be using the same conn? If so when any one of the STUN servers fails, it'll end up closing the shared connection in closeConnAndLog
this might require a different key that's specific to each ufrag + url.
There was a problem hiding this comment.
I remember if a candidate is ultimately not chosen, it'll also get closed by the ICE agent. In that case even the chosen srflx candidate will be closed too.
There was a problem hiding this comment.
that's right, good point.
udp_mux.go
Outdated
| @@ -16,6 +19,7 @@ type UDPMux interface { | |||
| io.Closer | |||
| GetConn(ufrag string) (net.PacketConn, error) | |||
| RemoveConnByUfrag(ufrag string) | |||
| GetXORMappedAddr(serverAddr net.Addr, deadline time.Duration) (*stun.XORMappedAddress, error) | |||
There was a problem hiding this comment.
this probably should use a different interface. it doesn't feel that the simple UDPMux should be concerned with STUN or XORMappedAddr.
|
@davidzhao @Sean-Der I kept UDPMux and UDPMuxDefault as is (original ICE version, well with a minor change, see below) and added UniversalUDPMux that embeds UDPMuxDefault. The core idea of UniversalUDPMuxDefault is to just handle packets coming from the STUN server discovering a XorMappedAddress. I used STUN URL to add some uniqueness to the muxedconn and to support multiple STUN servers. Important P.S. |
The original UDPMux only works for the HOST candidates. UniversalUDPMux adds support for SRFLX candidates and will later support Relay candidates. UniversalUDPMux embeds UDPMuxDefault and handles STUN server packets to discover XORMappedAddr forwarding the remaining packets for muxing to UDPMuxDefault.
|
Hey @Sean-Der @davidzhao I reset our fork branch to a clean state and commit the whole work once again. I'm very sorry for not doing it from the very beginning! Cheers, |
There was a problem hiding this comment.
This is really great work @braginini! It's awesome to have your contributions here.
Description
See reference issue for details #400
Core changes:
The reason why we went with 2 separate UDPMux'ex for Host and Srflx candidates is the conflicts between different candidate types on candidates gathering. Original UDPMux uses uFrag to mux connections which is fine for the case when there is a single candidate type. When there are more different candidate types the packets are getting mixed up.
There is a possible solution to that which is using the STUN transaction ID map. The logic will be more complicated.
Next Steps:
We will introduce a UDPMuxRelay alongside UDPMux (for host candidates) and UDPMuxSrflx (for server reflexive candidates). At this point, there will be 1 UDP port for each candidate type (host, srflx, relay).
We will then consolidate these 3 UDPMux'es into a single one. I believe that once all the candidate type UDPMux'es are ready we'd be able to come up with a better refactor.
Reference issue
#400
EDIT:
This change breaks the UDPMux interface by adding the GetXORMappedAddr function.
See #419 (comment)