Report the correct os_error in the SOCKET_READ_ERROR event

net/socket/tcp_client_socket_win.cc

Index: net/socket/tcp_client_socket_win.cc
===================================================================
--- net/socket/tcp_client_socket_win.cc	(revision 187920)
+++ net/socket/tcp_client_socket_win.cc	(working copy)
@@ -957,7 +957,6 @@
   DWORD num_bytes, flags;
   BOOL ok = WSAGetOverlappedResult(socket_, &core_->read_overlapped_,
                                    &num_bytes, FALSE, &flags);
-  WSAResetEvent(core_->read_overlapped_.hEvent);
   waiting_read_ = false;
   int rv;
   if (ok) {
@@ -975,6 +974,7 @@
     net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
                       CreateNetLogSocketErrorCallback(rv, os_error));
   }
+  WSAResetEvent(core_->read_overlapped_.hEvent);
   core_->read_iobuffer_ = NULL;
   core_->read_buffer_length_ = 0;
   DoReadCallback(rv);
@@ -1025,22 +1025,33 @@
   if (rv == SOCKET_ERROR) {
     os_error = WSAGetLastError();
     rv = MapSystemError(os_error);
-  } else if (network_events.lNetworkEvents & FD_READ) {
-    rv = DoRead(core_->read_iobuffer_, core_->read_buffer_length_,
-                read_callback_);
-    if (rv == ERR_IO_PENDING)
-      return;
-  } else if (network_events.lNetworkEvents & FD_CLOSE) {
-    if (network_events.iErrorCode[FD_CLOSE_BIT]) {
-      rv = MapSystemError(network_events.iErrorCode[FD_CLOSE_BIT]);
-      net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
-                        CreateNetLogSocketErrorCallback(rv, os_error));
+  } else if (network_events.lNetworkEvents) {
+    DCHECK_EQ(network_events.lNetworkEvents & ~(FD_READ | FD_CLOSE), 0);
+    if (network_events.lNetworkEvents == FD_CLOSE &&
+        network_events.iErrorCode[FD_CLOSE_BIT] == 0) {
+      // Graceful connection closure.
+      // TODO(wtc): should we still call DoRead() in this case? The MSDN
+      // page for WSAEventSelect says:
+      //   An application should check for remaining data upon receipt
+      //   of FD_CLOSE to avoid any possibility of losing data.
+      rv = 0;

[wtc, 2013/03/14 01:57:24]

I added this case as a performance optimization. However,
the quoted sentence from the MSDN page gave me second thoughts.

What do you think? Should I remove this special case?

[Pat Meenan, 2013/03/14 13:46:47]

I'd remove the special case since the docs indicate that we should still be
doing a read.  It's not much of a performance optimization anyway (effectively a
recv call) and it was one of the potential bugs that your patch was fixing.

[wtc, 2013/03/14 18:28:04]

Done.

[rvargas (doing something else), 2013/03/14 18:48:16]

I think that the optimization is fine and the doc comment is just talking about
the fact that FD_READ and FD_CLOSE can be signaled at the same time. On the
other hand, I doubt the gains are that important because the extra call should
return immediately (but it is an extra syscall).

That is to say, I'm fine either way but simpler code is slightly better if
there's no performance impact.

     } else {
-      rv = 0;
+      // If network_events.iErrorCode[FD_READ_BIT] or
+      // network_events.iErrorCode[FD_CLOSE_BIT] is nonzero, still call
+      // DoRead() because recv() reports a more accurate error code
+      // (WSAECONNRESET vs. WSAECONNABORTED) when the connection was
+      // reset.

[Pat Meenan, 2013/03/14 13:46:47]

Can you add to the comment after removing the special case above that we also
need to do the read according to the docs in the case of an FD_CLOSE.

[wtc, 2013/03/14 18:28:04]

Done.

+      rv = DoRead(core_->read_iobuffer_, core_->read_buffer_length_,
+                  read_callback_);
+      if (rv == ERR_IO_PENDING)
+        return;
     }
   } else {
-    // This should not happen but I have seen cases where we will get
-    // signaled but the network events flags are all clear (0).
+    // This may happen because Read() may succeed synchronously and
+    // consume all the received data without resetting the event object.
+    // Rather than trying to reset the event object properly, it seems

[rvargas (doing something else), 2013/03/14 18:48:16]

nit: I'd remove this second sentence because it implies that the proper way of
fixing the code is to add an extra WSAResetEvent as opposed to changing the
structure of the loop.

+    // more efficient to skip the WSAResetEvent system call on
+    // synchronous success of Read() and handle spurious wakeups here.
     core_->WatchForRead();
     return;
   }