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..51b7122b6265e6996f992fbaeaf395b4989dcb3a 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;
+
 

[mmenke, 2014/09/29 17:49:34]

nit:  Remove extra blank line.

[Jana, 2014/09/29 23:01:46]

Done.

 // 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,10 +143,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 +223,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 +240,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 +269,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);

[mmenke, 2014/09/29 17:49:34]

Previously, we didn't call RecordFastOpenStatus on synchronous read errors. 
That was a bug, right?

[Jana, 2014/09/29 23:01:46]

Yes, I believe so. That's why I moved it to HandleReadCompleted to ensure that
it was always called at the end of a read.

   return rv;
@@ -288,7 +287,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 +414,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 +432,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;

[mmenke, 2014/09/29 17:49:34]

This doesn't quite match the not-yet-failed case - it doesn't catch socket
reuses (Which is probably irrelevant), and does catch sockets that we never
actually call connect on.  I think this is coode enough, but maybe a comment
about it?

[Jana, 2014/09/29 23:01:47]

Yes, you're right on both accounts. I agree that it's reasonable to drop the
socket reuse cases (we're conditioning all of this on SSL sockets at this point
anyways), and I did think about doing it more accurately on a Write() call. I
estimated that this is reasonably adequate, especially since we're now only
measuring this for SSL sockets, and I expect that it should be uncommon for a
subsequent connect to not occur. I've added a comment to tcp_socket_libevent.h
and a TODO; PTAL.

 }
 
 bool TCPSocketLibevent::IsValid() const {
@@ -553,14 +571,25 @@ 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(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;
+    UpdateTCPFastOpenStatusAfterRead();

[mmenke, 2014/09/29 17:49:34]

Not relevant for this CL, but...  So if we ever get an error during connect,
like ERR_NO_ROUTE_TO_HOST, we disable fastopen until restart?

[mmenke, 2014/09/29 17:51:55]

Not relevant meaning I don't want to force a more complicated / smarter solution
in this CL, but I think we should think about it.

[Jana, 2014/09/29 23:01:46]

Yes we are more conservative than we need to be. We could condition turning TFO
off on more specific errors... I would also like to do optimizations to
re-enabling fastopen, such as re-enabling after a few hours, for instance.
Adding TODOs everywhere this flag is set to true to document this point. I've
also added a next step for filtering based on more specific write() errors. I
did think about this, and I think that data has to point the way here. I agree
that smarter machinery is in order, but we'll have to do this carefully to keep
the user from getting persistently blackholed.

+  }
+
   if (rv < 0) {
     net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
                       CreateNetLogSocketErrorCallback(rv, errno));
@@ -576,13 +605,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.

[mmenke, 2014/09/29 17:49:34]

This comment seems to be making the assumption that it was the first write that
failed, but this isn't necessarily true.  We may end up writing to a socket
multiple times before we end up reading from it, and this code triggers as long
as we haven't completed a read yet (successfully or with error).  Do we care to
distinguish fails on the first write vs failures on a subsequent write?

Also, if an embedder tries to read from and write to a socket at once, and both
fail, the read error always takes precedence with this code, as long as the
socket is alive long enough, regardless of the order we learn about the errors
from.  Do we care?

[Jana, 2014/09/29 23:01:47]

Hmm... good point. I am comfortable, especially given the scope of our
experiment at this point (SSL only), to make this assumption. Either ways, this
seems to me to fall under the category of better resilience on failures with
FastOpen, and I'm comfortable leaving a note here to document this point.
There are other issues with an embedder attempting read before a write: the read
will fail on a TFO socket (this is the read-before-write issue on a TFO socket
that is yet unresolved, and a major reason for limiting this experiment to SSL).
That said, yes, I think the read() error is the one that I explicitly wanted to
give precedence to. I hadn't thought about the case you describe, but TFO
failure modes are better articulated on a read, since we get additional
information from the kernel there.

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

I should note I was wrong here.  UpdateTCPFastOpenStatusAfterRead checks
tcp_fastopen_status_, and does nothing on certain statuses.

+      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 +654,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 +675,64 @@ 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::UpdateTCPFastOpenStatusAfterRead() {
+  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_READ_FAILED :
+            TCP_FASTOPEN_SLOW_CONNECT_READ_FAILED);
+    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);
   }
 }