Collect all ConnectionAttempts from both sockets in TransportConnectJob.

net/socket/transport_client_socket_pool.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/transport_client_socket_pool.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 #include "base/lazy_instance.h"
@@ skipping 225 matching lines @@
 void TransportConnectJob::GetAdditionalErrorState(ClientSocketHandle* handle) {
   // If hostname resolution failed, record an empty endpoint and the result.
   // If the actual socket Connect call failed, record the result and the last
   // address attempted.
   // TODO(ttuttle): Plumb into the socket layer and record *all* attempts.
   ConnectionAttempts attempts;
   if (resolve_result_ != OK) {
     DCHECK_EQ(0u, helper_.addresses().size());
     attempts.push_back(ConnectionAttempt(IPEndPoint(), resolve_result_));
   } else if (connect_result_ != OK) {
-    DCHECK_LT(0u, helper_.addresses().size());
-    attempts.push_back(
-        ConnectionAttempt(helper_.addresses().back(), connect_result_));

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

Doesn't this mean we can nuke connect_result_ from the member variable list?

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

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

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

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.

+    attempts.insert(attempts.begin(), connection_attempts_.begin(),
+                    connection_attempts_.end());
+    attempts.insert(attempts.begin(), fallback_connection_attempts_.begin(),
+                    fallback_connection_attempts_.end());

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

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.

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

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.

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

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.

   }
   handle->set_connection_attempts(attempts);
 }
 
 // static
 void TransportConnectJob::MakeAddressListStartWithIPv4(AddressList* list) {
   for (AddressList::iterator i = list->begin(); i != list->end(); ++i) {
     if (i->GetFamily() == ADDRESS_FAMILY_IPV4) {
       std::rotate(list->begin(), i, list->end());
       break;
@@ skipping 63 matching lines @@
         FROM_HERE,
         base::TimeDelta::FromMilliseconds(
             TransportConnectJobHelper::kIPv6FallbackTimerInMs),
         this,
         &TransportConnectJob::DoIPv6FallbackTransportConnect);
   }
   return rv;
 }
 
 int TransportConnectJob::DoTransportConnectComplete(int result) {
+  transport_socket_->GetConnectionAttempts(&connection_attempts_);
+
   if (result == OK) {
     bool is_ipv4 =
         helper_.addresses().front().GetFamily() == ADDRESS_FAMILY_IPV4;
     TransportConnectJobHelper::ConnectionLatencyHistogram race_result =
         TransportConnectJobHelper::CONNECTION_LATENCY_UNKNOWN;
     if (is_ipv4) {
       race_result = TransportConnectJobHelper::CONNECTION_LATENCY_IPV4_NO_RACE;
     } else {
       if (AddressListOnlyContainsIPv6(helper_.addresses())) {
         race_result = TransportConnectJobHelper::CONNECTION_LATENCY_IPV6_SOLO;
@@ skipping 65 matching lines @@
           *fallback_addresses_, net_log().net_log(), net_log().source());
   fallback_connect_start_time_ = base::TimeTicks::Now();
   int rv = fallback_transport_socket_->Connect(
       base::Bind(
           &TransportConnectJob::DoIPv6FallbackTransportConnectComplete,
           base::Unretained(this)));
   if (rv != ERR_IO_PENDING)
     DoIPv6FallbackTransportConnectComplete(rv);
 }
 
 void TransportConnectJob::DoIPv6FallbackTransportConnectComplete(int result) {

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

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 19:44:50]

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:11]

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.)

   // This should only happen when we're waiting for the main connect to succeed.
   if (helper_.next_state() !=
       TransportConnectJobHelper::STATE_TRANSPORT_CONNECT_COMPLETE) {
     NOTREACHED();
     return;
   }
 
   DCHECK_NE(ERR_IO_PENDING, result);
   DCHECK(fallback_transport_socket_.get());
   DCHECK(fallback_addresses_.get());
 
+  fallback_transport_socket_->GetConnectionAttempts(
+      &fallback_connection_attempts_);
+
   if (result == OK) {
     DCHECK(!fallback_connect_start_time_.is_null());
     connect_timing_.connect_start = fallback_connect_start_time_;
     helper_.HistogramDuration(
         TransportConnectJobHelper::CONNECTION_LATENCY_IPV4_WINS_RACE);
     SetSocket(fallback_transport_socket_.Pass());
     helper_.set_next_state(TransportConnectJobHelper::STATE_NONE);
     transport_socket_.reset();
   } else {
     // Be a bit paranoid and kill off the fallback members to prevent reuse.
@@ skipping 149 matching lines @@
     HigherLayeredPool* higher_pool) {
   base_.AddHigherLayeredPool(higher_pool);
 }
 
 void TransportClientSocketPool::RemoveHigherLayeredPool(
     HigherLayeredPool* higher_pool) {
   base_.RemoveHigherLayeredPool(higher_pool);
 }
 
 }  // namespace net