Switch the TCP reads on Windows to use non-blocking/non-async I/O.

net/socket/tcp_client_socket_win.cc

Index: net/socket/tcp_client_socket_win.cc
===================================================================
--- net/socket/tcp_client_socket_win.cc	(revision 161340)
+++ net/socket/tcp_client_socket_win.cc	(working copy)
@@ -28,6 +28,7 @@
 namespace {
 
 const int kTCPKeepAliveSeconds = 45;
+bool g_disable_overlapped_reads = false;
 
 bool SetSocketReceiveBufferSize(SOCKET socket, int32 size) {
   int rv = setsockopt(socket, SOL_SOCKET, SO_RCVBUF,
@@ -191,6 +192,7 @@
   // The buffers used in Read() and Write().
   scoped_refptr<IOBuffer> read_iobuffer_;
   scoped_refptr<IOBuffer> write_iobuffer_;
+  int read_buffer_length_;
   int write_buffer_length_;
 
   // Throttle the read size based on our current slow start state.
@@ -258,6 +260,7 @@
 TCPClientSocketWin::Core::Core(
     TCPClientSocketWin* socket)
     : write_buffer_length_(0),
+      read_buffer_length_(0),
       socket_(socket),
       ALLOW_THIS_IN_INITIALIZER_LIST(reader_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(writer_(this)),
@@ -301,7 +304,11 @@
     if (core_->socket_->waiting_connect()) {
       core_->socket_->DidCompleteConnect();
     } else {
-      core_->socket_->DidCompleteRead();
+      if (g_disable_overlapped_reads) {

[Ryan Sleevi, 2012/10/12 18:53:13]

BUG? What happens if overlapped reads are disabled after sockets have been
created? It seems like you'll get into a bad state.

My suggestion would be that each TCPClientSocketWin::Core, when created, stores
the value of whatever g_disable_overlapped_reads was, so that for the lifetime
of the socket, its state machine can never become confused.

[pmeenan, 2012/10/17 15:12:31]

Done.

+        core_->socket_->DidSignalRead();
+      } else {
+        core_->socket_->DidCompleteRead();
+      }
     }
   }
 
@@ -354,6 +361,10 @@
   SetNonBlocking(socket_);
 
   core_ = new Core(this);
+  if (g_disable_overlapped_reads) {
+    WSAEventSelect(socket_, core_->read_overlapped_.hEvent,
+                   FD_READ | FD_CLOSE);
+  }
 
   current_address_index_ = 0;
   use_history_.set_was_ever_connected();
@@ -485,7 +496,12 @@
   core_ = new Core(this);
   // WSAEventSelect sets the socket to non-blocking mode as a side effect.
   // Our connect() and recv() calls require that the socket be non-blocking.
-  WSAEventSelect(socket_, core_->read_overlapped_.hEvent, FD_CONNECT);
+  if (g_disable_overlapped_reads) {
+    WSAEventSelect(socket_, core_->read_overlapped_.hEvent,
+                   FD_CONNECT | FD_READ | FD_CLOSE);
+  } else {
+    WSAEventSelect(socket_, core_->read_overlapped_.hEvent, FD_CONNECT);
+  }
 
   SockaddrStorage storage;
   if (!endpoint.ToSockAddr(storage.addr, &storage.addr_len))
@@ -711,41 +727,67 @@
   DCHECK(read_callback_.is_null());
   DCHECK(!core_->read_iobuffer_);
 
-  buf_len = core_->ThrottleReadSize(buf_len);
-
-  WSABUF read_buffer;
-  read_buffer.len = buf_len;
-  read_buffer.buf = buf->data();
-
-  // TODO(wtc): Remove the assertion after enough testing.
-  AssertEventNotSignaled(core_->read_overlapped_.hEvent);
-  DWORD num, flags = 0;
-  int rv = WSARecv(socket_, &read_buffer, 1, &num, &flags,
-                   &core_->read_overlapped_, NULL);
-  if (rv == 0) {
-    if (ResetEventIfSignaled(core_->read_overlapped_.hEvent)) {
+  if (g_disable_overlapped_reads) {
+    int flags = 0;
+    int rv = recv(socket_, buf->data(), buf_len, flags);
+    if (rv == SOCKET_ERROR) {
+      int os_error = WSAGetLastError();
+      if (os_error != WSAEWOULDBLOCK) {
+        int net_error = MapSystemError(os_error);
+        net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
+                         CreateNetLogSocketErrorCallback(net_error, os_error));
+        return net_error;
+      }
+    } else {
       base::StatsCounter read_bytes("tcp.read_bytes");
-      read_bytes.Add(num);
-      num_bytes_read_ += num;
-      if (num > 0)
+      if (rv > 0) {
         use_history_.set_was_used_to_convey_data();
-      net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, num,
+        read_bytes.Add(rv);
+        num_bytes_read_ += rv;
+      }
+      net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, rv,
                                     buf->data());
-      return static_cast<int>(num);
+      return rv;
     }
   } else {
-    int os_error = WSAGetLastError();
-    if (os_error != WSA_IO_PENDING) {
-      int net_error = MapSystemError(os_error);
-      net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
-                        CreateNetLogSocketErrorCallback(net_error, os_error));
-      return net_error;
+    buf_len = core_->ThrottleReadSize(buf_len);
+
+    WSABUF read_buffer;
+    read_buffer.len = buf_len;
+    read_buffer.buf = buf->data();
+
+    // TODO(wtc): Remove the assertion after enough testing.
+    AssertEventNotSignaled(core_->read_overlapped_.hEvent);
+    DWORD num, flags = 0;
+    int rv = WSARecv(socket_, &read_buffer, 1, &num, &flags,
+                     &core_->read_overlapped_, NULL);
+    if (rv == 0) {
+      if (ResetEventIfSignaled(core_->read_overlapped_.hEvent)) {
+        base::StatsCounter read_bytes("tcp.read_bytes");
+        read_bytes.Add(num);
+        num_bytes_read_ += num;
+        if (num > 0)
+          use_history_.set_was_used_to_convey_data();
+        net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, num,
+                                      buf->data());
+        return static_cast<int>(num);
+      }
+    } else {
+      int os_error = WSAGetLastError();
+      if (os_error != WSA_IO_PENDING) {
+        int net_error = MapSystemError(os_error);
+        net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
+                         CreateNetLogSocketErrorCallback(net_error, os_error));
+        return net_error;
+      }
     }
   }
-  core_->WatchForRead();
+
   waiting_read_ = true;
   read_callback_ = callback;
   core_->read_iobuffer_ = buf;
+  core_->read_buffer_length_ = buf_len;
+  core_->WatchForRead();
   return ERR_IO_PENDING;
 }
 
@@ -765,7 +807,6 @@
   WSABUF write_buffer;
   write_buffer.len = buf_len;
   write_buffer.buf = buf->data();
-  core_->write_buffer_length_ = buf_len;
 
   // TODO(wtc): Remove the assertion after enough testing.
   AssertEventNotSignaled(core_->write_overlapped_.hEvent);
@@ -799,10 +840,11 @@
       return net_error;
     }
   }
-  core_->WatchForWrite();
   waiting_write_ = true;
   write_callback_ = callback;
   core_->write_iobuffer_ = buf;
+  core_->write_buffer_length_ = buf_len;
+  core_->WatchForWrite();
   return ERR_IO_PENDING;
 }
 
@@ -900,6 +942,54 @@
   }
 }
 
+void TCPClientSocketWin::DidSignalRead() {

[Ryan Sleevi, 2012/10/12 18:53:13]

style nit: The order in the implementation should match the order in the header.

Move this to line 1055 (after DidCompleteWrite)

[pmeenan, 2012/10/17 15:12:31]

Done.

+  DCHECK(waiting_read_);
+  int rv;
+  int os_error = 0, flags = 0;

[Ryan Sleevi, 2012/10/12 18:53:13]

style nit: Chromium style is to have one variable declaration per line.

int os_error = 0;
int flags = 0;

[pmeenan, 2012/10/17 15:12:31]

Done.

+  WSANETWORKEVENTS network_events;
+  rv = WSAEnumNetworkEvents(socket_, core_->read_overlapped_.hEvent,
+                              &network_events);

[Ryan Sleevi, 2012/10/12 18:53:13]

style nit: whitespace

[pmeenan, 2012/10/17 15:12:31]

Done.

+  if (rv != SOCKET_ERROR) {

[rvargas (doing something else), 2012/10/13 03:34:06]

nit: invert the order if the condition.

[pmeenan, 2012/10/17 15:12:31]

Done.

+    if (network_events.lNetworkEvents & FD_READ) {
+      rv = recv(socket_, core_->read_iobuffer_->data(),
+                core_->read_buffer_length_, flags);
+      if (rv == SOCKET_ERROR) {
+        os_error = WSAGetLastError();
+        if (os_error == WSAEWOULDBLOCK) {
+          core_->WatchForRead();
+          return;
+        } else {
+          rv = MapSystemError(os_error);
+        }
+      }
+    } else {
+      core_->WatchForRead();

[Ryan Sleevi, 2012/10/12 18:53:13]

Do you also need to be watching for FD_CLOSE? Will it be signalled as an FD_READ
of 0 in the event of an unclean close?

If it is signalled as FD_CLOSE, and we ignore it here, does that means we're
potentially holding on indefinitely to a socket that is effectively dead?

ISTM that this conditional should be much more aggressive about handling
expected values and explicitly failing for unhandled values.

[pmeenan, 2012/10/17 15:12:31]

Added the FD_CLOSE handling.  FD_READ does not get signaled in the case of a
close.

+      return;
+    }
+  } else {
+    os_error = WSAGetLastError();
+    rv = MapSystemError(os_error);
+  }
+  if (rv > 0) {
+      base::StatsCounter read_bytes("tcp.read_bytes");

[rvargas (doing something else), 2012/10/13 03:34:06]

This block logically follows the recv call so it would be clear if it's done
before leaving that if()... which points me to the fact that we should extract
the common code that calls recv from here and TCPClientSocketWin::Read().

[pmeenan, 2012/10/17 15:12:31]

This got a lot cleaner when I cleaned it up and it now follows the recv.

+      if (rv > 0) {
+        use_history_.set_was_used_to_convey_data();
+        read_bytes.Add(rv);
+        num_bytes_read_ += rv;
+      }
+      net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, rv,
+                                    core_->read_iobuffer_->data());
+  } else {
+    rv = MapSystemError(os_error);

[rvargas (doing something else), 2012/10/13 03:34:06]

nit: It's a little funky that we can end up here with rv being a system error
and we overwrite it again.

[pmeenan, 2012/10/17 15:12:31]

Should be a lot cleaner now.

+    net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
+                      CreateNetLogSocketErrorCallback(rv, os_error));
+  }
+  waiting_read_ = false;
+  core_->read_iobuffer_ = NULL;
+  core_->read_buffer_length_ = 0;
+  DoReadCallback(rv);
+}
+
 void TCPClientSocketWin::DidCompleteRead() {
   DCHECK(waiting_read_);
   DWORD num_bytes, flags;
@@ -963,4 +1053,8 @@
   DoWriteCallback(rv);
 }
 
+void TCPClientSocketWin::DisableOverlappedReads() {

[Ryan Sleevi, 2012/10/12 18:53:13]

style nit: The order in the implementation should match the order in the header.

Move this to line 868 (after SetNoDelay)

[pmeenan, 2012/10/17 15:12:31]

Done.

+  g_disable_overlapped_reads = true;
+}
+
 }  // namespace net