Collect all ConnectionAttempts from both sockets in TransportConnectJob.

Collect all ConnectionAttempts from both sockets in TransportConnectJob.

Before, the TransportConnectJob simply inferred that, if the main socket
failed to connect, the address it was using was the last address in the
list. With this change, the TCPClientSocket actually tracks all of the
connection attempts made (as it tries each address in the list), and the
TransportConnectJob copies the attempts from both the main and fallback
sockets and records all of them in the ClientSocketHandle in
GetAdditionalErrorState.

BUG=480565
TBR=jam

Committed: https://crrev.com/23fdb7b4ed86ee9f63b16a3b432fa2c721432bd2
Cr-Commit-Position: refs/heads/master@{#330012}

[Deprecated (see juliatuttle), 2015-04-24 02:34:33]

ttuttle@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Deprecated (see juliatuttle), 2015-04-24 02:34:33]

PTAL when you get a chance; note dependency in CL description.

[Deprecated (see juliatuttle), 2015-04-27 22:24:23]

Patchset #2 (id:20001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:24:31]

Patchset #2 (id:40001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:25:23]

Patchset #1 (id:1) has been deleted

[Randy Smith (Not in Mondays), 2015-05-01 17:39:50]

High level comments:
* Bug #?
* Remove dependency information from CL description.
* I'm confused by the line in the CL description that says "Tracks *all*
Connection Attempts in TCPClientSocket ...".  What's the change in behavior from
a naive observers viewpoint?  As an example of that naivete' (though I'm asking
for general doc, not just answering this specific question): AIUI, The old code
recorded a single error from the socket and put that into the
TransportConnectJob, and that was returned up the stack.  I get that you're now
returning information from both main and fallback sockets, but I don't get what
the difference in the kind of information you're returning from the socket->TCJ
is.

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

https://codereview.chromium.org/1096203006/diff/80001/net/socket/stream_socke...
net/socket/stream_socket.h:9: #include "net/socket/connection_attempts.h"
Replace with forward decl.

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (left):

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:248:
ConnectionAttempt(helper_.addresses().back(), connect_result_));
Doesn't this mean we can nuke connect_result_ from the member variable list?

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

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:249:
fallback_connection_attempts_.end());
Is ordering relevant for the ConnectionAttempts vector?  If so, probably worth a
comment in the header file as to what it means, and possibly here as to why you
chose this ordering.  If not, might be worthwhile making it a Set rather than a
Vector, or including a comment in the header file if you want to keep it a
vector, just so that consumers don't rely on the ordering in some weird way.

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:420: void
TransportConnectJob::DoIPv6FallbackTransportConnectComplete(int result) {
Just because I'm a paranoid person: What prevents both connections from
completing and hence the results of GetAdditionalErrorState() being a bit random
depending on whether it's called before or after the losing connection
completion callback?  The code in the callbacks seems to be written assuming
that only one of them will be called (the race winning histogram entries) but
I'm not seeing what guarantees that.

[Deprecated (see juliatuttle), 2015-05-04 17:24:37]

Patchset #1 (id:60001) has been deleted

[Deprecated (see juliatuttle), 2015-05-04 19:44:50]

PTAL.

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

https://codereview.chromium.org/1096203006/diff/80001/net/socket/stream_socke...
net/socket/stream_socket.h:9: #include "net/socket/connection_attempts.h"
On 2015/05/01 17:39:49, rdsmith wrote:
> Replace with forward decl.

Can't, it's a typedef.

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (left):

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:248:
ConnectionAttempt(helper_.addresses().back(), connect_result_));
On 2015/05/01 17:39:50, rdsmith wrote:
> Doesn't this mean we can nuke connect_result_ from the member variable list?

Why? We're still using it in the new version here.

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

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:249:
fallback_connection_attempts_.end());
On 2015/05/01 17:39:49, rdsmith wrote:
> Is ordering relevant for the ConnectionAttempts vector?  If so, probably worth
a
> comment in the header file as to what it means, and possibly here as to why
you
> chose this ordering.  If not, might be worthwhile making it a Set rather than
a
> Vector, or including a comment in the header file if you want to keep it a
> vector, just so that consumers don't rely on the ordering in some weird way.

Ordering is approximately "the order the connections were made", except main and
fallback aren't interleaved.

What I should really be doing is recording start and end times for each attempt,
so Domain Reliability can use those specifically.

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:420: void
TransportConnectJob::DoIPv6FallbackTransportConnectComplete(int result) {
On 2015/05/01 17:39:50, rdsmith wrote:
> Just because I'm a paranoid person: What prevents both connections from
> completing and hence the results of GetAdditionalErrorState() being a bit
random
> depending on whether it's called before or after the losing connection
> completion callback?  The code in the callbacks seems to be written assuming
> that only one of them will be called (the race winning histogram entries) but
> I'm not seeing what guarantees that.

Nothing guarantees it, but it's the best I can do without rearchitecting when
GetAdditionalErrorState is called.

[Randy Smith (Not in Mondays), 2015-05-06 15:30:12]

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.cc (left):

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:248:
ConnectionAttempt(helper_.addresses().back(), connect_result_));
On 2015/05/04 19:44:50, ttuttle wrote:
> On 2015/05/01 17:39:50, rdsmith wrote:
> > Doesn't this mean we can nuke connect_result_ from the member variable list?
> 
> Why? We're still using it in the new version here.

Ah, sorry, missed that.

I do wonder a bit about whether or not it's the right choice to not save errors
when we eventually get to success, but that's a high level question that doesn't
seem that important.

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

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:249:
fallback_connection_attempts_.end());
On 2015/05/04 19:44:50, ttuttle wrote:
> On 2015/05/01 17:39:49, rdsmith wrote:
> > Is ordering relevant for the ConnectionAttempts vector?  If so, probably
worth
> a
> > comment in the header file as to what it means, and possibly here as to why
> you
> > chose this ordering.  If not, might be worthwhile making it a Set rather
than
> a
> > Vector, or including a comment in the header file if you want to keep it a
> > vector, just so that consumers don't rely on the ordering in some weird way.
> 
> Ordering is approximately "the order the connections were made", except main
and
> fallback aren't interleaved.
> 
> What I should really be doing is recording start and end times for each
attempt,
> so Domain Reliability can use those specifically.

I still feel like this means we should make it clear at the definition location
that there's no ordering guarantees (or what the ordering guarantees are),
either with a comment or hitting it over the head by using a set<>.  If you
later add start/end times that won't change the lack of ordering guarantees.

https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.cc:420: void
TransportConnectJob::DoIPv6FallbackTransportConnectComplete(int result) {
On 2015/05/04 19:44:50, ttuttle wrote:
> On 2015/05/01 17:39:50, rdsmith wrote:
> > Just because I'm a paranoid person: What prevents both connections from
> > completing and hence the results of GetAdditionalErrorState() being a bit
> random
> > depending on whether it's called before or after the losing connection
> > completion callback?  The code in the callbacks seems to be written assuming
> > that only one of them will be called (the race winning histogram entries)
but
> > I'm not seeing what guarantees that.
> 
> Nothing guarantees it, but it's the best I can do without rearchitecting when
> GetAdditionalErrorState is called.

Huh.  Ok, two follow on question:
* It seems like a bug to me that both connections can complete, i.e. that we
don't abort one connection when the other one completes.  That seems like the
purpose of a race.  If there is a bug, I'd like to surface it.  Maybe run this
by Matt?  (Sorry, being lazy/efficient here in asking you to deal rather than
groking it on my own because I'm barely at code review level of focus.)

* Does that level of uncertainty satisfy the high level domain reliability
goals?  (Or, to put it differently, can you explain to me what the domain
reliability goals are and why that level of uncertainty satisfies them?  It
feels wonky to me.)

[Deprecated (see juliatuttle), 2015-05-08 23:31:52]

Okay, here's a new version:

1. It makes sure all of the attempts make it out of the TransportConnectJob --
if it returns successfully by providing a socket, it stuffs any attempts made on
the other socket into the one it's returning, and if it returns failure, it
saves the attempts made on both sockets to copy into the ClientSocketHandle in
GetAdditionalErrorState.

2. It clears the attempts on a REUSED_IDLE socket when returning it from a
socket pool, since the "get a connected socket" operation didn't actually fail,
and we don't want to report the failed connection attempt once each time we
reuse the socket!

Things that are still missing:

1. Unit tests.

2. An explanation of ordering.

PTAL at the changes, and I'll keep working on those parts.

[Randy Smith (Not in Mondays), 2015-05-12 20:11:09]

I'm afraid you're getting a lot of comments in proportion to the amount of
effort it took me to figure out the issues I was concerned about :-}.  Many of
these are interrelated, and just about all of them are about
comments/documentation.  Feel free to argue or do something different than what
I suggest; I mostly just want it to be clear to a hypothetical future reader
what's going on without too much pain on their part.

As I mentioned, I'd like to do some thinking about the consistency of your
entire stack, specifically:
* That in just about all cases you're recording only failed connection attempts,
and the doc and/or routine names note that.
* I'd like all methods that have the same name have the same semantics around
whether they replace or append both source and destination of the connection
attempts.  

I'm happy to have that be a separate CL.

https://codereview.chromium.org/1096203006/diff/220001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/1096203006/diff/220001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.h:283: void MaybeCopyConnectionAttempts();
I think it would be useful to have a comment somewhere describing the intent of
this function, given that it's used throughout the code.  I'm neutral about
whether that somewhere is here or at the point of implementation, given that
it's private.

https://codereview.chromium.org/1096203006/diff/220001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/1096203006/diff/220001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:496:
idle_socket.socket->ClearConnectionAttempts();
I have a bit of a twitch here, which I'll give voice to and let you do what you
want.  From a pure truth&beauty API perspective, the re-use of the socket isn't
really something that the socket itself should be thinking about.  It's
connected, it's used for a while, it's destroyed, that's all she wrote.  I'm
solidly aware that that abstraction layer is already being violated, but I'll
call out that were I in an ideal world, I'd want to keep it clean, and maybe
export an operator to clear the connection attempts, or maybe just making
retrieving the connection attempts clear them so that this behavior
automatically happens.

Actually, ok, real question, why isn't that what happens?  Why do we need to
clear connection attempts; won't they always be cleared by retrieval by the
first consumer?

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
net/socket/stream_socket.h:100: virtual void ClearConnectionAttempts();
I'm confused.  My sense (please correct me if I'm wrong) is that in most places
in this set of CLs you've had GetConnectionAttepts() clear the underlying
storage when it's called.  Is there a reason for the inconsistency here?  If I'm
right about the common idiom, I'm tempted to ask for a different name here just
to signal a different behavior.

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
net/socket/stream_socket.h:101: virtual void AddConnectionAttempts(const
ConnectionAttempts& attempts);
So there's a philosophy issue here.  I know that, at least in some contexts,
Sleevi's pretty adamant that interface classes shouldn't have default
implementations, in order to force each subclass to engage with the question of
how they want to deal with that aspect of the interface (as opposed to allowing
out of sight, out of mind, and making changes to interfaces potentially not be
propagated where they need to be).  I have more mixed feelings on the topic, as
it can create a lot of work for subclasses.  I'm still trying to figure out
whether, or how much, I disagree with Ryan, but while I'm figuring that out I'm
sorta in favor of sticking with the current best practice, which is =0 in ABCs. 
That's also supported by consistency--I think the rest of this class is
predominantly/all =0.  

I don't think pushing that here would create a lot of work for you; would you be
willing to do that?

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
net/socket/stream_socket.h:101: virtual void AddConnectionAttempts(const
ConnectionAttempts& attempts);
Also note my earlier comment (different CL?) about really wanting interface
class methods to be documented with comments in the interface class (relevant to
my other comment here as well).

(ETA) I'll specifically note that I'd like a specification of whether the
socket-internal set of connection attempts is nulled out on this call, and
whether the argument is assigned to or appended to.  I.e. what happens if you
call GetConnectionAttempts() twice with the same arguments?

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

https://codereview.chromium.org/1096203006/diff/220001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:334: // attempts made on the fallback
socket.
Acknowledgement of ugly hack in comment?  :-} :-J.  (Up to you--I'm just sorta
wanting a "Since, unfortunately, the only way to return information in the
success case is through the returned socket." or something to ease my guilt
about leaving this interface alone :-} :-J.)

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

https://codereview.chromium.org/1096203006/diff/220001/net/socket/transport_c...
net/socket/transport_client_socket_pool.h:200: void
CopyConnectionAttemptsFromClientSocketHandles();
Why a fully descriptive name here and a shortening of the name in the "Maybe"
case elsewhere in this CL?

https://codereview.chromium.org/1096203006/diff/240001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1096203006/diff/240001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1515: void
HttpStreamFactoryImpl::Job::MaybeCopyConnectionAttempts() {
Is there any way the state machine would allow this to be called twice?  If so,
wouldn't you get duplicate connection attempts on the destination socket?

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
net/socket/tcp_client_socket.h:124: ConnectionAttempts connection_attempts_;
Note that this is used to store failed connection attempts (changing name is an
acceptable way to do that).

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

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:244: } else if (connect_result_ !=
OK) {
Given our conversation as to logging philosophy at this level (which I'm
interpreting as "log all failures and make them available, whatever the result
was"), why is this only done if connect_result_ != OK?  As I understand it, this
routine shouldn't be called if connect_result != OK, but shouldn't that be a
DCHECK/NOTREACHED rather than a test?

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:334: // attempts made on the fallback
socket.
I'd like to just acknowledge that we're grabbing information in the middle of a
race, so would you be willing to add "... made *to this point* on ..."?

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:391:
CopyConnectionAttemptsFromClientSocketHandles();
So if the main connection fails, but the fallback socket connection completes
later (which I think is allowed by the current state machine?), this will work
but you'll do a gratuitous copy here.  But if the main connection fails, and the
fallback connection later fails, won't this result in two calls to
CopyConnectionAttemptsFromClientSocketHandles()?  Is that ok?  (I think it is,
but a) I want to make sure you agree, and b) I think it's ok because of the
precise API spec for the GetConnectionAttempts() interface, so I *really* want
to nail down that definition.)

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:448: // connection attempts made on
the main socket.
" ... made to this point on ..."

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

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.h:217: // Saved when connections fail
and passed up in GetAdditionalErrorState.
Ok, so I want to seriously bulk up this comment, and possibly include some of
the information here in the CL description.  What I think is happening is that:
* As a socket is used for connection attempts, connection failures accumulate on
that socket.
* When the TransportConnectJob hits a connection success case, it copies all of
the connection attempt information from the socket that isn't being returned to
the socket that is being returned.
* When the TransportConnectJob gives up in failure, it pulls all the information
out of the sockets and puts it in these variables, where it's available to
GetAdditionalErrorState().

Now that I've written that out, I'd like it to be that level of clear in the CL
description, and I'm ok with just adding a comment here "Only used in failure
case; on success the failed connection attempt information is stored on the
socket to be returned."

That ok?

[Deprecated (see juliatuttle), 2015-05-13 18:22:29]

https://codereview.chromium.org/1096203006/diff/220001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/1096203006/diff/220001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.h:283: void MaybeCopyConnectionAttempts();
On 2015/05/12 20:11:09, rdsmith wrote:
> I think it would be useful to have a comment somewhere describing the intent
of
> this function, given that it's used throughout the code.  I'm neutral about
> whether that somewhere is here or at the point of implementation, given that
> it's private.

Done.

https://codereview.chromium.org/1096203006/diff/220001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/1096203006/diff/220001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:496:
idle_socket.socket->ClearConnectionAttempts();
On 2015/05/12 20:11:09, rdsmith wrote:
> I have a bit of a twitch here, which I'll give voice to and let you do what
you
> want.  From a pure truth&beauty API perspective, the re-use of the socket
isn't
> really something that the socket itself should be thinking about.  It's
> connected, it's used for a while, it's destroyed, that's all she wrote.  I'm
> solidly aware that that abstraction layer is already being violated, but I'll
> call out that were I in an ideal world, I'd want to keep it clean, and maybe
> export an operator to clear the connection attempts, or maybe just making
> retrieving the connection attempts clear them so that this behavior
> automatically happens.

The socket isn't concerned about it, the socket pool is. The socket is,
unfortunately, also a conduit for the connection attempts, so it needs to have
the Add and Clear methods to allow the ConnectJob and SocketPoolBase,
respectively, to ensure the new owner of the socket sees the right set of
attempts.

> Actually, ok, real question, why isn't that what happens?  Why do we need to
> clear connection attempts; won't they always be cleared by retrieval by the
> first consumer?

GetConnectionAttempts does not clear them.

If you think it would be prettier, I could make it so that the
ClientSocketHandle clears the attempts on the socket after copying them.

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
net/socket/stream_socket.h:100: virtual void ClearConnectionAttempts();
On 2015/05/12 20:11:09, rdsmith wrote:
> I'm confused.  My sense (please correct me if I'm wrong) is that in most
places
> in this set of CLs you've had GetConnectionAttepts() clear the underlying
> storage when it's called.  Is there a reason for the inconsistency here?  If
I'm
> right about the common idiom, I'm tempted to ask for a different name here
just
> to signal a different behavior.  

I believe you're wrong; can you point me to an instance of where I clear them in
Get? (Note that Get is const.)

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
net/socket/stream_socket.h:101: virtual void AddConnectionAttempts(const
ConnectionAttempts& attempts);
On 2015/05/12 20:11:09, rdsmith wrote:
> Also note my earlier comment (different CL?) about really wanting interface
> class methods to be documented with comments in the interface class (relevant
to
> my other comment here as well).
> 
> (ETA) I'll specifically note that I'd like a specification of whether the
> socket-internal set of connection attempts is nulled out on this call, and
> whether the argument is assigned to or appended to.  I.e. what happens if you
> call GetConnectionAttempts() twice with the same arguments?

Done.

https://codereview.chromium.org/1096203006/diff/220001/net/socket/stream_sock...
net/socket/stream_socket.h:101: virtual void AddConnectionAttempts(const
ConnectionAttempts& attempts);
On 2015/05/12 20:11:09, rdsmith wrote:
> So there's a philosophy issue here.  I know that, at least in some contexts,
> Sleevi's pretty adamant that interface classes shouldn't have default
> implementations, in order to force each subclass to engage with the question
of
> how they want to deal with that aspect of the interface (as opposed to
allowing
> out of sight, out of mind, and making changes to interfaces potentially not be
> propagated where they need to be).  I have more mixed feelings on the topic,
as
> it can create a lot of work for subclasses.  I'm still trying to figure out
> whether, or how much, I disagree with Ryan, but while I'm figuring that out
I'm
> sorta in favor of sticking with the current best practice, which is =0 in
ABCs. 
> That's also supported by consistency--I think the rest of this class is
> predominantly/all =0.  
> 
> I don't think pushing that here would create a lot of work for you; would you
be
> willing to do that?

Will do. Sending comments in the meantime so you can see them while I go through
the subclasses.

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

https://codereview.chromium.org/1096203006/diff/220001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:334: // attempts made on the fallback
socket.
On 2015/05/12 20:11:09, rdsmith wrote:
> Acknowledgement of ugly hack in comment?  :-} :-J.  (Up to you--I'm just sorta
> wanting a "Since, unfortunately, the only way to return information in the
> success case is through the returned socket." or something to ease my guilt
> about leaving this interface alone :-} :-J.)

Done.

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

https://codereview.chromium.org/1096203006/diff/220001/net/socket/transport_c...
net/socket/transport_client_socket_pool.h:200: void
CopyConnectionAttemptsFromClientSocketHandles();
On 2015/05/12 20:11:09, rdsmith wrote:
> Why a fully descriptive name here and a shortening of the name in the "Maybe"
> case elsewhere in this CL?

I think I've managed to un-shorten the other without running afoul of line
length limits.

https://codereview.chromium.org/1096203006/diff/240001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1096203006/diff/240001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1515: void
HttpStreamFactoryImpl::Job::MaybeCopyConnectionAttempts() {
On 2015/05/12 20:11:09, rdsmith wrote:
> Is there any way the state machine would allow this to be called twice?  If
so,
> wouldn't you get duplicate connection attempts on the destination socket?

It can be called twice if we do RestartWithAuth, but that's okay -- we reset the
stream when we restart with auth, so the second copy will only copy attempts
from the new stream.

It *looks* like the other error cases are non-restartable, but it's not really
clear. If they go through Start, that's fine, as we'll create a new stream.

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
net/socket/tcp_client_socket.h:124: ConnectionAttempts connection_attempts_;
On 2015/05/12 20:11:09, rdsmith wrote:
> Note that this is used to store failed connection attempts (changing name is
an
> acceptable way to do that).

I will do this as part of the mass rename later, if that's okay?

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

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:244: } else if (connect_result_ !=
OK) {
On 2015/05/12 20:11:09, rdsmith wrote:
> Given our conversation as to logging philosophy at this level (which I'm
> interpreting as "log all failures and make them available, whatever the result
> was"), why is this only done if connect_result_ != OK?  As I understand it,
this
> routine shouldn't be called if connect_result != OK, but shouldn't that be a
> DCHECK/NOTREACHED rather than a test?

Done.

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:334: // attempts made on the fallback
socket.
On 2015/05/12 20:11:09, rdsmith wrote:
> I'd like to just acknowledge that we're grabbing information in the middle of
a
> race, so would you be willing to add "... made *to this point* on ..."?

Done.

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:391:
CopyConnectionAttemptsFromClientSocketHandles();
On 2015/05/12 20:11:09, rdsmith wrote:
> So if the main connection fails, but the fallback socket connection completes
> later (which I think is allowed by the current state machine?), this will work
> but you'll do a gratuitous copy here.  But if the main connection fails, and
the
> fallback connection later fails, won't this result in two calls to
> CopyConnectionAttemptsFromCThlientSocketHandles()?  Is that ok?  (I think it
is,
> but a) I want to make sure you agree, and b) I think it's ok because of the
> precise API spec for the GetConnectionAttempts() interface, so I *really* want
> to nail down that definition.)

If the main connection fails, this returns the failing result at the bottom of
the function, and that goes to the callback, which (supposedly) deletes the job,
so the job won't see the fallback socket fail too.

That said, if that somehow happens, it's fine; GetConnectionAttempts *replaces*
the input parameter with the connection attempts made under the purview of the
object it is called on, but does not modify the internal state of that object.

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:448: // connection attempts made on
the main socket.
On 2015/05/12 20:11:09, rdsmith wrote:
> " ... made to this point on ..."

Done.

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

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.h:217: // Saved when connections fail
and passed up in GetAdditionalErrorState.
On 2015/05/12 20:11:09, rdsmith wrote:
> Ok, so I want to seriously bulk up this comment, and possibly include some of
> the information here in the CL description.  What I think is happening is
that:
> * As a socket is used for connection attempts, connection failures accumulate
on
> that socket.
> * When the TransportConnectJob hits a connection success case, it copies all
of
> the connection attempt information from the socket that isn't being returned
to
> the socket that is being returned.
> * When the TransportConnectJob gives up in failure, it pulls all the
information
> out of the sockets and puts it in these variables, where it's available to
> GetAdditionalErrorState().
> 
> Now that I've written that out, I'd like it to be that level of clear in the
CL
> description, and I'm ok with just adding a comment here "Only used in failure
> case; on success the failed connection attempt information is stored on the
> socket to be returned."
> 
> That ok?

Done.

[Randy Smith (Not in Mondays), 2015-05-14 19:42:05]

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
net/socket/tcp_client_socket.h:124: ConnectionAttempts connection_attempts_;
On 2015/05/13 18:22:29, ttuttle wrote:
> On 2015/05/12 20:11:09, rdsmith wrote:
> > Note that this is used to store failed connection attempts (changing name is
> an
> > acceptable way to do that).
> 
> I will do this as part of the mass rename later, if that's okay?

Just stick a comment in right now, and then you can do what you want on the
rename.

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

https://codereview.chromium.org/1096203006/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:391:
CopyConnectionAttemptsFromClientSocketHandles();
On 2015/05/13 18:22:29, ttuttle wrote:
> On 2015/05/12 20:11:09, rdsmith wrote:
> > So if the main connection fails, but the fallback socket connection
completes
> > later (which I think is allowed by the current state machine?), this will
work
> > but you'll do a gratuitous copy here.  But if the main connection fails, and
> the
> > fallback connection later fails, won't this result in two calls to
> > CopyConnectionAttemptsFromCThlientSocketHandles()?  Is that ok?  (I think it
> is,
> > but a) I want to make sure you agree, and b) I think it's ok because of the
> > precise API spec for the GetConnectionAttempts() interface, so I *really*
want
> > to nail down that definition.)
> 
> If the main connection fails, this returns the failing result at the bottom of
> the function, and that goes to the callback, which (supposedly) deletes the
job,
> so the job won't see the fallback socket fail too.

If we're racing two connection attempts, why would we stop if one of them fails?
 Isn't the point of doing two to allow either of them to fail?

> 
> That said, if that somehow happens, it's fine; GetConnectionAttempts
*replaces*
> the input parameter with the connection attempts made under the purview of the
> object it is called on, but does not modify the internal state of that object.

Ok, not blocking for this CL, then.

https://codereview.chromium.org/1096203006/diff/260001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1096203006/diff/260001/net/socket/stream_sock...
net/socket/stream_socket.h:105: // for the first owner.)
I'd remove the parenthesis; it's about the consumer of the interface, which is
really none of the StreamSocket's business.  I'd probably say "Clears the
socket's list of connection attempts." but that's a nit&suggestion.

https://codereview.chromium.org/1096203006/diff/260001/net/socket/stream_sock...
net/socket/stream_socket.h:110: // the fallback [or main, if this was the
fallback] socket.)
Same comment about parenthetical, and similar but stronger comment about the
non-parenthetical; make it clear that the connection attempts being added to are
the internal set kept by the socket that will be returned by
GetConnectionAttempts().

https://codereview.chromium.org/1096203006/diff/380001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1096203006/diff/380001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1539: if (connection_->socket()) {
Is there a way to write a unit test to test this code?

https://codereview.chromium.org/1096203006/diff/380001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1096203006/diff/380001/net/socket/client_sock...
net/socket/client_socket_handle.h:161: //
|StreamSocket::GetConnectionAttempts|.)
Is this relevant for this level of API?  From my naive perspective,
ClientSocketHandle doesn't know about connection attempt failure/success; it's
just a storage location.)

https://codereview.chromium.org/1096203006/diff/380001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1096203006/diff/380001/net/socket/stream_sock...
net/socket/stream_socket.h:100: // connecting this socket. They come in no
particular order.
I'd rather have the ordering comment next to the ConnectionAttempts typedef; it
applies to the type, right?

https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_c...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:480: void
TransportConnectJob::CopyConnectionAttemptsFromClientSocketHandles() {
I think this name is misleading, since we have a ClientSocketHandle class, and
transport_socket_ and fallback_transport_socket_ are not of that class.

[Deprecated (see juliatuttle), 2015-05-14 20:02:35]

PTAL.

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_...
net/socket/tcp_client_socket.h:124: ConnectionAttempts connection_attempts_;
On 2015/05/14 19:42:04, rdsmith wrote:
> On 2015/05/13 18:22:29, ttuttle wrote:
> > On 2015/05/12 20:11:09, rdsmith wrote:
> > > Note that this is used to store failed connection attempts (changing name
is
> > an
> > > acceptable way to do that).
> > 
> > I will do this as part of the mass rename later, if that's okay?
> 
> Just stick a comment in right now, and then you can do what you want on the
> rename.

Done.

https://codereview.chromium.org/1096203006/diff/260001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1096203006/diff/260001/net/socket/stream_sock...
net/socket/stream_socket.h:105: // for the first owner.)
On 2015/05/14 19:42:05, rdsmith wrote:
> I'd remove the parenthesis; it's about the consumer of the interface, which is
> really none of the StreamSocket's business.  I'd probably say "Clears the
> socket's list of connection attempts." but that's a nit&suggestion.

Done.

https://codereview.chromium.org/1096203006/diff/260001/net/socket/stream_sock...
net/socket/stream_socket.h:110: // the fallback [or main, if this was the
fallback] socket.)
On 2015/05/14 19:42:05, rdsmith wrote:
> Same comment about parenthetical, and similar but stronger comment about the
> non-parenthetical; make it clear that the connection attempts being added to
are
> the internal set kept by the socket that will be returned by
> GetConnectionAttempts().

Done.

https://codereview.chromium.org/1096203006/diff/380001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1096203006/diff/380001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1539: if (connection_->socket()) {
On 2015/05/14 19:42:05, rdsmith wrote:
> Is there a way to write a unit test to test this code?

I think it is implicitly tested by the tests at the URLRequest layer -- the
fallback ones cover the success case, and the basic failure ones cover the
failure case.

https://codereview.chromium.org/1096203006/diff/380001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1096203006/diff/380001/net/socket/client_sock...
net/socket/client_socket_handle.h:161: //
|StreamSocket::GetConnectionAttempts|.)
On 2015/05/14 19:42:05, rdsmith wrote:
> Is this relevant for this level of API?  From my naive perspective,
> ClientSocketHandle doesn't know about connection attempt failure/success; it's
> just a storage location.)

It's a storage location that is fairly intimately tied with what a socket pool
needs to return on success or failure, I think?

https://codereview.chromium.org/1096203006/diff/380001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1096203006/diff/380001/net/socket/stream_sock...
net/socket/stream_socket.h:100: // connecting this socket. They come in no
particular order.
On 2015/05/14 19:42:05, rdsmith wrote:
> I'd rather have the ordering comment next to the ConnectionAttempts typedef;
it
> applies to the type, right?

Done.

https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_c...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:480: void
TransportConnectJob::CopyConnectionAttemptsFromClientSocketHandles() {
On 2015/05/14 19:42:05, rdsmith wrote:
> I think this name is misleading, since we have a ClientSocketHandle class, and
> transport_socket_ and fallback_transport_socket_ are not of that class.

Gosh, you're right. That was a think-o when I wrote it.

[Randy Smith (Not in Mondays), 2015-05-14 20:29:55]

I can't believe I/we ate the whole thing.  LGTM.

https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_c...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:480: void
TransportConnectJob::CopyConnectionAttemptsFromClientSocketHandles() {
On 2015/05/14 20:02:35, ttuttle wrote:
> On 2015/05/14 19:42:05, rdsmith wrote:
> > I think this name is misleading, since we have a ClientSocketHandle class,
and
> > transport_socket_ and fallback_transport_socket_ are not of that class.
> 
> Gosh, you're right. That was a think-o when I wrote it.

I zoned over it several times myself.  "ClientSocketHandle?  Sounds legit." 
:-}.

[Deprecated (see juliatuttle), 2015-05-14 20:34:00]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-05-14 20:34:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1096203006/#ps440001
(title: "Fix StreamSocket destructor")

[commit-bot: I haz the power, 2015-05-14 20:37:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1096203006/440001

[commit-bot: I haz the power, 2015-05-14 20:44:55]

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

[commit-bot: I haz the power, 2015-05-14 20:44:56]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Deprecated (see juliatuttle), 2015-05-14 20:46:18]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-05-14 20:46:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1096203006/#ps420002
(title: "No, really")

[commit-bot: I haz the power, 2015-05-14 20:47:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1096203006/420002

[commit-bot: I haz the power, 2015-05-14 21:32:49]

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

[commit-bot: I haz the power, 2015-05-14 21:32:50]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Deprecated (see juliatuttle), 2015-05-15 00:26:43]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-05-15 00:26:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1096203006/#ps470001
(title: "Return fake ConnectionAttempt in MockTCPClientSocket")

[commit-bot: I haz the power, 2015-05-15 00:28:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1096203006/470001

[commit-bot: I haz the power, 2015-05-15 01:28:12]

Committed patchset #21 (id:470001)

[commit-bot: I haz the power, 2015-05-15 01:29:09]

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

[Randy Smith (Not in Mondays), 2015-05-18 15:12:38]

PS21 LGTM.