Plumbing for TCP FastOpen for SSL sockets.

Plumbing for TCP FastOpen for SSL sockets.

TCP FastOpen is currently turned on for all TCP connections when explicitly enabled from flags. This CL implements the plumbing required to turn it on for SSL connections by default. A subsequent CL will do the Finch plumbing.

BUG=402005, 413370
Committed: https://crrev.com/dcb4ae922b46a90ef6ec199b772f399e4610f514
Cr-Commit-Position: refs/heads/master@{#294697}

[Jana, 2014-08-10 05:42:10]

Here are some tcpdumps for your reading pleasure. 

First, here's what you see with the user-enabled TFO for all connections (turned
on from about://flags):
--------------------
Non-SSL connection setup (Note TFO option is sent in the SYN):

22:12:26.584939 IP 172.22.113.244.41374 > 172.16.255.9.3128: Flags [S], seq
918333267, win 29200, options [mss 1460,sackOK,TS val 8721963 ecr 0,nop,wscale
7,Unknown Option 254f989], length 0
-----

SSL connection setup (With a new server, note TFO option in SYN and server
returns cookie):

22:12:29.130464 IP 172.22.113.244.50014 > 74.125.239.145.443: Flags [S], seq
1355087234, win 29200, options [mss 1460,sackOK,TS val 8722599 ecr 0,nop,wscale
7,Unknown Option 254f989], length 0

22:12:29.131395 IP 74.125.239.145.443 > 172.22.113.244.50014: Flags [S.], seq
2118878831, ack 1355087235, win 42540, options [mss 1430,sackOK,TS val 752234616
ecr 8722599,nop,wscale 7,Unknown Option 254f9897484ca502743a118], length 0
-----

With the code in this CL, and with TFO turned off in flags, you see TFO for SSL
connections only:
-------------------

SSL connection setup (With a known server, note TFO cookie in SYN, data of 223
bytes, and server acks it all):

22:16:39.038280 IP 172.22.113.244.43313 > 74.125.28.129.443: Flags [S], seq
2869014603:2869014826, win 29200, options [mss 1460,sackOK,TS val 8785076 ecr
0,nop,wscale 7,Unknown Option 254f9892c33d3033ed66621], length 223

22:16:39.057745 IP 74.125.28.129.443 > 172.22.113.244.43313: Flags [S.], seq
1634457087, ack 2869014827, win 42540, options [mss 1430,sackOK,TS val 710481809
ecr 8785076,nop,wscale 6], length 0
-----

Non-SSL connection setup (Note that no TFO option is sent in the SYN):

22:16:46.467842 IP 172.22.113.244.41432 > 172.16.255.9.3128: Flags [S], seq
3109428839, win 29200, options [mss 1460,sackOK,TS val 8786934 ecr 0,nop,wscale
7], length 0

[Jana, 2014-08-10 05:52:46]

I seem to also be seeing some connections to port 443 that are *not* carrying
the TFO option... I am not sure why this happens. It may be that there's some
logic in the kernel that triggers this behavior, but are there codepaths in
Chrome that may cause this? Is it possible to create SSL connections without
using TransportConnectJob::DoTransportConnect() with "ssl" in the group_name?

[chromium-reviews, 2014-08-10 16:51:31]

You meant they don't carry Fast Open cookie request option, or does
not carry Fast Open cookie option with data?

On Sat, Aug 9, 2014 at 10:52 PM,  <jri@chromium.org> wrote:
> I seem to also be seeing some connections to port 443 that are *not*
> carrying
> the TFO option... I am not sure why this happens. It may be that there's
> some
> logic in the kernel that triggers this behavior, but are there codepaths in
> Chrome that may cause this? Is it possible to create SSL connections without
> using TransportConnectJob::DoTransportConnect() with "ssl" in the
> group_name?
>
> https://codereview.chromium.org/451383002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-08-11 04:43:56]

Yes, it's possible. PPAPI plugins can create raw sockets bypassing our
network stack. Also, if you're doing tcpdump, there could be other
applications creating SSL connections. If you want to make sure that you're
catching all port 443 traffic through our network stack, try adding a
CHECK() to validate your invariant.


On Sun, Aug 10, 2014 at 9:50 AM, Yuchung Cheng <ycheng@google.com> wrote:

> You meant they don't carry Fast Open cookie request option, or does
> not carry Fast Open cookie option with data?
>
> On Sat, Aug 9, 2014 at 10:52 PM,  <jri@chromium.org> wrote:
> > I seem to also be seeing some connections to port 443 that are *not*
> > carrying
> > the TFO option... I am not sure why this happens. It may be that there's
> > some
> > logic in the kernel that triggers this behavior, but are there codepaths
> in
> > Chrome that may cause this? Is it possible to create SSL connections
> without
> > using TransportConnectJob::DoTransportConnect() with "ssl" in the
> > group_name?
> >
> > https://codereview.chromium.org/451383002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Adam Rice, 2014-08-11 05:33:38]

Note this logic is bypassed for WebSockets, since they use
WebSocketTransportClientSocketPool instead of TransportClientSocketPool. That
could be one source of non-TFO SSL connections.

Also, unencrypted WebSocket connections can use port 443. This is actually
useful for getting through some proxies that only permit tunnels on port 443.

https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled = new
SharedBoolean;
user_enabled can be just an ordinary bool. It is not accessed from another
thread, and the value is not written to from within the callback.

https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3) ==
"ssl") {
Should this also be enabled for pm/ssl sockets? I suppose a hostile server could
smuggle information in the cookie to distinguish hosts behind a NAT device, so
maybe not?

[Randy Smith (Not in Mondays), 2014-08-11 19:07:01]

This looks basically good to me (modulo issues below) but it should be reviewed
by someone who knows the network stack architecture and can make a judgement as
to whether the amount of violence being done to the layering is "too much" (and
who can suggest alternatives if it is).

https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h#n...
net/socket/stream_socket.h:83: virtual void EnableTCPFastOpen() {}
I dislike the idea of the stream socket abstraction knowing about the TCP Fast
Open concept.  I'd instead rather have this be "EnableIdempotentFirstWrite" or
"EnableConnectionWriteCombination" or something similarly abstract.  You can
turn that into enabling fast open for the concrete sockets for which it makes
sense.

https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
net/socket/tcp_socket.cc:18: bool g_tcp_fastopen_supported = false;
Is there a reason why all this logic is here rather than in
tcp_socket_libevent.cc?  It seems like it's all used there, and this code isn't
tied to TCPSocket in any way I see, so wouldn't it make sense to have that code
over there?

https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled = new
SharedBoolean;
On 2014/08/11 05:33:38, Adam Rice wrote:
> user_enabled can be just an ordinary bool. It is not accessed from another
> thread, and the value is not written to from within the callback.

Hmmm.  Alternatively to using PostTaskAndReply, I think you could use
PostTaskAndReplayWithResult(base::WorkerPool::GetTaskRunner()) and skip any
SharedBooleans at all.

https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
net/socket/transport_client_socket_pool.cc:198: group_name_(group_name) {
Make sure to get the socket pool changes reviewed by someone wise in the ways of
socket pools (Will's fine, Matt Menke would be my suggested backup).  I wince at
having to add another variable to the class just to track the group name--isn't
is already available somewhere?--but I don't know enough to know if that's a
reasonable thought.

[willchan no longer on Chromium, 2014-08-11 22:24:55]

I defer to Matt. Trying to shed more load :) Let me know if there's
anything in particular I really ought to look at.


On Mon, Aug 11, 2014 at 12:07 PM, <rdsmith@chromium.org> wrote:

> This looks basically good to me (modulo issues below) but it should be
> reviewed
> by someone who knows the network stack architecture and can make a
> judgement as
> to whether the amount of violence being done to the layering is "too much"
> (and
> who can suggest alternatives if it is).
>
>
> https://codereview.chromium.org/451383002/diff/1/net/
> socket/stream_socket.h
> File net/socket/stream_socket.h (right):
>
> https://codereview.chromium.org/451383002/diff/1/net/
> socket/stream_socket.h#newcode83
> net/socket/stream_socket.h:83: virtual void EnableTCPFastOpen() {}
> I dislike the idea of the stream socket abstraction knowing about the
> TCP Fast Open concept.  I'd instead rather have this be
> "EnableIdempotentFirstWrite" or "EnableConnectionWriteCombination" or
> something similarly abstract.  You can turn that into enabling fast open
> for the concrete sockets for which it makes sense.
>
>
> https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
> File net/socket/tcp_socket.cc (right):
>
> https://codereview.chromium.org/451383002/diff/1/net/
> socket/tcp_socket.cc#newcode18
> net/socket/tcp_socket.cc:18: bool g_tcp_fastopen_supported = false;
> Is there a reason why all this logic is here rather than in
> tcp_socket_libevent.cc?  It seems like it's all used there, and this
> code isn't tied to TCPSocket in any way I see, so wouldn't it make sense
> to have that code over there?
>
>
> https://codereview.chromium.org/451383002/diff/1/net/
> socket/tcp_socket.cc#newcode64
> net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled =
> new SharedBoolean;
> On 2014/08/11 05:33:38, Adam Rice wrote:
>
>> user_enabled can be just an ordinary bool. It is not accessed from
>>
> another
>
>> thread, and the value is not written to from within the callback.
>>
>
> Hmmm.  Alternatively to using PostTaskAndReply, I think you could use
> PostTaskAndReplayWithResult(base::WorkerPool::GetTaskRunner()) and skip
> any SharedBooleans at all.
>
>
> https://codereview.chromium.org/451383002/diff/1/net/
> socket/transport_client_socket_pool.cc
> File net/socket/transport_client_socket_pool.cc (right):
>
> https://codereview.chromium.org/451383002/diff/1/net/
> socket/transport_client_socket_pool.cc#newcode198
> net/socket/transport_client_socket_pool.cc:198: group_name_(group_name)
> {
> Make sure to get the socket pool changes reviewed by someone wise in the
> ways of socket pools (Will's fine, Matt Menke would be my suggested
> backup).  I wince at having to add another variable to the class just to
> track the group name--isn't is already available somewhere?--but I don't
> know enough to know if that's a reasonable thought.
>
> https://codereview.chromium.org/451383002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jana, 2014-08-12 00:10:51]

Yuchung: No, I meant negotiating TFO option in the handshake. I saw some TCP
connections going to port 443 that didn't carry [Unknown Option 254f989], which
I believevd to be the TFO option negotiation. Is that correct?

I'm running a fresh Chromium build, and I've not installed any plugins, so I
don't know what is doing this... it is entirely possible that it's not Chrome at
all since I'm looking at a tcpdump.

I'm not sure a CHECK() is warranted ... there's no reason to fail if some SSL
connections don't do TFO. That said, what I want to know if there's a big hunk
of SSL connections that this hackish group_name_ check in DoTransportConnect()
misses. Ideally it's an UMA-able value... where would you insert this CHECK() if
you did? I could DCHECK() it at least and/or add an UMA histogram for it.

[Randy Smith (Not in Mondays), 2014-08-12 00:14:38]

Matt, you should be aware you've been added as a reviewer; see comments from
Will and I.

[willchan no longer on Chromium, 2014-08-12 00:15:04]

Just to be clear, I meant a CHECK in your local dev build so you can
verify. I wouldn't advocate landing that :P


On Mon, Aug 11, 2014 at 5:14 PM, <rdsmith@chromium.org> wrote:

> Matt, you should be aware you've been added as a reviewer; see comments
> from
> Will and I.
>
> https://codereview.chromium.org/451383002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jana, 2014-08-12 00:17:31]

Matt: I thought that adding you as a reviewer would result in a separate email,
but it didn't. Thanks, Randy, for pointing it out.

Will: Ah -- ok, that makes more sense :-) Even then, where would you add the
CHECK?

[Jana, 2014-08-12 00:17:35]

Matt: I thought that adding you as a reviewer would result in a separate email,
but it didn't. Thanks, Randy, for pointing it out.

Will: Ah -- ok, that makes more sense :-) Even then, where would you add the
CHECK?

[ycheng, 2014-08-12 02:27:53]

On 2014/08/12 00:10:51, Jana wrote:
> Yuchung: No, I meant negotiating TFO option in the handshake. I saw some TCP
> connections going to port 443 that didn't carry [Unknown Option 254f989],
which
> I believevd to be the TFO option negotiation. Is that correct?
> 
> I'm running a fresh Chromium build, and I've not installed any plugins, so I
> don't know what is doing this... it is entirely possible that it's not Chrome
at
> all since I'm looking at a tcpdump.
> 
> I'm not sure a CHECK() is warranted ... there's no reason to fail if some SSL
> connections don't do TFO. That said, what I want to know if there's a big hunk
> of SSL connections that this hackish group_name_ check in DoTransportConnect()
> misses. Ideally it's an UMA-able value... where would you insert this CHECK()
if
> you did? I could DCHECK() it at least and/or add an UMA histogram for it.

Note that if the client detects recurring TFO SYN drops, it'll disable Fast Open
on that specific path (even if app calls sendmsg(MSG_FASTOPEN), and send a
regular SYN instead). I doubt that's the cause in your case, but FYI.

Details:
http://md-linux.mtv.corp.google.com/lxr/http/source/net/ipv4/tcp_output.c?v=u...

[Jana, 2014-08-12 05:04:23]

On 2014/08/11 05:33:38, Adam Rice wrote:
> Note this logic is bypassed for WebSockets, since they use
> WebSocketTransportClientSocketPool instead of TransportClientSocketPool. That
> could be one source of non-TFO SSL connections.

This is a possibility -- I'll instrument WebSocketTransportConnectionSocketPool
to verify.

> Also, unencrypted WebSocket connections can use port 443. This is actually
> useful for getting through some proxies that only permit tunnels on port 443.

Ack.

> https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
> File net/socket/tcp_socket.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
> net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled = new
> SharedBoolean;
> user_enabled can be just an ordinary bool. It is not accessed from another
> thread, and the value is not written to from within the callback.

Good call -- done.

>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> File net/socket/transport_client_socket_pool.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3)
==
> "ssl") {
> Should this also be enabled for pm/ssl sockets? I suppose a hostile server
could
> smuggle information in the cookie to distinguish hosts behind a NAT device, so
> maybe not?

I don't know what a pm/ssl group is -- can you say more? If we're comfortable
turning on TFO for them as well, then I'm happy to add that in as well.

[Adam Rice, 2014-08-12 05:19:58]

On 2014/08/12 05:04:23, Jana wrote:
> I don't know what a pm/ssl group is -- can you say more? If we're comfortable
> turning on TFO for them as well, then I'm happy to add that in as well.

If for any reason we make a request with cookies disabled, then the request is
made in "privacy mode", which disables SSL Channel ID. To make sure they use
distinct connections, they have a "pm/" prefix on the group name. If you look
at chrome://net-internals/#sockets there are usually a few there, even if you
haven't explicitly blocked any cookies.

[Jana, 2014-08-12 05:25:04]

On 2014/08/11 19:07:01, rdsmith wrote:
> This looks basically good to me (modulo issues below) but it should be
reviewed
> by someone who knows the network stack architecture and can make a judgement
as
> to whether the amount of violence being done to the layering is "too much"
(and
> who can suggest alternatives if it is).

Thanks! I'm happy to have mmenke comment on this code and suggest a way out if
this isn't clean enough.

> https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h
> File net/socket/stream_socket.h (right):
> 
>
https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h#n...
> net/socket/stream_socket.h:83: virtual void EnableTCPFastOpen() {}
> I dislike the idea of the stream socket abstraction knowing about the TCP Fast
> Open concept.  I'd instead rather have this be "EnableIdempotentFirstWrite" or
> "EnableConnectionWriteCombination" or something similarly abstract.  You can
> turn that into enabling fast open for the concrete sockets for which it makes
> sense.

So, I followed code that existed already (which isn't to say that the extant
code was in fact the right way to do it), and I'm happy to change it if
necessary. There's a UsingTCPFastOpen() method that is implemented as a pure
virtual function in StreamSocket, and is overridden in some god-awful child
classes. I've implemented the parallel setter method (EnableTCPFastOpen), but
didn't do it as pure virtual. I can change them both (and introduce some 20-odd
new files into this CL), or leave them both as xxxxTCPFastOpen (and do this name
change in a follow-up CL with a TODO here and/or a bug assigned to myself.) I
don't want to change one and leave the other as is, for consistency's sake.
Wdyt?

> https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
> File net/socket/tcp_socket.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
> net/socket/tcp_socket.cc:18: bool g_tcp_fastopen_supported = false;
> Is there a reason why all this logic is here rather than in
> tcp_socket_libevent.cc?  It seems like it's all used there, and this code
isn't
> tied to TCPSocket in any way I see, so wouldn't it make sense to have that
code
> over there?

The functions are all shared between tcp_socket_libevent and tcp_socket_win;
otherwise you'd have to recreate these methods in both. As I see it TCPSocket
seems to get mapped to TCPSocketWin or TCPSocketLibevent depending on POSIX or
not, and these TFO methods are available to users of both TCPSocketWin and
TCPSocketLibevent. I'm inclined to leave it in tcp_socket unless you think it's
meaningful to reproduce the code in both places... happy to do it if you think
so.

>
https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
> net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled = new
> SharedBoolean;
> On 2014/08/11 05:33:38, Adam Rice wrote:
> > user_enabled can be just an ordinary bool. It is not accessed from another
> > thread, and the value is not written to from within the callback.
> 
> Hmmm.  Alternatively to using PostTaskAndReply, I think you could use
> PostTaskAndReplayWithResult(base::WorkerPool::GetTaskRunner()) and skip any
> SharedBooleans at all.

This seemed like a good idea and I started doing it, but then
PostTaskAndReplyWithResult can handle only one parameter to the callback
method... I have two. Is it worthwhile figuring out how to reorganize the code
to avoid using a shared boolean? How bad is that cost?

>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> File net/socket/transport_client_socket_pool.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> net/socket/transport_client_socket_pool.cc:198: group_name_(group_name) {
> Make sure to get the socket pool changes reviewed by someone wise in the ways
of
> socket pools (Will's fine, Matt Menke would be my suggested backup).  I wince
at
> having to add another variable to the class just to track the group
name--isn't
> is already available somewhere?--but I don't know enough to know if that's a
> reasonable thought.

Ack. Added mmenke as a reviewer. I looked around but couldn't find a place where
group_name was being stored already. I agree -- it seems like overkill to have
to store this for just the one purpose ... perhaps mmenke has a better approach
to offer.

[Jana, 2014-08-12 05:29:41]

On 2014/08/12 05:19:58, Adam Rice wrote:
> On 2014/08/12 05:04:23, Jana wrote:
> > I don't know what a pm/ssl group is -- can you say more? If we're
comfortable
> > turning on TFO for them as well, then I'm happy to add that in as well.
> 
> If for any reason we make a request with cookies disabled, then the request is
> made in "privacy mode", which disables SSL Channel ID. To make sure they use
> distinct connections, they have a "pm/" prefix on the group name. If you look
> at chrome://net-internals/#sockets there are usually a few there, even if you
> haven't explicitly blocked any cookies.

Gotcha. Since we don't want any cookies in Chrome in Privacy Mode, I'm
comfortable with not storing TFO cookies in the kernel as well. It certainly
seems like the consistent way to reason about PM, so I'll leave it as is.

[mmenke, 2014-08-12 15:25:16]

https://codereview.chromium.org/451383002/diff/20001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/451383002/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:856: bool user_enabled_tfo = false;
"tfo" isn't what I'd call a standard abbreviation.  Suggest just using fast_open
or tcp_fast_open.

https://codereview.chromium.org/451383002/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:859: }
nit:  Remove braces.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc...
net/socket/tcp_socket.cc:20: bool g_tcp_fastopen_enabled = false;
Do we really need globals for TCP FastOpen?  May take a little wiring, but most
options are stored in HttpNetworkSession::Params.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc...
net/socket/tcp_socket.cc:63: scoped_refptr<SharedBoolean> system_supported = new
SharedBoolean;
Using a shared boolean seems unnecessary to me.  You can just use
PostTaskAndReplyWithResult instead, which results in a more natural signature
for SystemSupportsTCPFastOpen.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.h
File net/socket/tcp_socket.h (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.h#...
net/socket/tcp_socket.h:28: NET_EXPORT void
CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled_tfo);
Calling this with false does not indicate to me "Enable this for SSL only if
supported, if passed in false."

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.h#...
net/socket/tcp_socket.h:28: NET_EXPORT void
CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled_tfo);
Per earlier comment, "tfo" isn't a standard abbreviation, so should use
something else, either fast_open, or tcp_fast_open... Or just enabled_by_user or
something.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket_li...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.cc:375: }
nit:  There's a preference not to use braces for single line ifs.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.cc:504: // Records fastopen status regardless of
error in asynchronous case.
nit:  Maybe capitalize, too?

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.cc:631: }  // namespace net
nit:  keep the blank line before the end of the namespace.

https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3) ==
"ssl") {
This seems a really bad idea.  Looking at my connections now, I have a
connection to http://ssl.gstatic.com/, and a group (For another connection)
named pm/ssl/ssl.gstatic.com.  This classifies them both incorrectly.

I also think this should be finch trialed in this CL, so we have a shutoff,
particularly if we land this before branch.

[mmenke, 2014-08-12 15:36:44]

https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3) ==
"ssl") {
And just to elaborate a little, I don't think the transport client socket should
know or care if we're going to use SSL.  I suggest making use fastopen a
TransportSocketParam instead, and having the SSL code set it to true.

[Jana, 2014-08-12 16:05:31]

On 2014/08/12 15:36:44, mmenke wrote:
>
https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
> File net/socket/transport_client_socket_pool.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
> net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3)
==
> "ssl") {
> And just to elaborate a little, I don't think the transport client socket
should
> know or care if we're going to use SSL.  I suggest making use fastopen a
> TransportSocketParam instead, and having the SSL code set it to true.

A quick couple of responses, Matt:
1. Yes, the intent is to Finch trial this. I was looking for feedback on the
approach used here -- and so, thanks for reviewing the code :-) I will set up
the Finch trial soon.
2. I like what your suggestion of using TransportSocketParam, and I will try to
make it so.

[Randy Smith (Not in Mondays), 2014-08-12 16:12:25]

On 2014/08/12 16:05:31, Jana wrote:
> On 2014/08/12 15:36:44, mmenke wrote:
> >
>
https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
> > File net/socket/transport_client_socket_pool.cc (right):
> > 
> >
>
https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
> > net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3)
> ==
> > "ssl") {
> > And just to elaborate a little, I don't think the transport client socket
> should
> > know or care if we're going to use SSL.  I suggest making use fastopen a
> > TransportSocketParam instead, and having the SSL code set it to true.
> 
> A quick couple of responses, Matt:
> 1. Yes, the intent is to Finch trial this. I was looking for feedback on the
> approach used here -- and so, thanks for reviewing the code :-) I will set up
> the Finch trial soon.
> 2. I like what your suggestion of using TransportSocketParam, and I will try
to
> make it so.

I coulda sworn there was some problem with using TransportSocketParam (i.e. that
we considered that idea and dropped it) but I can't remember details and
scanning my notes doesn't show up anything.  So hopefully I'm imagining things
:-}.

[Randy Smith (Not in Mondays), 2014-08-12 18:32:28]

On 2014/08/12 05:25:04, Jana wrote:
> On 2014/08/11 19:07:01, rdsmith wrote:
> > This looks basically good to me (modulo issues below) but it should be
> reviewed
> > by someone who knows the network stack architecture and can make a judgement
> as
> > to whether the amount of violence being done to the layering is "too much"
> (and
> > who can suggest alternatives if it is).
> 
> Thanks! I'm happy to have mmenke comment on this code and suggest a way out if
> this isn't clean enough.
> 
> > https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h
> > File net/socket/stream_socket.h (right):
> > 
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h#n...
> > net/socket/stream_socket.h:83: virtual void EnableTCPFastOpen() {}
> > I dislike the idea of the stream socket abstraction knowing about the TCP
Fast
> > Open concept.  I'd instead rather have this be "EnableIdempotentFirstWrite"
or
> > "EnableConnectionWriteCombination" or something similarly abstract.  You can
> > turn that into enabling fast open for the concrete sockets for which it
makes
> > sense.
> 
> So, I followed code that existed already (which isn't to say that the extant
> code was in fact the right way to do it), and I'm happy to change it if
> necessary. There's a UsingTCPFastOpen() method that is implemented as a pure
> virtual function in StreamSocket, and is overridden in some god-awful child
> classes. I've implemented the parallel setter method (EnableTCPFastOpen), but
> didn't do it as pure virtual. I can change them both (and introduce some
20-odd
> new files into this CL), or leave them both as xxxxTCPFastOpen (and do this
name
> change in a follow-up CL with a TODO here and/or a bug assigned to myself.) I
> don't want to change one and leave the other as is, for consistency's sake.
> Wdyt?

Wow, UsingTCPFastOpen() looks like a mess.  I can't find anywhere it's "really"
called (i.e. where it's called other than to implement a containing sockets
implementation of the same call).  Can you?  Can we get rid of it?

WRT the larger issue: I'm good with a todo and later cleanup.

Side note on Rietveld: If you respond to comments via the "reply" button on the
comment where it's embedded in the diff, that keeps all the conversation about a
particular issue in one place, which makes it easier to review in the future.

> 
> > https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
> > File net/socket/tcp_socket.cc (right):
> > 
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
> > net/socket/tcp_socket.cc:18: bool g_tcp_fastopen_supported = false;
> > Is there a reason why all this logic is here rather than in
> > tcp_socket_libevent.cc?  It seems like it's all used there, and this code
> isn't
> > tied to TCPSocket in any way I see, so wouldn't it make sense to have that
> code
> > over there?
> 
> The functions are all shared between tcp_socket_libevent and tcp_socket_win;
> otherwise you'd have to recreate these methods in both. As I see it TCPSocket
> seems to get mapped to TCPSocketWin or TCPSocketLibevent depending on POSIX or
> not, and these TFO methods are available to users of both TCPSocketWin and
> TCPSocketLibevent. I'm inclined to leave it in tcp_socket unless you think
it's
> meaningful to reproduce the code in both places... happy to do it if you think
> so.

Nah, I hadn't thought about that wrinkle.  Fine to leave them where they are.

> 
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
> > net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled = new
> > SharedBoolean;
> > On 2014/08/11 05:33:38, Adam Rice wrote:
> > > user_enabled can be just an ordinary bool. It is not accessed from another
> > > thread, and the value is not written to from within the callback.
> > 
> > Hmmm.  Alternatively to using PostTaskAndReply, I think you could use
> > PostTaskAndReplayWithResult(base::WorkerPool::GetTaskRunner()) and skip any
> > SharedBooleans at all.
> 
> This seemed like a good idea and I started doing it, but then
> PostTaskAndReplyWithResult can handle only one parameter to the callback
> method... I have two. Is it worthwhile figuring out how to reorganize the code
> to avoid using a shared boolean? How bad is that cost?

Technical response: You can use a std::pair<> as a very simple fix. 
Alternatively, you could just do a PostTask where one of the argument is the
callback to post a reply to, have the handling routine PostTask back to invoke
that routine, and have that routine take two booleans as arguments.  There might
be a trickiness there in figuring out what task runner to post to, but I suspect
that problem's been solved in many places in the network code.

Preference response: Completely up to you what you do; I'm just giving you
options.  If I were you, I'd probably do the explicit PostTask with callback and
figure out how to make sure it got posted back to the correct task runner, but I
don't consider it a big deal.

> 
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> > File net/socket/transport_client_socket_pool.cc (right):
> > 
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> > net/socket/transport_client_socket_pool.cc:198: group_name_(group_name) {
> > Make sure to get the socket pool changes reviewed by someone wise in the
ways
> of
> > socket pools (Will's fine, Matt Menke would be my suggested backup).  I
wince
> at
> > having to add another variable to the class just to track the group
> name--isn't
> > is already available somewhere?--but I don't know enough to know if that's a
> > reasonable thought.
> 
> Ack. Added mmenke as a reviewer. I looked around but couldn't find a place
where
> group_name was being stored already. I agree -- it seems like overkill to have
> to store this for just the one purpose ... perhaps mmenke has a better
approach
> to offer.

[mmenke, 2014-08-12 18:44:23]

Sorry, I didn't read the original comments.

https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled = new
SharedBoolean;
On 2014/08/11 19:07:01, rdsmith wrote:
> On 2014/08/11 05:33:38, Adam Rice wrote:
> > user_enabled can be just an ordinary bool. It is not accessed from another
> > thread, and the value is not written to from within the callback.
> 
> Hmmm.  Alternatively to using PostTaskAndReply, I think you could use
> PostTaskAndReplayWithResult(base::WorkerPool::GetTaskRunner()) and skip any
> SharedBooleans at all.  

You can bind just one argument, actually...

base::WorkerPool::PostTaskAndReplyWithResult(
  FROM_HERE,
  // Do you need bind when not passing in arguments?
  base::Bind(SystemSupportsTCPFastOpen),
  base::Bind(CallbackForTCPFastOpenCheck, user_enabled),
  false);

Then just reverse the arguments order for CallbackForTCPFastOpenCheck, and make
them both bools, and you're done.

[Jana, 2014-08-13 07:33:40]

Randy, Matt, Thanks for your detailed reviews. (Randy -- I've taken your advice
on replying inline.) I've removed the shared bools, used
PostTaskAndReplyWithResult, and moved the decision of using FastOpen to
ssl_client_socket_pool, with signaling via a new member in
TransportSocketParams. I've added TODOs for cleanup.

I'm yet to (i) test this (will do tomorrow morning), and (ii) set up Finch trial
and add that code. In the meanwhile, can you PTAL and let me know if you're
comfortable with the code changes so far?

https://codereview.chromium.org/451383002/diff/20001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/451383002/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:856: bool user_enabled_tfo = false;
On 2014/08/12 15:25:15, mmenke wrote:
> "tfo" isn't what I'd call a standard abbreviation.  Suggest just using
fast_open
> or tcp_fast_open.

Changed, PTAL.

https://codereview.chromium.org/451383002/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:859: }
On 2014/08/12 15:25:15, mmenke wrote:
> nit:  Remove braces.

Done.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc...
net/socket/tcp_socket.cc:20: bool g_tcp_fastopen_enabled = false;
On 2014/08/12 15:25:15, mmenke wrote:
> Do we really need globals for TCP FastOpen?  May take a little wiring, but
most
> options are stored in HttpNetworkSession::Params.

I'll see how this might be done. Given that this code was already in place, I'm
considering leaving a TODO here alongwith the other TODOs from Randy for a
cleanup CL. Are you comfortable with that, or would you rather that I address
this issue in this CL?

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc...
net/socket/tcp_socket.cc:63: scoped_refptr<SharedBoolean> system_supported = new
SharedBoolean;
On 2014/08/12 15:25:15, mmenke wrote:
> Using a shared boolean seems unnecessary to me.  You can just use
> PostTaskAndReplyWithResult instead, which results in a more natural signature
> for SystemSupportsTCPFastOpen.

Done.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.h
File net/socket/tcp_socket.h (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.h#...
net/socket/tcp_socket.h:28: NET_EXPORT void
CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled_tfo);
On 2014/08/12 15:25:15, mmenke wrote:
> Per earlier comment, "tfo" isn't a standard abbreviation, so should use
> something else, either fast_open, or tcp_fast_open... Or just enabled_by_user
or
> something.

Done.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.h#...
net/socket/tcp_socket.h:28: NET_EXPORT void
CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled_tfo);
On 2014/08/12 15:25:15, mmenke wrote:
> Calling this with false does not indicate to me "Enable this for SSL only if
> supported, if passed in false."

Changed, PTAL. The initial intent was to indicate "Enable this for all
connections if true is passed in." I hope the new signaling is better ... if
not, please feel free to suggest something better.

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket_li...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket_li...
net/socket/tcp_socket_libevent.cc:375: }
On 2014/08/12 15:25:16, mmenke wrote:
> nit:  There's a preference not to use braces for single line ifs.

Done.

https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3) ==
"ssl") {
On 2014/08/12 15:25:16, mmenke wrote:
> This seems a really bad idea.  Looking at my connections now, I have a
> connection to http://ssl.gstatic.com/, and a group (For another connection)
> named http://pm/ssl/ssl.gstatic.com.  This classifies them both incorrectly.
> 
> I also think this should be finch trialed in this CL, so we have a shutoff,
> particularly if we land this before branch.

Acknowledged.

https://codereview.chromium.org/451383002/diff/20001/net/socket/transport_cli...
net/socket/transport_client_socket_pool.cc:270: if (group_name_.substr(0, 3) ==
"ssl") {
On 2014/08/12 15:36:44, mmenke wrote:
> And just to elaborate a little, I don't think the transport client socket
should
> know or care if we're going to use SSL.  I suggest making use fastopen a
> TransportSocketParam instead, and having the SSL code set it to true.

Changed to use TransportSocketParams, PTAL. (I've not done the Finch setup yet.)

[Jana, 2014-08-13 07:44:35]

A couple more things on PostTaskAndReplyWithResult:
(i) I finally looked into base::Bind, and it's quite cool -- I was able to do it
using mmenke's suggested code (for the most part).
(ii) Yes, you need to use Bind on even a single method with no args. The type
needs to be whatever Bind returns (Callback?), and not a function name.

[mmenke, 2014-08-13 15:03:07]

https://codereview.chromium.org/451383002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_pool.cc:312:
direct_params->enable_use_tcp_fastopen();
Hrm...It looks like nothing else modifies these params after construction. 
Given that they're refcounted as well, that makes me a little nervous.  It's
theoretically possible for the same params to be used for multiple calls to this
(Does it happen?  Maybe with multiple preconnects?  Which will still work, but
that makes me nervouse).

I see two ways around this:  Create a new params object here, or move this logic
into net/socket/client_socket_pool_manager.cc.  The latter may be the better
option - that's where we create the parameters in the first place, though it is
rather a mess.

[mmenke, 2014-08-13 15:36:50]

Oh, and forgot to response to your question

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/451383002/diff/20001/net/socket/tcp_socket.cc...
net/socket/tcp_socket.cc:20: bool g_tcp_fastopen_enabled = false;
On 2014/08/13 07:33:40, Jana wrote:
> On 2014/08/12 15:25:15, mmenke wrote:
> > Do we really need globals for TCP FastOpen?  May take a little wiring, but
> most
> > options are stored in HttpNetworkSession::Params.
> 
> I'll see how this might be done. Given that this code was already in place,
I'm
> considering leaving a TODO here alongwith the other TODOs from Randy for a
> cleanup CL. Are you comfortable with that, or would you rather that I address
> this issue in this CL?

Fine with a TODO, for now, though I suspect that means it will be grandfathered
in for quite a while.  :(

[Randy Smith (Not in Mondays), 2014-08-13 18:13:41]

On 2014/08/12 18:32:28, rdsmith wrote:
> On 2014/08/12 05:25:04, Jana wrote:
> > On 2014/08/11 19:07:01, rdsmith wrote:
> > > This looks basically good to me (modulo issues below) but it should be
> > reviewed
> > > by someone who knows the network stack architecture and can make a
judgement
> > as
> > > to whether the amount of violence being done to the layering is "too much"
> > (and
> > > who can suggest alternatives if it is).
> > 
> > Thanks! I'm happy to have mmenke comment on this code and suggest a way out
if
> > this isn't clean enough.
> > 
> > >
https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h
> > > File net/socket/stream_socket.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/stream_socket.h#n...
> > > net/socket/stream_socket.h:83: virtual void EnableTCPFastOpen() {}
> > > I dislike the idea of the stream socket abstraction knowing about the TCP
> Fast
> > > Open concept.  I'd instead rather have this be
"EnableIdempotentFirstWrite"
> or
> > > "EnableConnectionWriteCombination" or something similarly abstract.  You
can
> > > turn that into enabling fast open for the concrete sockets for which it
> makes
> > > sense.
> > 
> > So, I followed code that existed already (which isn't to say that the extant
> > code was in fact the right way to do it), and I'm happy to change it if
> > necessary. There's a UsingTCPFastOpen() method that is implemented as a pure
> > virtual function in StreamSocket, and is overridden in some god-awful child
> > classes. I've implemented the parallel setter method (EnableTCPFastOpen),
but
> > didn't do it as pure virtual. I can change them both (and introduce some
> 20-odd
> > new files into this CL), or leave them both as xxxxTCPFastOpen (and do this
> name
> > change in a follow-up CL with a TODO here and/or a bug assigned to myself.)
I
> > don't want to change one and leave the other as is, for consistency's sake.
> > Wdyt?
> 
> Wow, UsingTCPFastOpen() looks like a mess.  I can't find anywhere it's
"really"
> called (i.e. where it's called other than to implement a containing sockets
> implementation of the same call).  Can you?  Can we get rid of it?
> 
> WRT the larger issue: I'm good with a todo and later cleanup.
> 
> Side note on Rietveld: If you respond to comments via the "reply" button on
the
> comment where it's embedded in the diff, that keeps all the conversation about
a
> particular issue in one place, which makes it easier to review in the future.
> 
> > 
> > > https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc
> > > File net/socket/tcp_socket.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
> > > net/socket/tcp_socket.cc:18: bool g_tcp_fastopen_supported = false;
> > > Is there a reason why all this logic is here rather than in
> > > tcp_socket_libevent.cc?  It seems like it's all used there, and this code
> > isn't
> > > tied to TCPSocket in any way I see, so wouldn't it make sense to have that
> > code
> > > over there?
> > 
> > The functions are all shared between tcp_socket_libevent and tcp_socket_win;
> > otherwise you'd have to recreate these methods in both. As I see it
TCPSocket
> > seems to get mapped to TCPSocketWin or TCPSocketLibevent depending on POSIX
or
> > not, and these TFO methods are available to users of both TCPSocketWin and
> > TCPSocketLibevent. I'm inclined to leave it in tcp_socket unless you think
> it's
> > meaningful to reproduce the code in both places... happy to do it if you
think
> > so.
> 
> Nah, I hadn't thought about that wrinkle.  Fine to leave them where they are.

Actually, I'd like to re-open this thread.  My grepping suggests that
IsTCPFastOpenSupported() & 
IsTCPFastOpenEnabled() are defined in tcp_socket.* but only used in
TCPSocketLibevent.  
CheckAndMaybeEnableTCPFastOpen is used in io_thread.cc, and calls the other two,
but inside an ifdef 
that should mean they're always defined (although now that I look closely there
seems to be an ifdef 
mismatch between header and body; OS_POSIX vs. OS_LINUX_ and OS_ANDROID).  But
based on all that, 
it would seem like we could move everything besides
CheckAndMaybeEnableTCPFastOpen to tcp_socket_libevent.*.
WDYT?  (Not deeply invested, just wanting to explore IMO slightly cleaner code.)

> 
> > 
> > >
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/tcp_socket.cc#new...
> > > net/socket/tcp_socket.cc:64: scoped_refptr<SharedBoolean> user_enabled =
new
> > > SharedBoolean;
> > > On 2014/08/11 05:33:38, Adam Rice wrote:
> > > > user_enabled can be just an ordinary bool. It is not accessed from
another
> > > > thread, and the value is not written to from within the callback.
> > > 
> > > Hmmm.  Alternatively to using PostTaskAndReply, I think you could use
> > > PostTaskAndReplayWithResult(base::WorkerPool::GetTaskRunner()) and skip
any
> > > SharedBooleans at all.
> > 
> > This seemed like a good idea and I started doing it, but then
> > PostTaskAndReplyWithResult can handle only one parameter to the callback
> > method... I have two. Is it worthwhile figuring out how to reorganize the
code
> > to avoid using a shared boolean? How bad is that cost?
> 
> Technical response: You can use a std::pair<> as a very simple fix. 
> Alternatively, you could just do a PostTask where one of the argument is the
> callback to post a reply to, have the handling routine PostTask back to invoke
> that routine, and have that routine take two booleans as arguments.  There
might
> be a trickiness there in figuring out what task runner to post to, but I
suspect
> that problem's been solved in many places in the network code.
> 
> Preference response: Completely up to you what you do; I'm just giving you
> options.  If I were you, I'd probably do the explicit PostTask with callback
and
> figure out how to make sure it got posted back to the correct task runner, but
I
> don't consider it a big deal.
> 
> > 
> > >
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> > > File net/socket/transport_client_socket_pool.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/451383002/diff/1/net/socket/transport_client_...
> > > net/socket/transport_client_socket_pool.cc:198: group_name_(group_name) {
> > > Make sure to get the socket pool changes reviewed by someone wise in the
> ways
> > of
> > > socket pools (Will's fine, Matt Menke would be my suggested backup).  I
> wince
> > at
> > > having to add another variable to the class just to track the group
> > name--isn't
> > > is already available somewhere?--but I don't know enough to know if that's
a
> > > reasonable thought.
> > 
> > Ack. Added mmenke as a reviewer. I looked around but couldn't find a place
> where
> > group_name was being stored already. I agree -- it seems like overkill to
have
> > to store this for just the one purpose ... perhaps mmenke has a better
> approach
> > to offer.

[Randy Smith (Not in Mondays), 2014-08-13 18:15:39]

This is looking good to me, but I want to hold off on stamping until after the
finch trial is incorporated.

[Jana, 2014-08-14 06:35:31]

Thanks again, Matt and Randy, for your detailed review. I believe I have
addressed all points (modulo Finch) in this patch-set. I have (i) moved the TFO
enabling logic to client_socket_pool_manager.cc, and (ii) moved all the
TFO-related functions from tcp_socket.cc to tcp_socket_libevent.cc. I haven't
tested this out yet; I will do it when I'm back in the office tomorrow morning.

Matt: I hope to get to the TODO soon, but I understand your concern. I'd like to
get this trial started, since it's been hanging for quite a while. I will get on
the cleanup next. I would also like some help in understanding the required
plumbing, and I will bug you for it after this CL.

I have a few requests/questions/comments:
(i) I would really appreciate a careful and expert examination of the changes
I've made to client_socket_pool_manager.cc.
(ii) I don't think tcp_socket.cc is needed anymore, but I'm not sure what I need
to do to remove it. There seem to be dependencies, and I'm not sure where to
look for them and remove them (this is probably embarrassingly obvious, but
then, I don't embarrass easily :-)). I'm also happy to leave the file as it is
-- empty -- for possible future use.

https://codereview.chromium.org/451383002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_pool.cc:312:
direct_params->enable_use_tcp_fastopen();
On 2014/08/13 15:03:07, mmenke wrote:
> Hrm...It looks like nothing else modifies these params after construction. 
> Given that they're refcounted as well, that makes me a little nervous.  It's
> theoretically possible for the same params to be used for multiple calls to
this
> (Does it happen?  Maybe with multiple preconnects?  Which will still work, but
> that makes me nervouse).
> 
> I see two ways around this:  Create a new params object here, or move this
logic
> into net/socket/client_socket_pool_manager.cc.  The latter may be the better
> option - that's where we create the parameters in the first place, though it
is
> rather a mess.

I see... I understand, and I agree. I've modified client_socket_pool_manager to
do this with a new TransportSocketParams object -- PTAL. I would appreciate a
careful read of that code... I don't think I'm breaking anything but a couple of
more pairs of eyes are definitely helpful!

[mmenke, 2014-08-14 17:56:03]

Very quick pass

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:156: bool ignore_limits =
(request_load_flags & LOAD_IGNORE_LIMITS) != 0;
The struct was effectively const before, suggest keeping that way, and adding a
parameter.  You can keep the old construction order, but instead pass in
using_ssl as the new argument when you construct it here...

I'm also fine with splitting the constructor calls in the SSL and non-SSL cases,
as you're doing here (With the additional constructor parameter), just not sure
how much it gets us.

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:292: resolution_callback);
nit:  +2 indent

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:292: resolution_callback);
Shouldn't we be enabling fastopen if g_tcp_fastopen_enabled is true?

https://codereview.chromium.org/451383002/diff/80001/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/451383002/diff/80001/net/socket/tcp_socket.cc...
net/socket/tcp_socket.cc:2: // Use of this source code is governed by a
BSD-style license that can be
Don't need this file any more.

https://codereview.chromium.org/451383002/diff/80001/net/socket/transport_cli...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/80001/net/socket/transport_cli...
net/socket/transport_client_socket_pool.h:56: bool use_tcp_fastopen_;
Maybe use_tcp_fastopen_if_supported_?

[Jana, 2014-08-15 20:00:07]

Comments addressed, PTAL. Once this code is kosher, I'll add Finch code.

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:156: bool ignore_limits =
(request_load_flags & LOAD_IGNORE_LIMITS) != 0;
On 2014/08/14 17:56:03, mmenke wrote:
> The struct was effectively const before, suggest keeping that way, and adding
a
> parameter.  You can keep the old construction order, but instead pass in
> using_ssl as the new argument when you construct it here...
> 
> I'm also fine with splitting the constructor calls in the SSL and non-SSL
cases,
> as you're doing here (With the additional constructor parameter), just not
sure
> how much it gets us.

As we discussed offline, I've added a new constructor that accepts this param.
I've also added a TODO to clean this up by replacing the single constructor
instead of adding a new one. Not doing it in this CL to avoid touching many more
files here.

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:292: resolution_callback);
On 2014/08/14 17:56:03, mmenke wrote:
> Shouldn't we be enabling fastopen if g_tcp_fastopen_enabled is true?

Yes -- but the global is being used on *all* sockets and therefore enabled
further below

https://codereview.chromium.org/451383002/diff/80001/net/socket/transport_cli...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/80001/net/socket/transport_cli...
net/socket/transport_client_socket_pool.h:56: bool use_tcp_fastopen_;
On 2014/08/14 17:56:03, mmenke wrote:
> Maybe use_tcp_fastopen_if_supported_?

Done.

[Jana, 2014-08-16 00:01:11]

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:292: resolution_callback);
On 2014/08/15 20:00:06, Jana wrote:
> On 2014/08/14 17:56:03, mmenke wrote:
> > Shouldn't we be enabling fastopen if g_tcp_fastopen_enabled is true?
> 
> Yes -- but the global is being used on *all* sockets and therefore enabled
> further below

Sorry, I seem to have left this sentence hanging. I meant to say that I enable
it in tcp_socket_libevent.cc, because this global is used to turn TFO on for all
TCP sockets.

[mmenke, 2014-08-18 16:01:02]

On 2014/08/16 00:01:11, Jana wrote:
>
https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
> File net/socket/client_socket_pool_manager.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
> net/socket/client_socket_pool_manager.cc:292: resolution_callback);
> On 2014/08/15 20:00:06, Jana wrote:
> > On 2014/08/14 17:56:03, mmenke wrote:
> > > Shouldn't we be enabling fastopen if g_tcp_fastopen_enabled is true?
> > 
> > Yes -- but the global is being used on *all* sockets and therefore enabled
> > further below
> 
> Sorry, I seem to have left this sentence hanging. I meant to say that I enable
> it in tcp_socket_libevent.cc, because this global is used to turn TFO on for
all
> TCP sockets.

I think we should only have one mechanism for enabling Fast Open, and use that
one mechanism everywhere.  Due to the POST problem, I assume we'll never be able
to safely enable it for all connections, anyways...  Or am I wrong?

[mmenke, 2014-08-19 16:09:13]

One question...what about people who can lookup IPv6 DNS addresses, but all IPv6
connection attempts time out on?

[Randy Smith (Not in Mondays), 2014-08-19 18:25:43]

https://codereview.chromium.org/451383002/diff/100001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/100001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:123: void
CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled) {
This looks to be called on all platforms but not defined on Windows--am I
confused?

[Jana, 2014-08-22 03:27:07]

On 2014/08/18 16:01:02, mmenke wrote:
> On 2014/08/16 00:01:11, Jana wrote:
> >
>
https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
> > File net/socket/client_socket_pool_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/451383002/diff/80001/net/socket/client_socket...
> > net/socket/client_socket_pool_manager.cc:292: resolution_callback);
> > On 2014/08/15 20:00:06, Jana wrote:
> > > On 2014/08/14 17:56:03, mmenke wrote:
> > > > Shouldn't we be enabling fastopen if g_tcp_fastopen_enabled is true?
> > > 
> > > Yes -- but the global is being used on *all* sockets and therefore enabled
> > > further below
> > 
> > Sorry, I seem to have left this sentence hanging. I meant to say that I
enable
> > it in tcp_socket_libevent.cc, because this global is used to turn TFO on for
> all
> > TCP sockets.
> 
> I think we should only have one mechanism for enabling Fast Open, and use that
> one mechanism everywhere.  Due to the POST problem, I assume we'll never be
able
> to safely enable it for all connections, anyways...  Or am I wrong?

Matt -- per our conversation earlier, I will change the code to use a 3-state
variable for the TCPSocketParams constructor so that the entire enabling code
lies in one place, and change the user controls on TCP Fast Open to have ENABLE,
DISABLE, and DEFAULT.

[Jana, 2014-08-22 03:32:08]

On 2014/08/19 16:09:13, mmenke wrote:
> One question...what about people who can lookup IPv6 DNS addresses, but all
IPv6
> connection attempts time out on?

Hmm? Are you asking about Happy Eyeballs with TCP connects? This is a good
question ... since both connects will return immediately with a success, we will
use the v6 address, and then fail when we try a TFO connect. I'll look into it
further tomorrow and get back on this.

[Jana, 2014-08-22 03:35:19]

Quick response to Randy's comment.

https://codereview.chromium.org/451383002/diff/100001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/100001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:123: void
CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled) {
On 2014/08/19 18:25:43, rdsmith wrote:
> This looks to be called on all platforms but not defined on Windows--am I
> confused?

Ah -- Great catch! Yes, it is not defined... will fix it.

[mmenke, 2014-08-22 14:18:13]

On 2014/08/22 03:32:08, Jana wrote:
> On 2014/08/19 16:09:13, mmenke wrote:
> > One question...what about people who can lookup IPv6 DNS addresses, but all
> IPv6
> > connection attempts time out on?
> 
> Hmm? Are you asking about Happy Eyeballs with TCP connects? This is a good
> question ... since both connects will return immediately with a success, we
will
> use the v6 address, and then fail when we try a TFO connect. I'll look into it
> further tomorrow and get back on this.

Yes, I'm talking exactly about "Happy Eyeballs" (Why couldn't they have named it
something logical?  Like "IPv4 fallback"?)

[Jana, 2014-09-04 20:30:42]

I've made a few changes, PTAL. I'm not turning TFO for SSL on in this CL -- I'm
planning to do that separately in a CL that sets up the Finch experiment
plumbing. Changes in the past 3 patch-sets:
(i) Changed name TransportSocketParams and above to AutoConnectWithWrite.
(ii) Changed the TransportSocketParams constructor to use a 3-state parameter
for auto_connect_with_write_if_supported.
(iii) Fixed refactored code in InitSocketPoolHelper that was causing tests to
fail.

[Jana, 2014-09-04 20:53:22]

Matt: Per our offline conversation, I've refactored the TFO enabling logic to be
all in one place (TransportSocketParams constructor). PTAL.

On the question of Happy Eyeballs: At the moment, this Happy Eyeballs will not
work as designed, because v6 connect will return immediately as success. This
leads to a matrix of possible cases -- v4/v6 with and without TFO -- and ideally
you'd want to explore all possibilities. I think the simplest fix for now may be
to not use TCP Fast Open when we are racing v4 and v6, and revisit this question
separately. WDYT?

(It was a name conceived by Andrew Yourtchenko, I believe, and he was trying to
ameliorate the latency of failovers from v6 to v4 when using dual stacks...
lower latency == happier users == happier eyeballs. Like most catchy names, it's
not shooting for accuracy or descriptiveness. It's a transferred epithet, which
is kinda nice... if only because I get to say "transferred epithet" in a
Chromium CL :-))

[mmenke, 2014-09-05 19:36:42]

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:182: ssl_params = new
SSLSocketParams(proxy_tcp_params,
-2 indent

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:191: proxy_tcp_params = NULL;
-2 indent

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:230: // for SSL sockets.
Make sure to update your CL description to reflect this.

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:298: new
TransportSocketParams(origin_host_port,
+2 indent.

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:83: static const base::FilePath::CharType
kTCPFastOpenProcFilePath[] =
I seem to recall reading on Chromium dev that static in this case may result in
worse code on some platforms.

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:98: void CallbackForTCPFastOpenCheck(bool
user_enabled,
Functions should generally be named after what they do, not what calls them.

Suggest calling this SetTCPFastOpenConfig (Options?  Settings?  Or just
SetupTCPFastOpen?  Open to other ideas).

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:119:
base::WorkerPool::GetTaskRunner(/*task_is_slow=*/false),
File IO isn't considered slow?

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:423: if (!use_tcp_fastopen_if_supported_ &&
IsTCPFastOpenSupported())
The "use_tcp_fastopen_if_supported_" check isn't needed, since this just set a
bool.

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:424: use_tcp_fastopen_if_supported_ = true;
This variable should be renamed to use_tcp_fastopen_ (It's already behind an
IsSupported check)

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:29: extern bool
g_tcp_fastopen_supported;
These values don't exist on windows.  Also, think it makes more sense just to
add accessors (And put them in an anonymous namespace in the other file), rather
than using externs.

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:76: auto_connect_with_write_ =
AUTO_CONNECT_DISABLED;
This isn't needed - we already have a check that it's supported before enabling
it.

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
These should probably be a member of TransportSocketParams.

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
I find these names very confusing - I have no idea what "AUTO_CONNECT" means,
and knowing it means Fast Open still leaves me scratching my head.

Maybe just:

TcpFastOpenMode {
  [TCP_]FAST_OPEN_USE_DEFAULT,
  [TCP_]FAST_OPEN_USE_IF_SUPPORTED,
  [TCP_]FAST_OPEN_DISABLED,
};

[mmenke, 2014-09-05 19:39:18]

On 2014/09/04 20:53:22, Jana wrote:
> Matt: Per our offline conversation, I've refactored the TFO enabling logic to
be
> all in one place (TransportSocketParams constructor). PTAL.
> 
> On the question of Happy Eyeballs: At the moment, this Happy Eyeballs will not
> work as designed, because v6 connect will return immediately as success. This
> leads to a matrix of possible cases -- v4/v6 with and without TFO -- and
ideally
> you'd want to explore all possibilities. I think the simplest fix for now may
be
> to not use TCP Fast Open when we are racing v4 and v6, and revisit this
question
> separately. WDYT?

Oh, and disabling fastopen when we have both v4 and v6 IPs sounds fine to me.

[Jana, 2014-09-05 20:38:57]

Thanks, Matt. I've addressed your comments, PTAL. Later today, I'll verify that
we don't do TFO for v4+v6 hosts.

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:182: ssl_params = new
SSLSocketParams(proxy_tcp_params,
On 2014/09/05 19:36:41, mmenke wrote:
> -2 indent

Done.

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:191: proxy_tcp_params = NULL;
On 2014/09/05 19:36:41, mmenke wrote:
> -2 indent

Done.

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:230: // for SSL sockets.
On 2014/09/05 19:36:41, mmenke wrote:
> Make sure to update your CL description to reflect this.

Done.

https://codereview.chromium.org/451383002/diff/180001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:298: new
TransportSocketParams(origin_host_port,
On 2014/09/05 19:36:41, mmenke wrote:
> +2 indent.

Done.

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:83: static const base::FilePath::CharType
kTCPFastOpenProcFilePath[] =
On 2014/09/05 19:36:42, mmenke wrote:
> I seem to recall reading on Chromium dev that static in this case may result
in
> worse code on some platforms.

Hmm. I don't know why this is static in the first place -- the variable is
scoped within the function. Removing static.

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:98: void CallbackForTCPFastOpenCheck(bool
user_enabled,
On 2014/09/05 19:36:42, mmenke wrote:
> Functions should generally be named after what they do, not what calls them.
> 
> Suggest calling this SetTCPFastOpenConfig (Options?  Settings?  Or just
> SetupTCPFastOpen?  Open to other ideas).

Good suggestion -- done. (RegisterTCPFastOpenIntentAndSupport)

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:119:
base::WorkerPool::GetTaskRunner(/*task_is_slow=*/false),
On 2014/09/05 19:36:42, mmenke wrote:
> File IO isn't considered slow?

(This was the code prior to me touching it... don't know why it was so.) This
touches the filesystem (/proc), but not disk -- that should be faster than
standard file I/O. Does that still make it slow?

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:423: if (!use_tcp_fastopen_if_supported_ &&
IsTCPFastOpenSupported())
On 2014/09/05 19:36:42, mmenke wrote:
> The "use_tcp_fastopen_if_supported_" check isn't needed, since this just set a
> bool.

Good catch -- Done.

https://codereview.chromium.org/451383002/diff/180001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:424: use_tcp_fastopen_if_supported_ = true;
On 2014/09/05 19:36:42, mmenke wrote:
> This variable should be renamed to use_tcp_fastopen_ (It's already behind an
> IsSupported check)

Good suggestion -- done.

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
On 2014/09/05 19:36:42, mmenke wrote:
> I find these names very confusing - I have no idea what "AUTO_CONNECT" means,
> and knowing it means Fast Open still leaves me scratching my head.
> 
> Maybe just:
> 
> TcpFastOpenMode {
>   [TCP_]FAST_OPEN_USE_DEFAULT,
>   [TCP_]FAST_OPEN_USE_IF_SUPPORTED,
>   [TCP_]FAST_OPEN_DISABLED,
> };

As discussed offline, I am happy to change these if they lead to better
intuition. I tried to use a more generic name, as Randy suggested earlier.

https://codereview.chromium.org/451383002/diff/180001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
On 2014/09/05 19:36:42, mmenke wrote:
> These should probably be a member of TransportSocketParams.

SG. I'll do this when we decide what they should be called as well.

[Jana, 2014-09-05 23:50:09]

I've now added additional checks in TransportConnectJob::DoTransportConnect that
do not enable TFO if Happy Eyeballs is about to be used.

[mmenke, 2014-09-08 18:30:53]

On 2014/09/05 23:50:09, Jana wrote:
> I've now added additional checks in TransportConnectJob::DoTransportConnect
that
> do not enable TFO if Happy Eyeballs is about to be used.

Sorry, forgot about this one this morning.  I'll get to it tomorrow.

[Randy Smith (Not in Mondays), 2014-09-08 19:36:32]

On 2014/09/04 20:53:22, Jana wrote:
> Matt: Per our offline conversation, I've refactored the TFO enabling logic to
be
> all in one place (TransportSocketParams constructor). PTAL.
> 
> On the question of Happy Eyeballs: At the moment, this Happy Eyeballs will not
> work as designed, because v6 connect will return immediately as success. This
> leads to a matrix of possible cases -- v4/v6 with and without TFO -- and
ideally
> you'd want to explore all possibilities. I think the simplest fix for now may
be
> to not use TCP Fast Open when we are racing v4 and v6, and revisit this
question
> separately. WDYT?
> 
> (It was a name conceived by Andrew Yourtchenko, I believe, and he was trying
to
> ameliorate the latency of failovers from v6 to v4 when using dual stacks...
> lower latency == happier users == happier eyeballs. Like most catchy names,
it's
> not shooting for accuracy or descriptiveness. It's a transferred epithet,
which
> is kinda nice... if only because I get to say "transferred epithet" in a
> Chromium CL :-))

Scanning my email, there was a point at which the consensus was just to nuke TFO
for happy eyeballs until first success on IPv6, and then later we simplified it
down to nuking TFO for any cases in which we're doing the happy eyeballs race. 
So I think this is ok; I also have a memory of someone objecting to nuking TFO
for any happy eyeballs race, but I can't find that in my email archives.  

In case you feel like going to more extended way: The conversation was with Will
and Ryan, primarily, and the consensus we reached at that point was that, since
IPv6 problems are usually near the client, we should avoid TFO until we've
gotten some IPv6 success, and then enable it (which means we'll always use IPv6
if there's a happy eyeballs race).  The NCN was tagged as the place to store the
"Has IPv6 worked since the last network change?" boolean.  I can forward you the
email thread if you like.

[Randy Smith (Not in Mondays), 2014-09-08 19:54:53]

LGTM (though I make no claims about the socket pool stuff--that's Matt's baby).

[mmenke, 2014-09-09 14:29:28]

I'm not still not happy with the "autoconnect".  I'll talk to randy about that
when he's around.

https://codereview.chromium.org/451383002/diff/220001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/451383002/diff/220001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:848: always_enable_if_supported = true;
optional nit:  Could just merge these into one statement:

bool always_enable_if_supported =
    command_line.HasSwitch(switches::kEnableTcpFastOpen);

Could event get rid of the local, but I think the name makes it clearer.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:77: else
Use braces for both cases of an if/else statement when any one of the blocks
takes up more than one line.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:80: auto_connect_with_write_ =
AUTO_CONNECT_ENABLED;
Use braces for an if when the antecedent takes up more than one line.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:282:
AddressListOnlyContainsIPv6(helper_.addresses())))
optional:  Think this is a little clearer if the last line is indented 4 spaces
instead of 1.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:283:
transport_socket_->EnableTCPFastOpen();
use braces.

[mmenke, 2014-09-09 16:03:33]

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:282:
AddressListOnlyContainsIPv6(helper_.addresses())))
On 2014/09/09 14:29:28, mmenke wrote:
> optional:  Think this is a little clearer if the last line is indented 4
spaces
> instead of 1.

optional:  Or may be clearer as a function in an anonymous namespace: 
AddressListContainsIPv6AndIPv4 or something.  Up to you.  It's not a complicated
check, but think removing it reduces complexity when reading the code.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:282:
AddressListOnlyContainsIPv6(helper_.addresses())))
Should we have any tests for this?  Not sure how hard they'd be to manage...Just
use mock sockets, and check that fast open was/wasn't set?

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:26: // and if a write on the socket is
guaranteed to happen before any reads.
Think the NOTE isn't really clear.  How about:

This translates into using TCP Fast Open.  Fast open should not be used in the
case the first write to the socket may be non-idempotent, as it could result in
sending the data twice on failure.  NOTE:  Currently, ENABLED is used if the
request is idempotent, and USE_DEFAULT is used for mother other cases (Including
non-idempotent requests).

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
So I talked to Randy - he remains a bit concerned, since these are set on
"TransportSocketParams" rather than something like "TCPSocketParams".  Though
that's effectively what they are, the naming abstracts that out...kinda.  He
said he'd defer to me, though.

My thinking is that AUTO_CONNECT is confusing.  Calling this
FIRST_WRITE_IS_IDEMPOTENT would make sense...except that then having a "DEFAULT"
value makes little sense.

Maybe call them:

COMBINE_CONNECT_AND_WRITE_ALLOWED,
COMBINE_CONNECT_AND_WRITE_IF_SUPPORTED,
COMBINE_CONNECT_AND_WRITE_PROHIBITED,

I think that's much more accurate, though still doesn't really clear that
idempotence matters (Not that calling them TCP_FAST_OPEN_* really makes that
clear, either).

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:48: const OnHostResolutionCallback&
host_resolution_callback);
Can you get rid of this?  Looks like you've updated most callsites in net/?  If
not, that's fine, but should eventually aim to get rid of it.

[Adam Rice, 2014-09-10 05:39:27]

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
On 2014/09/09 16:03:33, mmenke wrote:
> So I talked to Randy - he remains a bit concerned, since these are set on
> "TransportSocketParams" rather than something like "TCPSocketParams".  Though
> that's effectively what they are, the naming abstracts that out...kinda.  He
> said he'd defer to me, though.

At the moment "TransportSocket" always means TCP/IP in net/. Has this ever not
been the case?

[mmenke, 2014-09-10 13:53:55]

On 2014/09/10 05:39:27, Adam Rice wrote:
>
https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
> File net/socket/transport_client_socket_pool.h (right):
> 
>
https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
> net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
> On 2014/09/09 16:03:33, mmenke wrote:
> > So I talked to Randy - he remains a bit concerned, since these are set on
> > "TransportSocketParams" rather than something like "TCPSocketParams". 
Though
> > that's effectively what they are, the naming abstracts that out...kinda.  He
> > said he'd defer to me, though.
> 
> At the moment "TransportSocket" always means TCP/IP in net/. Has this ever not
> been the case?

Way back when (Before I worked here, which would be 4+ years ago), we had one
socket pool for SSL and HTTP sockets, I believe.  Can't remember if it was
called TransportSocketPool, or just SocketPool, though.

[Jana, 2014-09-10 16:03:11]

Comments addressed inline, PTAL. I've moved the enum to be inside
TransportSocketParams and changed the enum values to COMBINE_CONNECT_AND_WRITE*
with some slight mods to the names. Modulo the question about a test, I believe
I've addressed all comments.

https://codereview.chromium.org/451383002/diff/220001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/451383002/diff/220001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:848: always_enable_if_supported = true;
On 2014/09/09 14:29:28, mmenke wrote:
> optional nit:  Could just merge these into one statement:
> 
> bool always_enable_if_supported =
>     command_line.HasSwitch(switches::kEnableTcpFastOpen);
> 
> Could event get rid of the local, but I think the name makes it clearer.

I had just done exactly this nit fixing, so done :-)

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:77: else
On 2014/09/09 14:29:28, mmenke wrote:
> Use braces for both cases of an if/else statement when any one of the blocks
> takes up more than one line.

Done.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:80: auto_connect_with_write_ =
AUTO_CONNECT_ENABLED;
On 2014/09/09 14:29:28, mmenke wrote:
> Use braces for an if when the antecedent takes up more than one line.

Done.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:282:
AddressListOnlyContainsIPv6(helper_.addresses())))
On 2014/09/09 14:29:28, mmenke wrote:
> optional:  Think this is a little clearer if the last line is indented 4
spaces
> instead of 1.

Acknowledged.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:282:
AddressListOnlyContainsIPv6(helper_.addresses())))
On 2014/09/09 16:03:33, mmenke wrote:
> Should we have any tests for this?  Not sure how hard they'd be to
manage...Just
> use mock sockets, and check that fast open was/wasn't set?

Do you have a pointer for a test that currently tests happy eyeballs? One
difficulty is that currently setting fast open on a socket requires the system
to support it (this is the /proc file read for checking if the system supports
fast open.)

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:283:
transport_socket_->EnableTCPFastOpen();
On 2014/09/09 14:29:28, mmenke wrote:
> use braces.

This code has changed, but what's the formatting logic for using braces here?
There is no body that is > 1 line. Is this because the previous expression (the
boolean expression) is > 1 line?

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:26: // and if a write on the socket is
guaranteed to happen before any reads.
On 2014/09/09 16:03:33, mmenke wrote:
> Think the NOTE isn't really clear.  How about:
> 
> This translates into using TCP Fast Open.  Fast open should not be used in the
> case the first write to the socket may be non-idempotent, as it could result
in
> sending the data twice on failure.  NOTE:  Currently, ENABLED is used if the
> request is idempotent, and USE_DEFAULT is used for mother other cases
(Including
> non-idempotent requests).

I've slightly modified it from your suggestion, PTAL.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:30: AUTO_CONNECT_DISABLED
On 2014/09/09 16:03:33, mmenke wrote:
> So I talked to Randy - he remains a bit concerned, since these are set on
> "TransportSocketParams" rather than something like "TCPSocketParams".  Though
> that's effectively what they are, the naming abstracts that out...kinda.  He
> said he'd defer to me, though.
> 
> My thinking is that AUTO_CONNECT is confusing.  Calling this
> FIRST_WRITE_IS_IDEMPOTENT would make sense...except that then having a
"DEFAULT"
> value makes little sense.
> 
> Maybe call them:
> 
> COMBINE_CONNECT_AND_WRITE_ALLOWED,
> COMBINE_CONNECT_AND_WRITE_IF_SUPPORTED,
> COMBINE_CONNECT_AND_WRITE_PROHIBITED,
> 
> I think that's much more accurate, though still doesn't really clear that
> idempotence matters (Not that calling them TCP_FAST_OPEN_* really makes that
> clear, either).

I'm fine with using this... I still think of it as auto-connect, since that is
basically what TFO does from a client API point of view. I am using DEFAULT and
DESIRED instead of ALLOWED and IF_SUPPORTED, since it's really not clear how
ALLOWED and IF_SUPPORTED are different (IF_SUPPORTED seems like a subset of
ALLOWED). I've also moved it to be inside TransportSocketParams. PTAL. 

I understand the concern, and I understand the desire for clarity ... perhaps we
*should* change this to TCPSocketParams? That would make things a lot simpler.
That would be a different CL though.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:48: const OnHostResolutionCallback&
host_resolution_callback);
On 2014/09/09 16:03:33, mmenke wrote:
> Can you get rid of this?  Looks like you've updated most callsites in net/? 
If
> not, that's fine, but should eventually aim to get rid of it.

Ah -- oversight. Done. I had already combined the constructors in the .cc file,
forgot to remove it here.

[mmenke, 2014-09-10 17:32:23]

Note my two responses in the last patch set (Both in
net/socket/transport_client_socket_pool.cc).  Others are all in the new one.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:282:
AddressListOnlyContainsIPv6(helper_.addresses())))
On 2014/09/10 16:03:11, Jana wrote:
> On 2014/09/09 16:03:33, mmenke wrote:
> > Should we have any tests for this?  Not sure how hard they'd be to
> manage...Just
> > use mock sockets, and check that fast open was/wasn't set?
> 
> Do you have a pointer for a test that currently tests happy eyeballs? One
> difficulty is that currently setting fast open on a socket requires the system
> to support it (this is the /proc file read for checking if the system supports
> fast open.) 

Yea, we'd need to force the tests to think they had (Or did not have) happy
eyeballs support.

TransportClientSocketPoolTest.IPv6FallbackSocketIPv4FinishesFirst and the nearby
tests check happy eyeballs behavior.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:283:
transport_socket_->EnableTCPFastOpen();
On 2014/09/10 16:03:11, Jana wrote:
> On 2014/09/09 14:29:28, mmenke wrote:
> > use braces.
> 
> This code has changed, but what's the formatting logic for using braces here?
> There is no body that is > 1 line. Is this because the previous expression
(the
> boolean expression) is > 1 line?

It's because the antecedent of the if statement takes up more than one line -
basically, if anything takes up more than one line, use braces.  They're also
often, though not always, used on else/ifs, even if everything is a single line.

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:279: bool
try_ipv6_connect_with_ipv4_fallback =
optional:  Maybe add a comment?  Something like "If the list contains IPv6 and
IPv4 addresses, the first address will be IPv6, and the IPv4 addresses will be
tried as fallback addresses, per "Happy Eyeballs."

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:289:
transport_socket_->EnableTCPFastOpen();
nit:  Use braces when the conditional takes more than one line.

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:36: // NOTE: Currently,
COMBINE_CONNECT_AND_WRITE_DESIRED is used if we know the
nit:  My fault, but shouldn't use "we" in comments.  (Maybe "... is used if it
is known..."?)

[Jana, 2014-09-11 16:02:36]

Tests added in transport_client_socket_pool_unittest.cc (thanks for your offline
help, Matt; sometimes I need a second pair of eyes to see stupid.) PTAL. I've
addressed the other nits as well. I believe this patchset addresses all
outstanding comments and issues.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:282:
AddressListOnlyContainsIPv6(helper_.addresses())))
On 2014/09/10 17:32:22, mmenke wrote:
> On 2014/09/10 16:03:11, Jana wrote:
> > On 2014/09/09 16:03:33, mmenke wrote:
> > > Should we have any tests for this?  Not sure how hard they'd be to
> > manage...Just
> > > use mock sockets, and check that fast open was/wasn't set?
> > 
> > Do you have a pointer for a test that currently tests happy eyeballs? One
> > difficulty is that currently setting fast open on a socket requires the
system
> > to support it (this is the /proc file read for checking if the system
supports
> > fast open.) 
> 
> Yea, we'd need to force the tests to think they had (Or did not have) happy
> eyeballs support.
> 
> TransportClientSocketPoolTest.IPv6FallbackSocketIPv4FinishesFirst and the
nearby
> tests check happy eyeballs behavior.

Tests added in transport_client_socket_pool_unittest.cc, PTAL.

https://codereview.chromium.org/451383002/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:283:
transport_socket_->EnableTCPFastOpen();
On 2014/09/10 17:32:22, mmenke wrote:
> On 2014/09/10 16:03:11, Jana wrote:
> > On 2014/09/09 14:29:28, mmenke wrote:
> > > use braces.
> > 
> > This code has changed, but what's the formatting logic for using braces
here?
> > There is no body that is > 1 line. Is this because the previous expression
> (the
> > boolean expression) is > 1 line?
> 
> It's because the antecedent of the if statement takes up more than one line -
> basically, if anything takes up more than one line, use braces.  They're also
> often, though not always, used on else/ifs, even if everything is a single
line.

Acknowledged.

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:279: bool
try_ipv6_connect_with_ipv4_fallback =
On 2014/09/10 17:32:23, mmenke wrote:
> optional:  Maybe add a comment?  Something like "If the list contains IPv6 and
> IPv4 addresses, the first address will be IPv6, and the IPv4 addresses will be
> tried as fallback addresses, per "Happy Eyeballs."

Done. Also added an RFC number to make it look official :-)

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:289:
transport_socket_->EnableTCPFastOpen();
On 2014/09/10 17:32:22, mmenke wrote:
> nit:  Use braces when the conditional takes more than one line.

Done.

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/451383002/diff/240001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:36: // NOTE: Currently,
COMBINE_CONNECT_AND_WRITE_DESIRED is used if we know the
On 2014/09/10 17:32:23, mmenke wrote:
> nit:  My fault, but shouldn't use "we" in comments.  (Maybe "... is used if it
> is known..."?)

Good catch. Changed to "if the data in the write is known to be ..."

[mmenke, 2014-09-11 18:14:35]

Think this looks good.

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_unittest.cc:1000:
TransportSocketParamsPeer::SetCombineConnectAndWritePolicy(
Rather than creating a peer for this purpose, suggest either just creating a new
set of params inline, copying the other values from params_.

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_unittest.cc:1038:
TEST_F(TransportClientSocketPoolTest, NoTCPFastOpenOnIPv6WithIPv4Fallback) {
Suggest a test just like this, except the IPv6 socket succeeds, to make sure
that one isn't using fast open, either.

Not sure if you'll need to set up a stalled IPv4 socket or not - I'd guess not,
but you should double chek that.

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_unittest.cc:1067:
EXPECT_FALSE(handle.socket()->UsingTCPFastOpen());
As a sanity check, suggest you check that the connected socket is using an IPv4
IP.  We have tests for that above, but I'm paranoid.

[mmenke, 2014-09-11 18:20:37]

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_unittest.cc:1000:
TransportSocketParamsPeer::SetCombineConnectAndWritePolicy(
On 2014/09/11 18:14:35, mmenke wrote:
> Rather than creating a peer for this purpose, suggest either just creating a
new
> set of params inline, copying the other values from params_.

Oops...left out my "or".  Or making a function to create params, that takes in a
combine connect and write setting.

[Jana, 2014-09-12 01:04:40]

Thanks, Matt. I've addressed your comments, PTAL. I've modified/added the tests.
As we discussed offline, I've  also added accessor methods to both
tcp_socket_libevent.cc and tcp_socket_win.cc (with prototypes in tcp_socket.h).

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_unittest.cc:1000:
TransportSocketParamsPeer::SetCombineConnectAndWritePolicy(
On 2014/09/11 18:20:37, mmenke wrote:
> On 2014/09/11 18:14:35, mmenke wrote:
> > Rather than creating a peer for this purpose, suggest either just creating a
> new
> > set of params inline, copying the other values from params_.
> 
> Oops...left out my "or".  Or making a function to create params, that takes in
a
> combine connect and write setting.

Done.

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_unittest.cc:1038:
TEST_F(TransportClientSocketPoolTest, NoTCPFastOpenOnIPv6WithIPv4Fallback) {
On 2014/09/11 18:14:35, mmenke wrote:
> Suggest a test just like this, except the IPv6 socket succeeds, to make sure
> that one isn't using fast open, either.
> 
> Not sure if you'll need to set up a stalled IPv4 socket or not - I'd guess
not,
> but you should double chek that.

Added test, PTAL.

https://codereview.chromium.org/451383002/diff/280001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_unittest.cc:1067:
EXPECT_FALSE(handle.socket()->UsingTCPFastOpen());
On 2014/09/11 18:14:35, mmenke wrote:
> As a sanity check, suggest you check that the connected socket is using an
IPv4
> IP.  We have tests for that above, but I'm paranoid.

In general, I like to not duplicate test coverage. In this case, since we need
to ensure that the cases of IPv6 succeeding vs. failing are presuppositions for
the rest of the test, adding this check has a slightly different value than for
the tests above, so adding it.

[Jana, 2014-09-12 01:04:43]

Thanks, Matt. I've addressed your comments, PTAL. I've modified/added the tests.
As we discussed offline, I've  also added accessor methods to both
tcp_socket_libevent.cc and tcp_socket_win.cc (with prototypes in tcp_socket.h).

[mmenke, 2014-09-12 14:20:19]

LGTM, just a couple nits.

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket.h
File net/socket/tcp_socket.h (right):

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket.h...
net/socket/tcp_socket.h:17: namespace net {
nit:  We generally have a blank line after the start of namespaces.  (Exceptions
are for forward declarations, and sometimes when nesting namespaces)

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:35: namespace {
nit:  +linebreak.

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_w...
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.cc:27: namespace {
nit:  add back linebreak.

[Jana, 2014-09-12 15:25:52]

Thanks, Matt, I've addressed your nits. PTAL.

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket.h
File net/socket/tcp_socket.h (right):

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket.h...
net/socket/tcp_socket.h:17: namespace net {
On 2014/09/12 14:20:18, mmenke wrote:
> nit:  We generally have a blank line after the start of namespaces. 
(Exceptions
> are for forward declarations, and sometimes when nesting namespaces)

Done.

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:35: namespace {
On 2014/09/12 14:20:19, mmenke wrote:
> nit:  +linebreak.

Done.

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_w...
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.cc:27: namespace {
On 2014/09/12 14:20:19, mmenke wrote:
> nit:  add back linebreak.

Done.

[mmenke, 2014-09-12 15:29:44]

Still LGTM

On 2014/09/12 15:25:52, Jana wrote:
> Thanks, Matt, I've addressed your nits. PTAL.
> 
> https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket.h
> File net/socket/tcp_socket.h (right):
> 
>
https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket.h...
> net/socket/tcp_socket.h:17: namespace net {
> On 2014/09/12 14:20:18, mmenke wrote:
> > nit:  We generally have a blank line after the start of namespaces. 
> (Exceptions
> > are for forward declarations, and sometimes when nesting namespaces)
> 
> Done.
> 
>
https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_l...
> File net/socket/tcp_socket_libevent.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_l...
> net/socket/tcp_socket_libevent.cc:35: namespace {
> On 2014/09/12 14:20:19, mmenke wrote:
> > nit:  +linebreak.
> 
> Done.
> 
>
https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_w...
> File net/socket/tcp_socket_win.cc (right):
> 
>
https://codereview.chromium.org/451383002/diff/300001/net/socket/tcp_socket_w...
> net/socket/tcp_socket_win.cc:27: namespace {
> On 2014/09/12 14:20:19, mmenke wrote:
> > nit:  add back linebreak.
> 
> Done.

[Jana, 2014-09-12 15:33:59]

Sorry, missed that you actually "LGTM"d it (still having my coffee this
morning.) Thanks much! Submitting.

[Jana, 2014-09-12 16:24:50]

Fixed a small compile issue after syncing. Submitting.

[Jana, 2014-09-12 16:25:03]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2014-09-12 16:25:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/451383002/340001

[commit-bot: I haz the power, 2014-09-12 16:33:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 16:33:43]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/55298)

[Jana, 2014-09-12 18:15:39]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2014-09-12 18:19:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/451383002/360001

[Jana, 2014-09-12 18:40:30]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2014-09-12 18:49:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/451383002/380001

[commit-bot: I haz the power, 2014-09-12 20:34:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 20:34:38]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jana, 2014-09-12 22:38:35]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:41:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/451383002/400001

[commit-bot: I haz the power, 2014-09-12 23:54:18]

Committed patchset #21 (id:400001) as be12aaeb80f415169f4cee0737d0d20e2b49044a

[commit-bot: I haz the power, 2014-09-13 00:02:33]

Patchset 21 (id:??) landed as
https://crrev.com/dcb4ae922b46a90ef6ec199b772f399e4610f514
Cr-Commit-Position: refs/heads/master@{#294697}

[Jana, 2014-09-13 00:29:44]

I'd be remiss if I didn't thank both Matt and Randy for being great reviewers,
and for helping make this CL substantially better than it was. Thank you!