962371 - De-prioritizes TURN TCP in case UDP works

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Nils Ohlmeier [:drno]]

It seems in the current implementation if a TURN server is used the choice between UDP and TCP being used just depends on fact which allocation succeeded faster.
This sometimes, depending on network conditions and speed of the TURN server, results in TURN TCP getting used even though UDP works as well.

TURN TCP should be de-prioritized in a way that it only gets chosen in case UDP does not work at all, so it only gets used as the last fall-back solution.

Comment 1

[Eric Rescorla (:ekr)]

The simple fix here seems to be to add a transport check here and assign
different priorities for TCP and UDP.

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate.c#345

Comment 2

[Eric Rescorla (:ekr)]

Assigning to Byron, though I could do this if Byron is too busy.

Comment 3

[Nils Ohlmeier [:drno]]

I thought the maybe even easier implementation for now, if we don't want to add transport priorities right now, could be to just have some internal flag which indicates that we have received for example at least one success response for UDP allocation request at the same server. If that flag is set when your TCP allocations complete you would wait a little bit more if the UDP allocations complete shortly after and use these instead.
But I'll leave up to implementer how to do it best.

Comment 4

[Byron Campen [:bwc]]

We could go a couple of ways with this. We could either have a configurable priority value for any candidate that uses TCP, and I guess this would carry less weight than the candidate type (for example, if we end up implementing full ICE TCP, a TCP host candidate would be preferred over a relayed UDP candidate). But this seems somewhat less flexible than just adding a configurable NR_ICE_REG_PREF_TYPE_RELAYED_TCP (and, if we end up implementing full ICE TCP, we would add a new configurable for each candidate type for TCP).

Comment 5

[Adam Roach [:abr]]

(In reply to Byron Campen [:bwc] from comment #4)
> We could go a couple of ways with this. We could either have a configurable
> priority value for any candidate that uses TCP, and I guess this would carry
> less weight than the candidate type (for example, if we end up implementing
> full ICE TCP, a TCP host candidate would be preferred over a relayed UDP
> candidate). But this seems somewhat less flexible than just adding a
> configurable NR_ICE_REG_PREF_TYPE_RELAYED_TCP (and, if we end up
> implementing full ICE TCP, we would add a new configurable for each
> candidate type for TCP).

That seems pretty reasonable. In terms of the actual priorities, I'm guessing we'd bump relayed from 0 to 5, and put relayed_tcp down at 0?

An alternate approach that ends up being more flexible in the long-run is to have a TCP "penalty" that subtracts, say, 5 from the type preference if the transport is TCP (with a floor of zero, just in case). Combined with adjusting the relayed type priority from 0 to 5, this has the same effect as I describe above, and it means we don't need to touch this again for any future TCP adaptations.

Comment 6

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #5)
> (In reply to Byron Campen [:bwc] from comment #4)
> > We could go a couple of ways with this. We could either have a configurable
> > priority value for any candidate that uses TCP, and I guess this would carry
> > less weight than the candidate type (for example, if we end up implementing
> > full ICE TCP, a TCP host candidate would be preferred over a relayed UDP
> > candidate). But this seems somewhat less flexible than just adding a
> > configurable NR_ICE_REG_PREF_TYPE_RELAYED_TCP (and, if we end up
> > implementing full ICE TCP, we would add a new configurable for each
> > candidate type for TCP).
> 
> That seems pretty reasonable. In terms of the actual priorities, I'm
> guessing we'd bump relayed from 0 to 5, and put relayed_tcp down at 0?
> 
> An alternate approach that ends up being more flexible in the long-run is to
> have a TCP "penalty" that subtracts, say, 5 from the type preference if the
> transport is TCP (with a floor of zero, just in case). Combined with
> adjusting the relayed type priority from 0 to 5, this has the same effect as
> I describe above, and it means we don't need to touch this again for any
> future TCP adaptations.

Having a fixed penalty is simpler, but perhaps less flexible; it seems to me that stuff like UDP relayed versus TCP reflexive is a bit of a toss-up. If you wanted to prefer UDP relayed in that case, the penalty would have to be very large, which would then make it difficult to prefer TCP host over UDP reflexive (TCP host would probably not be too bad if we're talking about two endpoints on the same local network).

Comment 7

[Byron Campen [:bwc]]

Attachment: Make priority for TCP relayed candidates lower than UDP relayed candidates.

One potential fix.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8363894 [details] [diff] [review]
Make priority for TCP relayed candidates lower than UDP relayed candidates.

Review of attachment 8363894 [details] [diff] [review]:
-----------------------------------------------------------------

Something I noticed that bugs me; we're not using the #defines when setting these config values in nricectx.cpp. This seems like a bad idea to me. Should I go ahead and fix them?

Comment 9

[Byron Campen [:bwc]]

Hey, let me know if this patch clears up the problem.

Comment 10

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=8738875ddfe0

Comment 11

[Adam Roach [:abr]]

Comment on attachment 8363894 [details] [diff] [review]
Make priority for TCP relayed candidates lower than UDP relayed candidates.

Review of attachment 8363894 [details] [diff] [review]:
-----------------------------------------------------------------

I'd really like to see some unit tests on this bug before we close it. You can do them in a separate patch if you prefer.

r+ with nits.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +358,5 @@
>        case RELAYED:
> +        if(cand->base.protocol == IPPROTO_UDP) {
> +          if(r=NR_reg_get_uchar(NR_ICE_REG_PREF_TYPE_RELAYED,&type_preference))
> +            ABORT(r);
> +        } else if(cand->base.protocol == IPPROTO_TCP) {

nICEr style is to put a line break before the "else".

@@ +362,5 @@
> +        } else if(cand->base.protocol == IPPROTO_TCP) {
> +          if(r=NR_reg_get_uchar(NR_ICE_REG_PREF_TYPE_RELAYED_TCP,&type_preference))
> +            ABORT(r);
> +
> +        } else {

here, too.

::: media/mtransport/third_party/nICEr/src/ice/ice_reg.h
@@ +42,5 @@
>  #define NR_ICE_REG_PREF_TYPE_HOST       "ice.pref.type.host"
>  #define NR_ICE_REG_PREF_TYPE_RELAYED    "ice.pref.type.relayed"
>  #define NR_ICE_REG_PREF_TYPE_SRV_RFLX   "ice.pref.type.srv_rflx"
>  #define NR_ICE_REG_PREF_TYPE_PEER_RFLX  "ice.pref.type.peer_rflx"
>  

This extra space seems unnecesary.

@@ +43,5 @@
>  #define NR_ICE_REG_PREF_TYPE_RELAYED    "ice.pref.type.relayed"
>  #define NR_ICE_REG_PREF_TYPE_SRV_RFLX   "ice.pref.type.srv_rflx"
>  #define NR_ICE_REG_PREF_TYPE_PEER_RFLX  "ice.pref.type.peer_rflx"
>  
> +#define NR_ICE_REG_PREF_TYPE_RELAYED_TCP    "ice.pref.type.relayed.tcp"

In keeping with the tree-nature of the preference interface (and being sensitive to the fact that these changes need to be upstreamed, where the strings might be more exposed than they are in Firefox), let's use something more like "ice.pref.type.relayed_tcp"

Comment 12

[Adam Roach [:abr]]

(In reply to Byron Campen [:bwc] from comment #8)

> Something I noticed that bugs me; we're not using the #defines when setting
> these config values in nricectx.cpp. This seems like a bad idea to me.
> Should I go ahead and fix them?

Not without first consulting with EKR. I'm not sure why it's structured the way it is, but I suspect it was done rather purposefully.

Comment 13

[Adam Roach [:abr]]

EKR: see Byron's question in comment 8.

Comment 14

[Byron Campen [:bwc]]

Attachment: Make priority for TCP relayed candidates lower than UDP relayed candidates.

Fix nits

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8363971 [details] [diff] [review]
Make priority for TCP relayed candidates lower than UDP relayed candidates.

Review of attachment 8363971 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+

Comment 16

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #11)
> Comment on attachment 8363894 [details] [diff] [review]
> Make priority for TCP relayed candidates lower than UDP relayed candidates.
> 
> Review of attachment 8363894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd really like to see some unit tests on this bug before we close it. You
> can do them in a separate patch if you prefer.

   Looking at the code, we are hard-coded to request TURN UDP allocations first. Without a test TURN server that can be configured to delay UDP allocations (or something that will eat the first TURN UDP request, maybe), this is going to be difficult to write a targeted test-case for. We can verify that the patch results in lower priority for the TURN TCP candidates though. Would that be sufficient?

Comment 17

[Eric Rescorla (:ekr)]

(In reply to Adam Roach [:abr] from comment #12)
> (In reply to Byron Campen [:bwc] from comment #8)
> 
> > Something I noticed that bugs me; we're not using the #defines when setting
> > these config values in nricectx.cpp. This seems like a bad idea to me.
> > Should I go ahead and fix them?
> 
> Not without first consulting with EKR. I'm not sure why it's structured the
> way it is, but I suspect it was done rather purposefully.

I am not sure, tbh. It may be historical, or it may be some includefile
issue. If you try it and it works it's fine by me.

Comment 18

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #17)
> (In reply to Adam Roach [:abr] from comment #12)
> > (In reply to Byron Campen [:bwc] from comment #8)
> > 
> > > Something I noticed that bugs me; we're not using the #defines when setting
> > > these config values in nricectx.cpp. This seems like a bad idea to me.
> > > Should I go ahead and fix them?
> > 
> > Not without first consulting with EKR. I'm not sure why it's structured the
> > way it is, but I suspect it was done rather purposefully.
> 
> I am not sure, tbh. It may be historical, or it may be some includefile
> issue. If you try it and it works it's fine by me.

I'll give it a go. Needinfoing myself as a reminder.

Comment 19

[Byron Campen [:bwc]]

Attachment: Part 2. Remove some magic constants.

Fix something that bugged me.

Comment 20

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=ebc04cccbdb3

Comment 21

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #9)
> Hey, let me know if this patch clears up the problem.

I ran three test calls between MTV "Mozilla" and "Mozilla Guest" networks. In all three cases I saw a mix of UDP and TCP for the first few seconds, but then everything switches to UDP only.
So looks like it is working.

Comment 22

[Adam Roach [:abr]]

Comment on attachment 8364512 [details] [diff] [review]
Part 2. Remove some magic constants.

Review of attachment 8364512 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, looks good.

Comment 23

[Eric Rescorla (:ekr)]

(In reply to Nils Ohlmeier [:drno] from comment #21)
> (In reply to Byron Campen [:bwc] from comment #9)
> > Hey, let me know if this patch clears up the problem.
> 
> I ran three test calls between MTV "Mozilla" and "Mozilla Guest" networks.
> In all three cases I saw a mix of UDP and TCP for the first few seconds, but
> then everything switches to UDP only.
> So looks like it is working.

I would have expected no UDP in most cases. Can you fwd the logs to Byron
so he can evaluate?

Comment 24

[Nils Ohlmeier [:drno]]

(In reply to Eric Rescorla (:ekr) from comment #23)
> I would have expected no UDP in most cases. Can you fwd the logs to Byron
> so he can evaluate?

Why would you expect no UDP?
I traced on the wire only on the "Mozilla" WiFi side so far, and that network supports UDP fine. So I want/expect it to use TURN UDP, rather then TURN TCP.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4cd87e44213
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b888357a3b2

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4cd87e44213
https://hg.mozilla.org/mozilla-central/rev/5b888357a3b2