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 219 matching lines @@
       return LOAD_STATE_IDLE;
   }
   NOTREACHED();
   return LOAD_STATE_IDLE;
 }
 
 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) {

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

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?

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

Done.

-    DCHECK_LT(0u, helper_.addresses().size());
-    attempts.push_back(
-        ConnectionAttempt(helper_.addresses().back(), connect_result_));
+    attempts.insert(attempts.begin(), connection_attempts_.begin(),
+                    connection_attempts_.end());
+    attempts.insert(attempts.begin(), fallback_connection_attempts_.begin(),
+                    fallback_connection_attempts_.end());
   }
   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 64 matching lines @@
         base::TimeDelta::FromMilliseconds(
             TransportConnectJobHelper::kIPv6FallbackTimerInMs),
         this,
         &TransportConnectJob::DoIPv6FallbackTransportConnect);
   }
   return rv;
 }
 
 int TransportConnectJob::DoTransportConnectComplete(int result) {
   if (result == OK) {
+    // Success will be returned via the main socket, so also include connection
+    // attempts made on the fallback socket.

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

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 ..."?

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

Done.

+    if (fallback_transport_socket_) {
+      ConnectionAttempts fallback_attempts;
+      fallback_transport_socket_->GetConnectionAttempts(&fallback_attempts);
+      transport_socket_->AddConnectionAttempts(fallback_attempts);
+    }
+
     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;
       } else {
@@ skipping 28 matching lines @@
             100);
         break;
       default:
         NOTREACHED();
         break;
     }
 
     SetSocket(transport_socket_.Pass());
     fallback_timer_.Stop();
   } else {
+    // Failure will be returned via |GetAdditionalErrorState|, so save
+    // connection attempts from both sockets for use there.
+    CopyConnectionAttemptsFromClientSocketHandles();

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

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

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

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.

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

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?
Ok, not blocking for this CL, then.

+
     // Be a bit paranoid and kill off the fallback members to prevent reuse.
     fallback_transport_socket_.reset();
     fallback_addresses_.reset();
   }
 
   connect_result_ = result;
 
+  // N.B.: The owner of the ConnectJob will delete it after the callback is
+  // called, so the fallback socket, if any, won't stick around for long.
+
   return result;
 }
 
 void TransportConnectJob::DoIPv6FallbackTransportConnect() {
   // The timer should only fire while we're waiting for the main connect to
   // succeed.
   if (helper_.next_state() !=
       TransportConnectJobHelper::STATE_TRANSPORT_CONNECT_COMPLETE) {
     NOTREACHED();
     return;
@@ skipping 23 matching lines @@
     NOTREACHED();
     return;
   }
 
   DCHECK_NE(ERR_IO_PENDING, result);
   DCHECK(fallback_transport_socket_.get());
   DCHECK(fallback_addresses_.get());
 
   if (result == OK) {
     DCHECK(!fallback_connect_start_time_.is_null());
+
+    // Success will be returned via the fallback socket, so also include
+    // connection attempts made on the main socket.

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

" ... made to this point on ..."

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

Done.

+    if (transport_socket_) {
+      ConnectionAttempts attempts;
+      transport_socket_->GetConnectionAttempts(&attempts);
+      fallback_transport_socket_->AddConnectionAttempts(attempts);
+    }
+
     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 {
+    // Failure will be returned via |GetAdditionalErrorState|, so save
+    // connection attempts from both sockets for use there.
+    CopyConnectionAttemptsFromClientSocketHandles();
+
     // Be a bit paranoid and kill off the fallback members to prevent reuse.
     fallback_transport_socket_.reset();
     fallback_addresses_.reset();
   }
+
+  // N.B.: The owner of the ConnectJob will delete it after the callback is
+  // called, so the main socket, if any, won't stick around for long.
+
   NotifyDelegateOfCompletion(result);  // Deletes |this|
 }
 
 int TransportConnectJob::ConnectInternal() {
   return helper_.DoConnectInternal(this);
 }
 
+void TransportConnectJob::CopyConnectionAttemptsFromClientSocketHandles() {
+  if (transport_socket_)
+    transport_socket_->GetConnectionAttempts(&connection_attempts_);
+  if (fallback_transport_socket_) {
+    fallback_transport_socket_->GetConnectionAttempts(
+        &fallback_connection_attempts_);
+  }
+}
+
 scoped_ptr<ConnectJob>
 TransportClientSocketPool::TransportConnectJobFactory::NewConnectJob(
     const std::string& group_name,
     const PoolBase::Request& request,
     ConnectJob::Delegate* delegate) const {
   return scoped_ptr<ConnectJob>(
       new TransportConnectJob(group_name,
                               request.priority(),
                               request.params(),
                               ConnectionTimeout(),
@@ skipping 129 matching lines @@
     HigherLayeredPool* higher_pool) {
   base_.AddHigherLayeredPool(higher_pool);
 }
 
 void TransportClientSocketPool::RemoveHigherLayeredPool(
     HigherLayeredPool* higher_pool) {
   base_.RemoveHigherLayeredPool(higher_pool);
 }
 
 }  // namespace net