Add DISABLE_TCP_TRANSPORT flag in the Transport API.

Add DISABLE_TCP_TRANSPORT flag in the Transport API.

The new property allows disabling TCP-based transport ports which improves performance for stream connections.

BUG=41776
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102060

[Wez, 2011-09-13 19:57:18]

http://codereview.chromium.org/7888022/diff/5001/ppapi/c/dev/ppb_transport_dev.h
File ppapi/c/dev/ppb_transport_dev.h (right):

http://codereview.chromium.org/7888022/diff/5001/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:54: // PP_TransportPorts values. By default all
ports are disabled.
It might be better to use the term "protocols" rather than "ports", since the
term "port" relates to the internals of libjingle, whereas what matters to the
caller is the protocols used.

Does it make sense for all ports to be disabled?

http://codereview.chromium.org/7888022/diff/5001/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:73: PP_TRANSPORTPORTS_RELAY_SSLTCP = 1 << 5,
Are we ever likely to want to add more of these, e.g. UDP4 to force IPv4 UDP
once IPv6 is available?

If so then splitting them into blocks might be better, e.g. UDP & TCP in the
lower 16-bits of "direct" types, STUN in an 8-bit block of nat-traversing types,
and the RELAY options in a final 8-bit block of relay types.

http://codereview.chromium.org/7888022/diff/5001/webkit/glue/p2p_transport.h
File webkit/glue/p2p_transport.h (right):

http://codereview.chromium.org/7888022/diff/5001/webkit/glue/p2p_transport.h#...
webkit/glue/p2p_transport.h:85: // Bit-mast of PP_TransportPorts values.
Typo: "mast".

http://codereview.chromium.org/7888022/diff/5001/webkit/plugins/ppapi/ppb_tra...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/7888022/diff/5001/webkit/plugins/ppapi/ppb_tra...
webkit/plugins/ppapi/ppb_transport_impl.cc:231: return PP_ERROR_BADARGUMENT;
Why does this only apply to "tcp"-tyle channels?

[Sergey Ulanov, 2011-09-13 20:31:37]

http://codereview.chromium.org/7888022/diff/5001/ppapi/c/dev/ppb_transport_dev.h
File ppapi/c/dev/ppb_transport_dev.h (right):

http://codereview.chromium.org/7888022/diff/5001/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:54: // PP_TransportPorts values. By default all
ports are disabled.
On 2011/09/13 19:57:19, Wez wrote:
> It might be better to use the term "protocols" rather than "ports", since the
> term "port" relates to the internals of libjingle, whereas what matters to the
> caller is the protocols used.
Ok, I'll change it to protocol.

> 
> Does it make sense for all ports to be disabled?
That was a typo in the comment. By default all are enabled.

http://codereview.chromium.org/7888022/diff/5001/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:73: PP_TRANSPORTPORTS_RELAY_SSLTCP = 1 << 5,
On 2011/09/13 19:57:19, Wez wrote:
> Are we ever likely to want to add more of these, e.g. UDP4 to force IPv4 UDP
> once IPv6 is available?
> 
> If so then splitting them into blocks might be better, e.g. UDP & TCP in the
> lower 16-bits of "direct" types, STUN in an 8-bit block of nat-traversing
types,
> and the RELAY options in a final 8-bit block of relay types.

It's probable that we may want to add more protocols in the future, but I don't
think there is a valid reason to distinguish IPv4 and IPv6 ports, so I don't
think it's worth splitting these in blocks, as we don't know what kind of
protocols we will want to add in the future.

http://codereview.chromium.org/7888022/diff/5001/webkit/glue/p2p_transport.h
File webkit/glue/p2p_transport.h (right):

http://codereview.chromium.org/7888022/diff/5001/webkit/glue/p2p_transport.h#...
webkit/glue/p2p_transport.h:85: // Bit-mast of PP_TransportPorts values.
On 2011/09/13 19:57:19, Wez wrote:
> Typo: "mast".

Done.

http://codereview.chromium.org/7888022/diff/5001/webkit/plugins/ppapi/ppb_tra...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/7888022/diff/5001/webkit/plugins/ppapi/ppb_tra...
webkit/plugins/ppapi/ppb_transport_impl.cc:231: return PP_ERROR_BADARGUMENT;
On 2011/09/13 19:57:19, Wez wrote:
> Why does this only apply to "tcp"-tyle channels?

Done.

[juberti, 2011-09-13 21:14:19]

http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
File content/renderer/p2p/port_allocator.cc (right):

http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
content/renderer/p2p/port_allocator.cc:280: (allocator_->config_.protocols &
PP_TRANSPORTPROTOCOL_RELAY_UDP)) {
Why do we care what protocol the remote side uses to talk to the relay server?

[Sergey Ulanov, 2011-09-13 21:55:48]

http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
File content/renderer/p2p/port_allocator.cc (right):

http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
content/renderer/p2p/port_allocator.cc:280: (allocator_->config_.protocols &
PP_TRANSPORTPROTOCOL_RELAY_UDP)) {
On 2011/09/13 21:14:19, juberti wrote:
> Why do we care what protocol the remote side uses to talk to the relay server?

For chromoting we disable TCP ports because they provide poor performance with
PseudoTCP. For the same reason we need to disable relay_tcp and relay_ssltcp.

The performance problem when running PseudoTCP over TCP can be fixed, but it
would require some changes in libjingle: AsyncPacketSocket interface and its
implementation would need to handle blocking sends properly. Currently
AsyncTCPSocket and AsyncUDPSocket simply drop outgoing packets when write cannot
be completed synchronously. PseudoTCP interprets dropped packets as network
congestion even when they are not caused by network congestion, which causes
PseudoTCP to shrink congestion window.

[Wez, 2011-09-14 00:37:53]

On 2011/09/13 21:55:48, sergeyu wrote:
>
http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
> File content/renderer/p2p/port_allocator.cc (right):
> 
>
http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
> content/renderer/p2p/port_allocator.cc:280: (allocator_->config_.protocols &
> PP_TRANSPORTPROTOCOL_RELAY_UDP)) {
> On 2011/09/13 21:14:19, juberti wrote:
> > Why do we care what protocol the remote side uses to talk to the relay
server?
> 
> For chromoting we disable TCP ports because they provide poor performance with
> PseudoTCP. For the same reason we need to disable relay_tcp and relay_ssltcp.
> 
> The performance problem when running PseudoTCP over TCP can be fixed, but it
> would require some changes in libjingle: AsyncPacketSocket interface and its
> implementation would need to handle blocking sends properly. Currently
> AsyncTCPSocket and AsyncUDPSocket simply drop outgoing packets when write
cannot
> be completed synchronously. PseudoTCP interprets dropped packets as network
> congestion even when they are not caused by network congestion, which causes
> PseudoTCP to shrink congestion window.


I think the problem is more fundamental than that.  PseudoTCP maintains timers
which control retransmission, etc, and those may fire if the underlying TCP
channel sees packet drops and has to retransmit, leading to retransmissions by
PseudoTCP, further congesting the channel.  

To work well over TCP channels, we need to be able to dynamically switch
PseudoTCP into a mode where it doesn't re-transmit data, and ensure that we
don't drop data that it queues for us (presumably by ensuring we have sufficient
buffering for the PseudoTCP window size).  We could switch it into an
encapsulation-only mode, or switch it off entirely, but that presents problems
if the channel switches underlying transport.

[juberti, 2011-09-14 19:43:07]

On 2011/09/14 00:37:53, Wez wrote:
> On 2011/09/13 21:55:48, sergeyu wrote:
> >
>
http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
> > File content/renderer/p2p/port_allocator.cc (right):
> > 
> >
>
http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allo...
> > content/renderer/p2p/port_allocator.cc:280: (allocator_->config_.protocols &
> > PP_TRANSPORTPROTOCOL_RELAY_UDP)) {
> > On 2011/09/13 21:14:19, juberti wrote:
> > > Why do we care what protocol the remote side uses to talk to the relay
> server?
> > 
> > For chromoting we disable TCP ports because they provide poor performance
with
> > PseudoTCP. For the same reason we need to disable relay_tcp and
relay_ssltcp.
> > 
> > The performance problem when running PseudoTCP over TCP can be fixed, but it
> > would require some changes in libjingle: AsyncPacketSocket interface and its
> > implementation would need to handle blocking sends properly. Currently
> > AsyncTCPSocket and AsyncUDPSocket simply drop outgoing packets when write
> cannot
> > be completed synchronously. PseudoTCP interprets dropped packets as network
> > congestion even when they are not caused by network congestion, which causes
> > PseudoTCP to shrink congestion window.
> 
> 
> I think the problem is more fundamental than that.  PseudoTCP maintains timers
> which control retransmission, etc, and those may fire if the underlying TCP
> channel sees packet drops and has to retransmit, leading to retransmissions by
> PseudoTCP, further congesting the channel.  
> 
> To work well over TCP channels, we need to be able to dynamically switch
> PseudoTCP into a mode where it doesn't re-transmit data, and ensure that we
> don't drop data that it queues for us (presumably by ensuring we have
sufficient
> buffering for the PseudoTCP window size).  We could switch it into an
> encapsulation-only mode, or switch it off entirely, but that presents problems
> if the channel switches underlying transport.

But there's no guarantee you have end-to-end reliability even with the relay leg
being TCP. I think you would still need PseudoTcp to do retransmits here.

Anyway, I get the desire to disable TCP fallback, since you believe in your case
it is better to fail to establish the session than to have poor performance.
Could we just make this control mechanism a UDP vs TCP control, instead of
having 6 knobs?

I also think there are still cases where you could end up using TCP even with
these flags; your flags control the port types for the remote client, but they
don't control what protocol is used for the local client to contact the relay,

[Sergey Ulanov, 2011-09-15 17:44:27]

On Wed, Sep 14, 2011 at 12:43 PM, <juberti@google.com> wrote:

> On 2011/09/14 00:37:53, Wez wrote:
>
>> On 2011/09/13 21:55:48, sergeyu wrote:
>> >
>>
>
> http://codereview.chromium.**org/7888022/diff/11/content/**
>
renderer/p2p/port_allocator.cc<http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allocator.cc>
>
>> > File content/renderer/p2p/port_**allocator.cc (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/7888022/diff/11/content/**
>
renderer/p2p/port_allocator.**cc#newcode280<http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allocator.cc#newcode280>
>
>> > content/renderer/p2p/port_**allocator.cc:280:
>> (allocator_->config_.protocols &
>> > PP_TRANSPORTPROTOCOL_RELAY_**UDP)) {
>> > On 2011/09/13 21:14:19, juberti wrote:
>> > > Why do we care what protocol the remote side uses to talk to the relay
>> server?
>> >
>> > For chromoting we disable TCP ports because they provide poor
>> performance
>>
> with
>
>> > PseudoTCP. For the same reason we need to disable relay_tcp and
>>
> relay_ssltcp.
>
>> >
>> > The performance problem when running PseudoTCP over TCP can be fixed,
>> but it
>> > would require some changes in libjingle: AsyncPacketSocket interface and
>> its
>> > implementation would need to handle blocking sends properly. Currently
>> > AsyncTCPSocket and AsyncUDPSocket simply drop outgoing packets when
>> write
>> cannot
>> > be completed synchronously. PseudoTCP interprets dropped packets as
>> network
>> > congestion even when they are not caused by network congestion, which
>> causes
>> > PseudoTCP to shrink congestion window.
>>
>
>
>  I think the problem is more fundamental than that.  PseudoTCP maintains
>> timers
>> which control retransmission, etc, and those may fire if the underlying
>> TCP
>> channel sees packet drops and has to retransmit, leading to
>> retransmissions by
>> PseudoTCP, further congesting the channel.
>>
>
>  To work well over TCP channels, we need to be able to dynamically switch
>> PseudoTCP into a mode where it doesn't re-transmit data, and ensure that
>> we
>> don't drop data that it queues for us (presumably by ensuring we have
>>
> sufficient
>
>> buffering for the PseudoTCP window size).  We could switch it into an
>> encapsulation-only mode, or switch it off entirely, but that presents
>> problems
>> if the channel switches underlying transport.
>>
>
> But there's no guarantee you have end-to-end reliability even with the
> relay leg
> being TCP. I think you would still need PseudoTcp to do retransmits here.
>
> In ether case it is not related to this CL. Lets continue this discussion
in the future outside this CL.



> Anyway, I get the desire to disable TCP fallback, since you believe in your
> case
> it is better to fail to establish the session than to have poor
> performance.
> Could we just make this control mechanism a UDP vs TCP control, instead of
> having 6 knobs?
>

For Chromoting we disable NAT traversal by disabling STUN and relays. These
flags are also useful for testing.


> I also think there are still cases where you could end up using TCP even
> with
> these flags; your flags control the port types for the remote client, but
> they
> don't control what protocol is used for the local client to contact the
> relay,


I don't think this is correct. RelayPort only connects to ports for which it
has addresses. If it doesn't have port number for TCP connections, it will
not be able to use TCP to connect to the relay server.



>
>
>
http://codereview.chromium.**org/7888022/<http://codereview.chromium.org/7888...
>

[juberti, 2011-09-15 20:10:59]

On 2011/09/15 17:44:27, sergeyu wrote:
> On Wed, Sep 14, 2011 at 12:43 PM, <mailto:juberti@google.com> wrote:
> 
> > On 2011/09/14 00:37:53, Wez wrote:
> >
> >> On 2011/09/13 21:55:48, sergeyu wrote:
> >> >
> >>
> >
> > http://codereview.chromium.**org/7888022/diff/11/content/**
> >
>
renderer/p2p/port_allocator.cc<http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allocator.cc>
> >
> >> > File content/renderer/p2p/port_**allocator.cc (right):
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/7888022/diff/11/content/**
> >
>
renderer/p2p/port_allocator.**cc#newcode280<http://codereview.chromium.org/7888022/diff/11/content/renderer/p2p/port_allocator.cc#newcode280>
> >
> >> > content/renderer/p2p/port_**allocator.cc:280:
> >> (allocator_->config_.protocols &
> >> > PP_TRANSPORTPROTOCOL_RELAY_**UDP)) {
> >> > On 2011/09/13 21:14:19, juberti wrote:
> >> > > Why do we care what protocol the remote side uses to talk to the relay
> >> server?
> >> >
> >> > For chromoting we disable TCP ports because they provide poor
> >> performance
> >>
> > with
> >
> >> > PseudoTCP. For the same reason we need to disable relay_tcp and
> >>
> > relay_ssltcp.
> >
> >> >
> >> > The performance problem when running PseudoTCP over TCP can be fixed,
> >> but it
> >> > would require some changes in libjingle: AsyncPacketSocket interface and
> >> its
> >> > implementation would need to handle blocking sends properly. Currently
> >> > AsyncTCPSocket and AsyncUDPSocket simply drop outgoing packets when
> >> write
> >> cannot
> >> > be completed synchronously. PseudoTCP interprets dropped packets as
> >> network
> >> > congestion even when they are not caused by network congestion, which
> >> causes
> >> > PseudoTCP to shrink congestion window.
> >>
> >
> >
> >  I think the problem is more fundamental than that.  PseudoTCP maintains
> >> timers
> >> which control retransmission, etc, and those may fire if the underlying
> >> TCP
> >> channel sees packet drops and has to retransmit, leading to
> >> retransmissions by
> >> PseudoTCP, further congesting the channel.
> >>
> >
> >  To work well over TCP channels, we need to be able to dynamically switch
> >> PseudoTCP into a mode where it doesn't re-transmit data, and ensure that
> >> we
> >> don't drop data that it queues for us (presumably by ensuring we have
> >>
> > sufficient
> >
> >> buffering for the PseudoTCP window size).  We could switch it into an
> >> encapsulation-only mode, or switch it off entirely, but that presents
> >> problems
> >> if the channel switches underlying transport.
> >>
> >
> > But there's no guarantee you have end-to-end reliability even with the
> > relay leg
> > being TCP. I think you would still need PseudoTcp to do retransmits here.
> >
> > In ether case it is not related to this CL. Lets continue this discussion
> in the future outside this CL.

Sure.
> 
> > Anyway, I get the desire to disable TCP fallback, since you believe in your
> > case
> > it is better to fail to establish the session than to have poor
> > performance.
> > Could we just make this control mechanism a UDP vs TCP control, instead of
> > having 6 knobs?
> >
> 
> For Chromoting we disable NAT traversal by disabling STUN and relays. These
> flags are also useful for testing.

That won't disable NAT traversal in all cases though (unless you always disable
on both sides). A peer can learn the NAT IP of the other side via ICE checks.

I'm not opposed to having these flags, but they seem very debug-y and perhaps
should not be exposed to the general API caller.

> 
> > I also think there are still cases where you could end up using TCP even
> > with
> > these flags; your flags control the port types for the remote client, but
> > they
> > don't control what protocol is used for the local client to contact the
> > relay,
> 
> 
> I don't think this is correct. RelayPort only connects to ports for which it
> has addresses. If it doesn't have port number for TCP connections, it will
> not be able to use TCP to connect to the relay server.

This is on the allocation side, the protocol that the client uses to get an
address from the relay. Unless you are stripping the relay TCP and SSLTCP
addresses when configuring the port allocator, the allocator will try to contact
the relay over those protocols if UDP fails.

> 
> 
> 
> >
> >
> >
>
http://codereview.chromium.**org/7888022/%3Chttp://codereview.chromium.org/78...>
> >

[Sergey Ulanov, 2011-09-15 23:18:15]

On Thu, Sep 15, 2011 at 1:11 PM, <juberti@google.com> wrote:

> On 2011/09/15 17:44:27, sergeyu wrote:
>
>>
>>  In ether case it is not related to this CL. Lets continue this discussion
>> in the future outside this CL.
>>
>
> Sure.
>
>
>  > Anyway, I get the desire to disable TCP fallback, since you believe in
>> your
>> > case
>> > it is better to fail to establish the session than to have poor
>> > performance.
>> > Could we just make this control mechanism a UDP vs TCP control, instead
>> of
>> > having 6 knobs?
>> >
>>
>
>  For Chromoting we disable NAT traversal by disabling STUN and relays.
>> These
>> flags are also useful for testing.
>>
>
> That won't disable NAT traversal in all cases though (unless you always
> disable
> on both sides). A peer can learn the NAT IP of the other side via ICE
> checks.
>

Yes, this is a known issue. That's the reason we disable outgoing
connections on the host side when NAT traversal is disabled.


> I'm not opposed to having these flags, but they seem very debug-y and
> perhaps
> should not be exposed to the general API caller.


Ok, I agree. I've replaced this property with one flag DISABLE_TCP_TRANSPORT
that allows to disable all TCP-based protocols.



>
>
>
>  > I also think there are still cases where you could end up using TCP even
>> > with
>> > these flags; your flags control the port types for the remote client,
>> but
>> > they
>> > don't control what protocol is used for the local client to contact the
>> > relay,
>>
>
>
>  I don't think this is correct. RelayPort only connects to ports for which
>> it
>> has addresses. If it doesn't have port number for TCP connections, it will
>> not be able to use TCP to connect to the relay server.
>>
>
> This is on the allocation side, the protocol that the client uses to get an
> address from the relay. Unless you are stripping the relay TCP and SSLTCP
> addresses when configuring the port allocator, the allocator will try to
> contact
> the relay over those protocols if UDP fails.
>

I'm confused. I think we always allocate session over HTTP, no? And I do
strip relay addresses in PortAllocator depending on which types of protocols
are enabled (see P2PPortAllocatorSession::AddConfig()).


>
>
http://codereview.chromium.**org/7888022/<http://codereview.chromium.org/7888...
>

[juberti, 2011-09-19 21:08:53]

On 2011/09/15 23:18:15, sergeyu wrote:
> On Thu, Sep 15, 2011 at 1:11 PM, <mailto:juberti@google.com> wrote:
> 
> > On 2011/09/15 17:44:27, sergeyu wrote:
> >
> >>
> >>  In ether case it is not related to this CL. Lets continue this discussion
> >> in the future outside this CL.
> >>
> >
> > Sure.
> >
> >
> >  > Anyway, I get the desire to disable TCP fallback, since you believe in
> >> your
> >> > case
> >> > it is better to fail to establish the session than to have poor
> >> > performance.
> >> > Could we just make this control mechanism a UDP vs TCP control, instead
> >> of
> >> > having 6 knobs?
> >> >
> >>
> >
> >  For Chromoting we disable NAT traversal by disabling STUN and relays.
> >> These
> >> flags are also useful for testing.
> >>
> >
> > That won't disable NAT traversal in all cases though (unless you always
> > disable
> > on both sides). A peer can learn the NAT IP of the other side via ICE
> > checks.
> >
> 
> Yes, this is a known issue. That's the reason we disable outgoing
> connections on the host side when NAT traversal is disabled.


OK.

> 
> 
> > I'm not opposed to having these flags, but they seem very debug-y and
> > perhaps
> > should not be exposed to the general API caller.
> 
> 
> Ok, I agree. I've replaced this property with one flag DISABLE_TCP_TRANSPORT
> that allows to disable all TCP-based protocols.
> 

OK.

> 
> 
> >
> >
> >
> >  > I also think there are still cases where you could end up using TCP even
> >> > with
> >> > these flags; your flags control the port types for the remote client,
> >> but
> >> > they
> >> > don't control what protocol is used for the local client to contact the
> >> > relay,
> >>
> >
> >
> >  I don't think this is correct. RelayPort only connects to ports for which
> >> it
> >> has addresses. If it doesn't have port number for TCP connections, it will
> >> not be able to use TCP to connect to the relay server.
> >>
> >
> > This is on the allocation side, the protocol that the client uses to get an
> > address from the relay. Unless you are stripping the relay TCP and SSLTCP
> > addresses when configuring the port allocator, the allocator will try to
> > contact
> > the relay over those protocols if UDP fails.
> >
> 
> I'm confused. I think we always allocate session over HTTP, no? And I do
> strip relay addresses in PortAllocator depending on which types of protocols
> are enabled (see P2PPortAllocatorSession::AddConfig()).
> 

The initial authorization is done over HTTP. However there is a subsequent step
where the client actually contacts the relay to bind the local socket to the
relay socket, in order to receive packets. 

i.e. 

C1 ---> protocol 1 (internal) --> relay <-- protocol 2 (external) <--- C2

Protocol 2, the "external" protocol, is the protocol specified in the candidates
you hand out. Protocol 1, the "internal" protocol, is the way the client talks
to the relay, and can be completely different from protocol 2.

[Sergey Ulanov, 2011-09-19 22:07:03]

On 2011/09/19 21:08:53, juberti wrote:
> On 2011/09/15 23:18:15, sergeyu wrote:
> > On Thu, Sep 15, 2011 at 1:11 PM, <mailto:juberti@google.com> wrote:
> > >  > I also think there are still cases where you could end up using TCP
even
> > >> > with
> > >> > these flags; your flags control the port types for the remote client,
> > >> but
> > >> > they
> > >> > don't control what protocol is used for the local client to contact the
> > >> > relay,
> > >>
> > >
> > >
> > >  I don't think this is correct. RelayPort only connects to ports for which
> > >> it
> > >> has addresses. If it doesn't have port number for TCP connections, it
will
> > >> not be able to use TCP to connect to the relay server.
> > >>
> > >
> > > This is on the allocation side, the protocol that the client uses to get
an
> > > address from the relay. Unless you are stripping the relay TCP and SSLTCP
> > > addresses when configuring the port allocator, the allocator will try to
> > > contact
> > > the relay over those protocols if UDP fails.
> > >
> > 
> > I'm confused. I think we always allocate session over HTTP, no? And I do
> > strip relay addresses in PortAllocator depending on which types of protocols
> > are enabled (see P2PPortAllocatorSession::AddConfig()).
> > 
> 
> The initial authorization is done over HTTP. However there is a subsequent
step
> where the client actually contacts the relay to bind the local socket to the
> relay socket, in order to receive packets. 
> 
> i.e. 
> 
> C1 ---> protocol 1 (internal) --> relay <-- protocol 2 (external) <--- C2
> 
> Protocol 2, the "external" protocol, is the protocol specified in the
candidates
> you hand out. Protocol 1, the "internal" protocol, is the way the client talks
> to the relay, and can be completely different from protocol 2.

Do I understand it correctly that your concern is that we cannot control type of
connection from C2 to the relay? If this flag is used then both clients would
need set it, this will guarantee that the UDP is used for both connections
C1->relay and C2->relay.

[juberti, 2011-09-20 18:37:45]

On 2011/09/19 22:07:03, sergeyu wrote:
> On 2011/09/19 21:08:53, juberti wrote:
> > On 2011/09/15 23:18:15, sergeyu wrote:
> > > On Thu, Sep 15, 2011 at 1:11 PM, <mailto:juberti@google.com> wrote:
> > > >  > I also think there are still cases where you could end up using TCP
> even
> > > >> > with
> > > >> > these flags; your flags control the port types for the remote client,
> > > >> but
> > > >> > they
> > > >> > don't control what protocol is used for the local client to contact
the
> > > >> > relay,
> > > >>
> > > >
> > > >
> > > >  I don't think this is correct. RelayPort only connects to ports for
which
> > > >> it
> > > >> has addresses. If it doesn't have port number for TCP connections, it
> will
> > > >> not be able to use TCP to connect to the relay server.
> > > >>
> > > >
> > > > This is on the allocation side, the protocol that the client uses to get
> an
> > > > address from the relay. Unless you are stripping the relay TCP and
SSLTCP
> > > > addresses when configuring the port allocator, the allocator will try to
> > > > contact
> > > > the relay over those protocols if UDP fails.
> > > >
> > > 
> > > I'm confused. I think we always allocate session over HTTP, no? And I do
> > > strip relay addresses in PortAllocator depending on which types of
protocols
> > > are enabled (see P2PPortAllocatorSession::AddConfig()).
> > > 
> > 
> > The initial authorization is done over HTTP. However there is a subsequent
> step
> > where the client actually contacts the relay to bind the local socket to the
> > relay socket, in order to receive packets. 
> > 
> > i.e. 
> > 
> > C1 ---> protocol 1 (internal) --> relay <-- protocol 2 (external) <--- C2
> > 
> > Protocol 2, the "external" protocol, is the protocol specified in the
> candidates
> > you hand out. Protocol 1, the "internal" protocol, is the way the client
talks
> > to the relay, and can be completely different from protocol 2.
> 
> Do I understand it correctly that your concern is that we cannot control type
of
> connection from C2 to the relay? If this flag is used then both clients would
> need set it, this will guarantee that the UDP is used for both connections
> C1->relay and C2->relay.

LGTM

Discussed with Sergey over IM. The internal protocol will always be UDP with
these changes, and no external TCP/SSLTCP candidates will be handed out, due to
the way TCP/SSLTCP candidate allocations works.

[brettw, 2011-09-20 19:55:28]

LGTM rubberstamp