Avoid unnecessary turn address resolution in the turn client#773
Avoid unnecessary turn address resolution in the turn client#773amanakin wants to merge 1 commit intopion:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #773 +/- ##
==========================================
- Coverage 78.84% 78.83% -0.02%
==========================================
Files 41 41
Lines 5441 5452 +11
==========================================
+ Hits 4290 4298 +8
- Misses 921 923 +2
- Partials 230 231 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| // We don't need to resolve address of the turn server, since we are using | ||
| // proxy dialer. | ||
| if _, err = a.net.ResolveUDPAddr("udp4", turnServerAddr); err != nil { |
There was a problem hiding this comment.
Why can't we always omit the TURN Server Addr now?
There was a problem hiding this comment.
In the proxy case, the TURN address is only used only for debug logging. So maybe completely skipping it is too much? Some might find having the address in the logs useful for debugging. What do you think?
There was a problem hiding this comment.
Do you think it would be possible to make this happen internally in the TURN package? Otherwise every user of the package has to do this check.
I think we should remove it personally. It confused me, and I imagine people in the future won't understand the purpose (and delete or modify)
There was a problem hiding this comment.
The only viable approach I can see on the turn library side is to add IgnoreTURNResolveError flag to this configuration struct: https://github.com/pion/turn/blob/master/client.go#L39
And it breaks the minimalism of the config, unfortunately :(
There was a problem hiding this comment.
@Sean-Der Sorry for the delay.
We can try to do something like that pion/turn#455 on the TURN package.
And then in the ICE package we will just propagate IgnoreTURNResolveErrors=true in case of a proxy connection.
WDYT?
|
@amanakin I released a new version of |
2 participants
Description
The
turn.NewClientfunction in the library currently requires TURN server addresses passed to it to be resolvable. This causes an error when using a proxy, as the TURN address might only be resolvable by the proxy server itself, not by the client application initiating the connection.To avoid this error in proxy scenarios, this PR modifies the initialization to omit the TURN address when creating the
turn.Client.Blocker:
This change depends on functionality introduced in
pion/turnPull Request #450.