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..97062efe1a42fc0a2eee88dffde751a60566a212 100644
--- a/net/socket/tcp_socket_libevent.cc
+++ b/net/socket/tcp_socket_libevent.cc
@@ -39,6 +39,8 @@ 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 +131,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,10 +142,7 @@ TCPSocketLibevent::TCPSocketLibevent(NetLog* net_log,
 
 TCPSocketLibevent::~TCPSocketLibevent() {
   net_log_.EndEvent(NetLog::TYPE_SOCKET_ALIVE);
-  if (tcp_fastopen_connected_) {
-    UMA_HISTOGRAM_ENUMERATION("Net.TcpFastOpenSocketConnection",
-                              fast_open_status_, FAST_OPEN_MAX_VALUE);
-  }
+  Close();
 }
 
 int TCPSocketLibevent::Open(AddressFamily family) {
@@ -222,7 +222,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 +239,7 @@ bool TCPSocketLibevent::IsConnected() const {
   if (!socket_)
     return false;
 
-  if (use_tcp_fastopen_ && !tcp_fastopen_connected_ &&
+  if (use_tcp_fastopen_ && !tcp_fastopen_write_attempted_ &&

[mmenke, 2014/09/30 14:30:34]

One more question:  tcp_fastopen_write_attempted_ is set to true on the first
write attempt, not when the first write (Which is also really the connection
attempt) completes.  Will socket_->IsConnected return true that soon?

[Jana, 2014/10/01 01:54:00]

Good question -- it took me some digging to figure this out. So the answer turns
out is yes. socket_->IsConnected() uses recv() and looks for EWOULDBLOCK to
indicate that the socket is connected. If our TCP FastOpen sendto() returns
success, then any subsequent read on a socket which has a TFO write in progress
returns EWOULDBLOCK, which means socket_->IsConnected returns true. If the
sendto() returns -1, with EINPROGRESS, that translates to IO_PENDING, which also
results in the same outcome on socket_->IsConnected. If the write fails
otherwise, then it's an error. So, as long as the Write() (connect) has started,
socket_->IsConnected() will return true. Does that answer your question?

       socket_->HasPeerAddress()) {
     // With TCP FastOpen, we pretend that the socket is connected.
     // This allows GetPeerAddress() to return peer_address_.
@@ -268,8 +268,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 +286,8 @@ 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_) {
     rv = TcpFastOpenWrite(buf, buf_len, write_callback);
   } else {
     rv = socket_->Write(buf, buf_len, write_callback);
@@ -414,8 +413,17 @@ bool TCPSocketLibevent::SetNoDelay(bool no_delay) {
 
 void TCPSocketLibevent::Close() {
   socket_.reset();
+
+  // Record and reset TCP FastOpen state.
+  if (tcp_fastopen_write_attempted_ ||
+      tcp_fastopen_status_ == TCP_FASTOPEN_PREVIOUSLY_FAILED) {
+    UMA_HISTOGRAM_ENUMERATION("Net.TcpFastOpenSocketConnection",
+                              tcp_fastopen_status_, TCP_FASTOPEN_MAX_VALUE);
+  }
+  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,8 +431,17 @@ bool TCPSocketLibevent::UsingTCPFastOpen() const {
 }
 
 void TCPSocketLibevent::EnableTCPFastOpenIfSupported() {
-  if (IsTCPFastOpenSupported())
+  if (!IsTCPFastOpenSupported())
+    return;
+
+  // 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(!g_tcp_fastopen_has_failed)
     use_tcp_fastopen_ = true;
+  else
+    tcp_fastopen_status_ = TCP_FASTOPEN_PREVIOUSLY_FAILED;
 }
 
 bool TCPSocketLibevent::IsValid() const {
@@ -553,14 +570,27 @@ 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_) {
+    // 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 (jri): This currently results in conservative behavior, where TCP
+    // FastOpen is turned off on _any_ error. Implement optimizations,
+    // such as turning off TCP FastOpen on more specific errors, and
+    // re-attempting TCP FastOpen after a certain amount of time has passed.
+    if (rv >= 0)

[mmenke, 2014/09/30 14:30:34]

Is 0 a reliable indicator of success here?

[Jana, 2014/10/01 01:54:00]

I think so. 0 indicates a close, not an error, which is what should happen on
any (including TFO) failure. As I understand it, if a TFO connect fails
underneath, read returns -1 with errno set to ECONNREFUSED.

[mmenke, 2014/10/01 02:11:12]

Right, my concern was just that a confused server could somehow managed to make
this look like a graceful close...  Can't really see how that would happen,
though.

+      tcp_fastopen_connected_ = true;
+    else
+      g_tcp_fastopen_has_failed = true;
+    UpdateTCPFastOpenStatusAfterRead();
+  }
+
   if (rv < 0) {
     net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
                       CreateNetLogSocketErrorCallback(rv, errno));
@@ -576,13 +606,24 @@ 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.
+      // TODO (jri): This currently results in conservative behavior, where TCP
+      // FastOpen is turned off on _any_ error. Implement optimizations,
+      // such as turning off TCP FastOpen on more specific errors, and
+      // re-attempting TCP FastOpen after a certain amount of time has passed.
+      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;
@@ -606,10 +647,11 @@ int TCPSocketLibevent::TcpFastOpenWrite(
 
   int flags = 0x20000000;  // Magic flag to enable TCP_FASTOPEN.
 #if defined(OS_LINUX)
-  // sendto() will fail with EPIPE when the system doesn't support TCP Fast
-  // Open. Theoretically that shouldn't happen since the caller should check
-  // for system support on startup, but users may dynamically disable TCP Fast
-  // Open via sysctl.
+  // sendto() will fail with EPIPE when the system doesn't implement TCP
+  // FastOpen, and with EOPNOTSUPP when the system implements TCP FastOpen
+  // but it is disabled. Theoretically these shouldn't happen
+  // since the caller should check for system support on startup, but
+  // users may dynamically disable TCP FastOpen via sysctl.
   flags |= MSG_NOSIGNAL;
 #endif // defined(OS_LINUX)
   rv = HANDLE_EINTR(sendto(socket_->socket_fd(),
@@ -618,10 +660,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 +681,69 @@ 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
+    // since TCP FastOpen was not implemented or disabled in the OS.
+    // Record status and turn off TCP FastOpen for all subsequent connections.
+    // TODO (jri): This is almost certainly too conservative, since it blanket
+    // turns off TCP FastOpen on any write error. Two things need to be done
+    // here: (i) record a histogram of write errors; in particular, record
+    // occurrences of EOPNOTSUPP and EPIPE, and (ii) afterwards, consider
+    // turning off TCP FastOpen on more specific errors.
+    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::UpdateTCPFastOpenStatusAfterRead() {
+  if (!use_tcp_fastopen_ ||
+      (tcp_fastopen_status_ != TCP_FASTOPEN_FAST_CONNECT_RETURN &&
+       tcp_fastopen_status_ != TCP_FASTOPEN_SLOW_CONNECT_RETURN)) {

[mmenke, 2014/09/30 14:30:34]

I think it's a little weird that here we check tcp_fastopen_status_, but in
HandleReadCompleted we check tcp_fastopen_connected_.  In both places, we're
largely interested in the same thing.

Do we even need the tcp_fastopen_status_ check there?

[Jana, 2014/10/01 01:54:00]

We don't need the fastopen status check here at all... since we shouldn't be
calling this method unless we're in the ReadCompleted call with
tcp_fastopen_write_attempted && !tcp_fastopen_connected. This was leftover code
from earlier that I didn't clean up... I think its safe now to remove this, so
I'm removing it, and adding a DCHECK.

[mmenke, 2014/10/01 02:11:12]

Two read failures in a row could get us here...But yea, after one read failure,
no one should be reading from us again.

+    // 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_READ_FAILED :
+            TCP_FASTOPEN_SLOW_CONNECT_READ_FAILED);
+    return;
+  }
+
+  bool getsockopt_success(false);
+  bool server_acked_data(false);

[mmenke, 2014/09/30 14:30:34]

nit:  While you're here, "=" is almost always used to initialize fundamental
types and enums

[Jana, 2014/10/01 01:54:00]

Done.

 #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);
   }
 }