Adds TCP FastOpen blackhole recovery code to avoid getting persistently blackholed.

net/socket/tcp_socket_libevent.cc

Index: net/socket/tcp_socket_libevent.cc
diff --git a/net/socket/tcp_socket_libevent.cc b/net/socket/tcp_socket_libevent.cc
index f39611b603b63df6774e035017a7bbb60aef0750..0d76f94281452c7989ba151842e73fb38e992d8e 100644
--- a/net/socket/tcp_socket_libevent.cc
+++ b/net/socket/tcp_socket_libevent.cc
@@ -39,6 +39,9 @@ bool g_tcp_fastopen_supported = false;
 // True if TCP FastOpen is user-enabled for all connections.
 // TODO(jri): Change global variable to param in HttpNetworkSession::Params.
 bool g_tcp_fastopen_user_enabled = false;
+// True if TCP FastOpen connect-with-write has failed at least once.
+bool g_tcp_fastopen_has_failed = false;
+
 
 // SetTCPNoDelay turns on/off buffering in the kernel. By default, TCP sockets
 // will wait up to 200ms for more data to complete a packet before transmitting.
@@ -129,8 +132,9 @@ void CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled) {
 TCPSocketLibevent::TCPSocketLibevent(NetLog* net_log,
                                      const NetLog::Source& source)
     : use_tcp_fastopen_(false),
+      tcp_fastopen_write_attempted_(false),
       tcp_fastopen_connected_(false),
-      fast_open_status_(FAST_OPEN_STATUS_UNKNOWN),
+      tcp_fastopen_status_(TCP_FASTOPEN_STATUS_UNKNOWN),
       logging_multiple_connect_attempts_(false),
       net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_SOCKET)) {
   net_log_.BeginEvent(NetLog::TYPE_SOCKET_ALIVE,
@@ -139,9 +143,9 @@ TCPSocketLibevent::TCPSocketLibevent(NetLog* net_log,
 
 TCPSocketLibevent::~TCPSocketLibevent() {
   net_log_.EndEvent(NetLog::TYPE_SOCKET_ALIVE);
-  if (tcp_fastopen_connected_) {
+  if (tcp_fastopen_write_attempted_) {
     UMA_HISTOGRAM_ENUMERATION("Net.TcpFastOpenSocketConnection",
-                              fast_open_status_, FAST_OPEN_MAX_VALUE);
+                              tcp_fastopen_status_, TCP_FASTOPEN_MAX_VALUE);
   }
 }
 
@@ -222,7 +226,7 @@ int TCPSocketLibevent::Connect(const IPEndPoint& address,
 
   if (use_tcp_fastopen_) {
     // With TCP FastOpen, we pretend that the socket is connected.
-    DCHECK(!tcp_fastopen_connected_);
+    DCHECK(!tcp_fastopen_write_attempted_);
     socket_->SetPeerAddress(storage);
     return OK;
   }
@@ -239,7 +243,7 @@ bool TCPSocketLibevent::IsConnected() const {
   if (!socket_)
     return false;
 
-  if (use_tcp_fastopen_ && !tcp_fastopen_connected_ &&
+  if (use_tcp_fastopen_ && !tcp_fastopen_write_attempted_ &&
       socket_->HasPeerAddress()) {
     // With TCP FastOpen, we pretend that the socket is connected.
     // This allows GetPeerAddress() to return peer_address_.
@@ -268,8 +272,6 @@ int TCPSocketLibevent::Read(IOBuffer* buf,
                  // use it when Read() completes, as otherwise, this transfers
                  // ownership of buf to socket.
                  base::Unretained(this), make_scoped_refptr(buf), callback));
-  if (rv >= 0)
-    RecordFastOpenStatus();
   if (rv != ERR_IO_PENDING)
     rv = HandleReadCompleted(buf, rv);
   return rv;
@@ -288,7 +290,9 @@ int TCPSocketLibevent::Write(IOBuffer* buf,
                  // ownership of buf to socket.
                  base::Unretained(this), make_scoped_refptr(buf), callback);
   int rv;
-  if (use_tcp_fastopen_ && !tcp_fastopen_connected_) {
+
+  if (use_tcp_fastopen_ && !tcp_fastopen_write_attempted_) {
+    DCHECK(!g_tcp_fastopen_has_failed);
     rv = TcpFastOpenWrite(buf, buf_len, write_callback);
   } else {
     rv = socket_->Write(buf, buf_len, write_callback);
@@ -414,8 +418,12 @@ bool TCPSocketLibevent::SetNoDelay(bool no_delay) {
 
 void TCPSocketLibevent::Close() {
   socket_.reset();
+
+  // Reset TCP FastOpen state.

[mmenke, 2014/09/24 14:46:11]

Should we have:

if (tcp_fastopen_write_attempted_) {
  UMA_HISTOGRAM_ENUMERATION("Net.TcpFastOpenSocketConnection"
                            fast_open_status_, FAST_OPEN_MAX_VALUE);
}

And then just call Close() in the destructor?

[Jana, 2014/09/25 16:10:16]

Good suggestion -- Done.

+  use_tcp_fastopen_ = false;
   tcp_fastopen_connected_ = false;
-  fast_open_status_ = FAST_OPEN_STATUS_UNKNOWN;
+  tcp_fastopen_write_attempted_ = false;
+  tcp_fastopen_status_ = TCP_FASTOPEN_STATUS_UNKNOWN;
 }
 
 bool TCPSocketLibevent::UsingTCPFastOpen() const {
@@ -423,7 +431,11 @@ bool TCPSocketLibevent::UsingTCPFastOpen() const {
 }
 
 void TCPSocketLibevent::EnableTCPFastOpenIfSupported() {
-  if (IsTCPFastOpenSupported())
+  // Do not enable TCP FastOpen if it had previously failed.
+  // This check conservatively avoids middleboxes that may blackhole
+  // TCP FastOpen SYN+Data packets; on such a failure, subsequent sockets
+  // should not use TCP FastOpen.
+  if (IsTCPFastOpenSupported() && !g_tcp_fastopen_has_failed)
     use_tcp_fastopen_ = true;
 }
 
@@ -553,14 +565,26 @@ void TCPSocketLibevent::ReadCompleted(const scoped_refptr<IOBuffer>& buf,
                                       const CompletionCallback& callback,
                                       int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
-  // Records TCP FastOpen status regardless of error in asynchronous case.
-  // TODO(rdsmith,jri): Change histogram name to indicate it could be called on
-  // error.
-  RecordFastOpenStatus();
   callback.Run(HandleReadCompleted(buf.get(), rv));
 }
 
 int TCPSocketLibevent::HandleReadCompleted(IOBuffer* buf, int rv) {
+  if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_ &&
+      !g_tcp_fastopen_has_failed) {

[mmenke, 2014/09/24 14:46:11]

The !g_tcp_fastopen_has_failed still seems kinda weird here.  Suppose this
happens:

* Try to connect socket A using Tcp Fast Open
* Try to connect socket B using Tcp Fast Open
* Socket A ends up here with a failure.  g_tcp_fastopen_has_failed is set to
false.
* Socket B ends up here (With a success or a failure):  We now have
tcp_fastopen_write_attempted_ is true, tcp_fastopen_connected_ is false,
g_tcp_fastopen_has_failed is true, and tcp_fastopen_status_ is
TCP_FASTOPEN_FAST_CONNECT_RETURN or TCP_FASTOPEN_SLOW_CONNECT_RETURN,
presumably.  We don't update tcp_fastopen_status_ for socket B because of the
failure, but we then record the histogram on close/destruction for socket
B...And the value we record isn't actually in any way comparable to the
histogram we record for socket A, since we don't call RecordTCPFastOpenStatus().

[Jana, 2014/09/25 16:10:16]

Hmm -- good point. I had the g_tcp_fastopen_has_failed check as a redundant one,
but as you point out in your example, it prevents a perfectly good histogram
event from being recorded correctly. Also in light of my new histogram
additions, it makes sense to simply allow all failures to go through and record
themselves. I'm removing the check for g_tcp_fastopen_has_failed here.

+    // A TCP FastOpen connect-with-write was attempted. This read was a
+    // subsequent read, which either succeeded or failed. If the read
+    // succeeded, the socket is considered connected via TCP FastOpen.
+    // If the read failed, TCP FastOpen is (conservatively) turned off for all
+    // subsequent connections. TCP FastOpen status is recorded in both cases.
+    // TODO(rdsmith,jri): Change histogram name to indicate it could be called
+    // on error.
+    if (rv >= 0)
+      tcp_fastopen_connected_ = true;
+    else
+      g_tcp_fastopen_has_failed = true;
+    RecordTCPFastOpenStatus();
+  }
+
   if (rv < 0) {
     net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
                       CreateNetLogSocketErrorCallback(rv, errno));
@@ -576,13 +600,20 @@ int TCPSocketLibevent::HandleReadCompleted(IOBuffer* buf, int rv) {
 
 void TCPSocketLibevent::WriteCompleted(const scoped_refptr<IOBuffer>& buf,
                                        const CompletionCallback& callback,
-                                       int rv) const {
+                                       int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   callback.Run(HandleWriteCompleted(buf.get(), rv));
 }
 
-int TCPSocketLibevent::HandleWriteCompleted(IOBuffer* buf, int rv) const {
+int TCPSocketLibevent::HandleWriteCompleted(IOBuffer* buf, int rv) {
   if (rv < 0) {
+    if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
+      // TCP FastOpen connect-with-write was attempted, and the write failed
+      // for unknown reasons. Record status and (conservatively) turn off
+      // TCP FastOpen for all subsequent connections.
+      tcp_fastopen_status_ = TCP_FASTOPEN_ERROR;
+      g_tcp_fastopen_has_failed = true;
+    }
     net_log_.AddEvent(NetLog::TYPE_SOCKET_WRITE_ERROR,
                       CreateNetLogSocketErrorCallback(rv, errno));
     return rv;
@@ -618,10 +649,10 @@ int TCPSocketLibevent::TcpFastOpenWrite(
                            flags,
                            storage.addr,
                            storage.addr_len));
-  tcp_fastopen_connected_ = true;
+  tcp_fastopen_write_attempted_ = true;
 
   if (rv >= 0) {
-    fast_open_status_ = FAST_OPEN_FAST_CONNECT_RETURN;
+    tcp_fastopen_status_ = TCP_FASTOPEN_FAST_CONNECT_RETURN;
     return rv;
   }
 
@@ -639,45 +670,63 @@ int TCPSocketLibevent::TcpFastOpenWrite(
   }
 
   if (rv != ERR_IO_PENDING) {
-    fast_open_status_ = FAST_OPEN_ERROR;
+    // TCP FastOpen connect-with-write was attempted, and the write failed for
+    // unknown reasons. Record status and (conservatively) turn off
+    // TCP FastOpen for all subsequent connections.
+    tcp_fastopen_status_ = TCP_FASTOPEN_ERROR;
+    g_tcp_fastopen_has_failed = true;
     return rv;
   }
 
-  fast_open_status_ = FAST_OPEN_SLOW_CONNECT_RETURN;
+  tcp_fastopen_status_ = TCP_FASTOPEN_SLOW_CONNECT_RETURN;
   return socket_->WaitForWrite(buf, buf_len, callback);
 }
 
-void TCPSocketLibevent::RecordFastOpenStatus() {
-  if (use_tcp_fastopen_ &&
-      (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN ||
-       fast_open_status_ == FAST_OPEN_SLOW_CONNECT_RETURN)) {
-    DCHECK_NE(FAST_OPEN_STATUS_UNKNOWN, fast_open_status_);
-    bool getsockopt_success(false);
-    bool server_acked_data(false);
+void TCPSocketLibevent::RecordTCPFastOpenStatus() {

[mmenke, 2014/09/24 14:46:11]

Maybe UpdateTCPFastOpenStatusAfterRead?

[Jana, 2014/09/25 16:10:16]

Done.

+  if (!use_tcp_fastopen_ ||
+      (tcp_fastopen_status_ != TCP_FASTOPEN_FAST_CONNECT_RETURN &&
+       tcp_fastopen_status_ != TCP_FASTOPEN_SLOW_CONNECT_RETURN)) {
+    // Not using TCP FastOpen, or status has been finalized.
+    return;
+  }
+  DCHECK_NE(TCP_FASTOPEN_STATUS_UNKNOWN, tcp_fastopen_status_);
+
+  if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
+    // TCP FastOpen connect-with-write was attempted, and failed.
+    tcp_fastopen_status_ =
+      (tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN ?
+       TCP_FASTOPEN_FAST_CONNECT_FAILED : TCP_FASTOPEN_SLOW_CONNECT_FAILED);

[mmenke, 2014/09/24 14:46:11]

nit:  4 space indent.

[Jana, 2014/09/25 16:10:16]

Done.

+    return;
+  }
+
+  bool getsockopt_success(false);
+  bool server_acked_data(false);
 #if defined(TCP_INFO)
-    // Probe to see the if the socket used TCP FastOpen.
-    tcp_info info;
-    socklen_t info_len = sizeof(tcp_info);
-    getsockopt_success =
-        getsockopt(socket_->socket_fd(), IPPROTO_TCP, TCP_INFO,
-                   &info, &info_len) == 0 &&
-        info_len == sizeof(tcp_info);
-    server_acked_data = getsockopt_success &&
-        (info.tcpi_options & TCPI_OPT_SYN_DATA);
+  // Probe to see the if the socket used TCP FastOpen.
+  tcp_info info;
+  socklen_t info_len = sizeof(tcp_info);
+  getsockopt_success = getsockopt(socket_->socket_fd(), IPPROTO_TCP, TCP_INFO,
+                                  &info, &info_len) == 0 &&
+                       info_len == sizeof(tcp_info);
+  server_acked_data = getsockopt_success &&
+                      (info.tcpi_options & TCPI_OPT_SYN_DATA);
 #endif
-    if (getsockopt_success) {
-      if (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN) {
-        fast_open_status_ = (server_acked_data ? FAST_OPEN_SYN_DATA_ACK :
-                             FAST_OPEN_SYN_DATA_NACK);
-      } else {
-        fast_open_status_ = (server_acked_data ? FAST_OPEN_NO_SYN_DATA_ACK :
-                             FAST_OPEN_NO_SYN_DATA_NACK);
-      }
+
+  if (getsockopt_success) {
+    if (tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN) {
+      tcp_fastopen_status_ = (server_acked_data ?
+                              TCP_FASTOPEN_SYN_DATA_ACK :
+                              TCP_FASTOPEN_SYN_DATA_NACK);
     } else {
-      fast_open_status_ = (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN ?
-                           FAST_OPEN_SYN_DATA_FAILED :
-                           FAST_OPEN_NO_SYN_DATA_FAILED);
+      tcp_fastopen_status_ = (server_acked_data ?
+                              TCP_FASTOPEN_NO_SYN_DATA_ACK :
+                              TCP_FASTOPEN_NO_SYN_DATA_NACK);
     }
+  } else {
+    tcp_fastopen_status_ =
+        (tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN ?
+         TCP_FASTOPEN_SYN_DATA_GETSOCKOPT_FAILED :
+         TCP_FASTOPEN_NO_SYN_DATA_GETSOCKOPT_FAILED);
   }
 }