Add a feature to bound the time spent on TCP connect attempts.

By default, there is no timeout on individual TCP connect attempts (handshakes) other than that enforced by the host OS's network stack.

This change introduces the "TimeoutTcpConnectAttempt" feature, which when enabled will cause slow attempts to fail sooner with ERR_TIMED_OUT.

The time permitted for an individual attempt is a function of three feature parameters, and the estimated network speed:

  clamp(Param("TimeoutTcpConnectAttemptMin"),
        Param("TimeoutTcpConnectAttemptMax"),
        estimated_rtt * Param("kTimeoutTcpConnectAttemptRTTMultiplier"))

In other words, the timeout will be between "TimeoutTcpConnectAttemptMin" and "TimeoutTcpConnectAttemptMax", depending on how fast we think the network is.

Bug: 1123197
Change-Id: Ibfcf8b26b5b424bf9d12f5604294d7a99950f10f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436349
Commit-Queue: Eric Roman <eroman@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812782}

net/base/features.cc

@@ -177,5 +177,23 @@ extern const base::FeatureParam<int> kLimitOpenUDPSocketsMax(
     "LimitOpenUDPSocketsMax",
     6000);
 
+const base::Feature kTimeoutTcpConnectAttempt{
+    "TimeoutTcpConnectAttempt", base::FEATURE_DISABLED_BY_DEFAULT};
+
+extern const base::FeatureParam<double> kTimeoutTcpConnectAttemptRTTMultiplier(
+    &kTimeoutTcpConnectAttempt,
+    "TimeoutTcpConnectAttemptRTTMultiplier",
+    5.0);
+
+extern const base::FeatureParam<base::TimeDelta> kTimeoutTcpConnectAttemptMin(
+    &kTimeoutTcpConnectAttempt,
+    "TimeoutTcpConnectAttemptMin",
+    base::TimeDelta::FromSeconds(8));
+
+extern const base::FeatureParam<base::TimeDelta> kTimeoutTcpConnectAttemptMax(
+    &kTimeoutTcpConnectAttempt,
+    "TimeoutTcpConnectAttemptMax",
+    base::TimeDelta::FromSeconds(30));
+
 }  // namespace features
 }  // namespace net

net/base/features.h

@@ -266,6 +266,28 @@ NET_EXPORT extern const base::Feature kLimitOpenUDPSockets;
 // this will result in a failure (ERR_INSUFFICIENT_RESOURCES).
 NET_EXPORT extern const base::FeatureParam<int> kLimitOpenUDPSocketsMax;
 
+// Enables a timeout on individual TCP connect attempts, based on
+// the parameter values.
+NET_EXPORT extern const base::Feature kTimeoutTcpConnectAttempt;
+
+// FeatureParams associated with kTimeoutTcpConnectAttempt.
+
+// When there is an estimated RTT available, the experimental TCP connect
+// attempt timeout is calculated as:
+//
+//  clamp(kTimeoutTcpConnectAttemptMin,
+//        kTimeoutTcpConnectAttemptMax,
+//        <Estimated RTT> * kTimeoutTcpConnectAttemptRTTMultiplier);
+//
+// Otherwise the TCP connect attempt timeout is set to
+// kTimeoutTcpConnectAttemptMax.
+NET_EXPORT extern const base::FeatureParam<double>
+    kTimeoutTcpConnectAttemptRTTMultiplier;
+NET_EXPORT extern const base::FeatureParam<base::TimeDelta>
+    kTimeoutTcpConnectAttemptMin;
+NET_EXPORT extern const base::FeatureParam<base::TimeDelta>
+    kTimeoutTcpConnectAttemptMax;
+
 }  // namespace features
 }  // namespace net
 

net/socket/tcp_client_socket.cc

@@ -13,6 +13,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/notreached.h"
 #include "base/time/time.h"
+#include "net/base/features.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
@@ -259,6 +260,17 @@ int TCPClientSocket::DoConnect() {
     socket_->socket_performance_watcher()->OnConnectionChanged();
 
   start_connect_attempt_ = base::TimeTicks::Now();
+
+  // Start a timer to fail the connect attempt if it takes too long.
+  base::TimeDelta attempt_timeout = GetConnectAttemptTimeout();
+  if (!attempt_timeout.is_max()) {
+    DCHECK(!connect_attempt_timer_.IsRunning());
+    connect_attempt_timer_.Start(
+        FROM_HERE, attempt_timeout,
+        base::BindOnce(&TCPClientSocket::OnConnectAttemptTimeout,
+                       base::Unretained(this)));
+  }
+
   return ConnectInternal(endpoint);
 }
 
@@ -266,6 +278,7 @@ int TCPClientSocket::DoConnectComplete(int result) {
   if (start_connect_attempt_) {
     EmitConnectAttemptHistograms(result);
     start_connect_attempt_ = base::nullopt;
+    connect_attempt_timer_.Stop();
   }
 
   if (result == OK)
@@ -292,6 +305,10 @@ int TCPClientSocket::DoConnectComplete(int result) {
   return result;
 }
 
+void TCPClientSocket::OnConnectAttemptTimeout() {
+  DidCompleteConnect(ERR_TIMED_OUT);
+}
+
 int TCPClientSocket::ConnectInternal(const IPEndPoint& endpoint) {
   // |socket_| is owned by this class and the callback won't be run once
   // |socket_| is gone. Therefore, it is safe to use base::Unretained() here.
@@ -318,6 +335,7 @@ void TCPClientSocket::DoDisconnect() {
   if (start_connect_attempt_) {
     EmitConnectAttemptHistograms(ERR_ABORTED);
     start_connect_attempt_ = base::nullopt;
+    connect_attempt_timer_.Stop();
   }
 
   total_received_bytes_ = 0;
@@ -602,4 +620,31 @@ void TCPClientSocket::EmitConnectAttemptHistograms(int result) {
   }
 }
 
+base::TimeDelta TCPClientSocket::GetConnectAttemptTimeout() {
+  if (!base::FeatureList::IsEnabled(features::kTimeoutTcpConnectAttempt))
+    return base::TimeDelta::Max();
+
+  base::Optional<base::TimeDelta> transport_rtt = base::nullopt;
+  if (network_quality_estimator_)
+    transport_rtt = network_quality_estimator_->GetTransportRTT();
+
+  base::TimeDelta min_timeout = features::kTimeoutTcpConnectAttemptMin.Get();
+  base::TimeDelta max_timeout = features::kTimeoutTcpConnectAttemptMax.Get();
+
+  if (!transport_rtt)
+    return max_timeout;
+
+  base::TimeDelta adaptive_timeout =
+      transport_rtt.value() *
+      features::kTimeoutTcpConnectAttemptRTTMultiplier.Get();
+
+  if (adaptive_timeout <= min_timeout)
+    return min_timeout;
+
+  if (adaptive_timeout >= max_timeout)
+    return max_timeout;
+
+  return adaptive_timeout;
+}
+
 }  // namespace net

net/socket/tcp_client_socket.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/power_monitor/power_observer.h"
+#include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "net/base/address_list.h"
 #include "net/base/completion_once_callback.h"
@@ -153,6 +154,8 @@ class NET_EXPORT TCPClientSocket : public TransportClientSocket,
   int DoConnect();
   int DoConnectComplete(int result);
 
+  void OnConnectAttemptTimeout();
+
   // Calls the connect method of |socket_|. Used in tests, to ensure a socket
   // never connects.
   virtual int ConnectInternal(const IPEndPoint& endpoint);
@@ -176,6 +179,12 @@ class NET_EXPORT TCPClientSocket : public TransportClientSocket,
   // |result|.
   void EmitConnectAttemptHistograms(int result);
 
+  // Gets the timeout to use for the next TCP connect attempt. This is an
+  // experimentally controlled value based on the estimated transport round
+  // trip time. If no timeout is to be enforced, returns
+  // base::TimeDelta::Max().
+  base::TimeDelta GetConnectAttemptTimeout();
+
   std::unique_ptr<TCPSocket> socket_;
 
   // Local IP address and port we are bound to. Set to NULL if Bind()
@@ -222,6 +231,8 @@ class NET_EXPORT TCPClientSocket : public TransportClientSocket,
   // Can be nullptr.
   NetworkQualityEstimator* network_quality_estimator_;
 
+  base::OneShotTimer connect_attempt_timer_;
+
   base::WeakPtrFactory<TCPClientSocket> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(TCPClientSocket);