Closed Bug 1322659 Opened 8 years ago Closed 8 years ago

Too many STUN/TURN servers don't help with conectivity

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file)

Bug 1322659: log warnings and error for too many STUN/TURN servers.
58 bytes, text/x-review-board-request
bwc
: review+
Details 
      No description provided.
backlog: --- → webrtc/webaudio+
Rank: 25
Depends on: 1322438
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98196

::: dom/media/PeerConnection.js:597
(Diff revision 1)
>          throw new this._win.DOMException(msg + " - malformed URI: " + uriStr,
>                                           "SyntaxError");
>        }
>      };
>  
> +    stunServers = turnServers = 0;

If every TURN server is also a STUN server, we can probably just count STUN servers.

::: dom/media/PeerConnection.js:638
(Diff revision 1)
>            this.logWarning(url.scheme.toUpperCase() + " is not yet supported.");
>          }
> +        if ((stunServers >= 5) || (turnServers >= 5)) {
> +          this.logError("Using five or more STUN/TURN servers causes problems");
> +        } else if ((stunServers > 2) || (turnServers > 2)) {
> +          this.logWarning("Using more then two STUN/TURN servers slows down discovery");

s/then/than/
Attachment #8817581 - Flags: review?(docfaraday) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/de0d1099b71e
log warnings and error for too many STUN/TURN servers. r=bwc
https://hg.mozilla.org/mozilla-central/rev/de0d1099b71e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98300

::: dom/media/PeerConnection.js:635
(Diff revision 4)
>          }
>          if (url.scheme in { stuns:1, turns:1 }) {
>            this.logWarning(url.scheme.toUpperCase() + " is not yet supported.");
>          }
> +        if (stunServers >= 5) {
> +          this.logError("Using five or more STUN/TURN servers causes problems");

What problems does this cause exactly? Seems a little strong to make it an error, given that the spec says nothing about this. Maybe it should be a warning also?
Flags: needinfo?(docfaraday)
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98302

::: dom/media/PeerConnection.js:635
(Diff revision 4)
>          }
>          if (url.scheme in { stuns:1, turns:1 }) {
>            this.logWarning(url.scheme.toUpperCase() + " is not yet supported.");
>          }
> +        if (stunServers >= 5) {
> +          this.logError("Using five or more STUN/TURN servers causes problems");

Also, should we maybe log this outside the for-loop or check for == 5 so we don't log multiple times for every server over 2?
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98306

::: dom/media/PeerConnection.js:635
(Diff revision 4)
>          }
>          if (url.scheme in { stuns:1, turns:1 }) {
>            this.logWarning(url.scheme.toUpperCase() + " is not yet supported.");
>          }
> +        if (stunServers >= 5) {
> +          this.logError("Using five or more STUN/TURN servers causes problems");

Lastly, if a server has two urls, this seems to count it twice, was that intended?
(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> Comment on attachment 8817581 [details]
> Bug 1322659: log warnings and error for too many STUN/TURN servers.
> 
> https://reviewboard.mozilla.org/r/97816/#review98300
> 
> ::: dom/media/PeerConnection.js:635
> (Diff revision 4)
> >          }
> >          if (url.scheme in { stuns:1, turns:1 }) {
> >            this.logWarning(url.scheme.toUpperCase() + " is not yet supported.");
> >          }
> > +        if (stunServers >= 5) {
> > +          this.logError("Using five or more STUN/TURN servers causes problems");
> 
> What problems does this cause exactly? Seems a little strong to make it an
> error, given that the spec says nothing about this. Maybe it should be a
> warning also?

It radically slows down ICE, due to the combinatorial explosion of candidate pairs, perhaps to the point of failure. 5+ STUN/TURN servers is pants-on-head crazy, so I'm ok with this being logged as an error.
Flags: needinfo?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #11)
> It radically slows down ICE, due to the combinatorial explosion of candidate
> pairs, perhaps to the point of failure. 5+ STUN/TURN servers is
> pants-on-head crazy, so I'm ok with this being logged as an error.

Understood, but unless our code omits doing something this is a warning not an error. With errors we get into spec compliance issues.
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98302

> Also, should we maybe log this outside the for-loop or check for == 5 so we don't log multiple times for every server over 2?

How about we do == 3 for the first warning. And to bump up the annoyance for the second message we do in fact leave it in the loop and leave it at >= 5?
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98300

> What problems does this cause exactly? Seems a little strong to make it an error, given that the spec says nothing about this. Maybe it should be a warning also?

What the over all patch tries to achieve is to point out to people that more STUN/TURN servers do not improve connectivity at all, but in fact (can) make it worse.
I'm fine with making this a warning. But I would like to make the message for more then five a lot more clear then with just two. Not sure if the current message is achieving this.
Maybe it would help to write up a blog post on the topic and then have the warning point to that blog post?
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98306

> Lastly, if a server has two urls, this seems to count it twice, was that intended?

Do you mean it counts for STUN and TURN? Yes that is intentional as every TURN server can be used as STUN server as well (although I should probably check if we really do that in nICEr - I know we do it for the TCP case, but not sure for the UDP one).

Otherwise if two urls with for example different host names point to the same server it will cause the nICEr to check candidates for each of the URLs. So again a scenario which slows things down in best case.
Comment on attachment 8817581 [details]
Bug 1322659: log warnings and error for too many STUN/TURN servers.

https://reviewboard.mozilla.org/r/97816/#review98306

> Do you mean it counts for STUN and TURN? Yes that is intentional as every TURN server can be used as STUN server as well (although I should probably check if we really do that in nICEr - I know we do it for the TCP case, but not sure for the UDP one).
> 
> Otherwise if two urls with for example different host names point to the same server it will cause the nICEr to check candidates for each of the URLs. So again a scenario which slows things down in best case.

No. My understanding was that we renamed `url` to `urls` and added support for an array of urls per server, because due to NATs there may be more than one way (URL) to find the SAME server.

I'd be surprised if having more urls adds to the load, so counting the number of URLs toward the warning seems wrong, but I could be off.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: