Retrieve more accurate error code (WSAECONNRESET vs.

net/socket/tcp_client_socket_win.cc

Index: net/socket/tcp_client_socket_win.cc
===================================================================
--- net/socket/tcp_client_socket_win.cc	(revision 185536)
+++ net/socket/tcp_client_socket_win.cc	(working copy)
@@ -266,7 +266,7 @@
     TCPClientSocketWin* socket)
     : read_buffer_length_(0),
       write_buffer_length_(0),
-      disable_overlapped_reads_(g_disable_overlapped_reads),
+      disable_overlapped_reads_(true),

[wtc, 2013/03/06 17:33:43]

Please ignore this change. I need this temporary change
to force non-blocking reads in my debugging.

       non_blocking_reads_initialized_(false),
       socket_(socket),
       ALLOW_THIS_IN_INITIALIZER_LIST(reader_(this)),
@@ -840,6 +840,9 @@
                      FD_READ | FD_CLOSE);
       core_->non_blocking_reads_initialized_ = true;
     }
+    // Sometimes core_->read_overlapped_.hEvent is signaled at this
+    // point. It is usually the 3rd-5th times we call recv() on the
+    // socket, never the first time.
     int rv = recv(socket_, buf->data(), buf_len, 0);
     if (rv == SOCKET_ERROR) {
       int os_error = WSAGetLastError();
@@ -858,6 +861,8 @@
       }
       net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, rv,
                                     buf->data());
+      BOOL ok = WSAResetEvent(core_->read_overlapped_.hEvent);

[rvargas (doing something else), 2013/03/06 23:08:49]

I think this can cause unwanted hangs (unlikely, but possible). There can be
more bytes available right after we come back from recv, and clearing the event
would prevent us from knowing about them, unless new bytes come from the net.

We could manually reset the event right before calling recv, but that would be
reducing the size of the race window, not eliminating it.

[Pat Meenan, 2013/03/07 15:11:04]

What are you trying to solve by manually resetting the event here?  It gets
reset automatically in WSAEnumNetworkEvents which should be atomic and eliminate
any race conditions, keeping the state management all within winsock.

From:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741690(v=vs.85).aspx

"The proper way to reset the state of an event object used with the
WSAEventSelect function is to pass the handle of the event object to the
WSAEnumNetworkEvents function in the hEventObject parameter. This will reset the
event object and adjust the status of active FD events on the socket in an
atomic fashion."

+      CHECK(ok);
       return rv;
     }
   } else {
@@ -1022,25 +1027,20 @@
   WSANETWORKEVENTS network_events;
   int rv = WSAEnumNetworkEvents(socket_, core_->read_overlapped_.hEvent,
                                 &network_events);
+  AssertEventNotSignaled(core_->read_overlapped_.hEvent);

[wtc, 2013/03/06 17:33:43]

This asserts that WSAEnumNetworkEvents resets the event object
(on success), as documented.

[rvargas (doing something else), 2013/03/06 23:08:49]

Sounds like a race to me.

[Pat Meenan, 2013/03/07 15:11:04]

Agreed, it's not atomic so it's theoretically possible for data to arrive
between the WSAEnumNetworkEvents call and the Assert.  Not likely but still
possible.

   if (rv == SOCKET_ERROR) {
+    NOTREACHED();

[rvargas (doing something else), 2013/03/06 23:08:49]

I'm assuming this is for development purposes... I don't think we should leave
this checked in.

     os_error = WSAGetLastError();
     rv = MapSystemError(os_error);
-  } else if (network_events.lNetworkEvents & FD_READ) {
+  } else if (network_events.lNetworkEvents) {
     rv = DoRead(core_->read_iobuffer_, core_->read_buffer_length_,
                 read_callback_);

[wtc, 2013/03/06 17:33:43]

I got the inspiration of this change from the old-fashioned
way of using select(), or the modern way of using libevent.
In those models, an error condition also causes the socket
to be reported as readable, and we need to call recv() on
the socket and let it fail to find out what the error is.

I tried this and indeed this retrieves the more accurate
WSAECONNRESET error after the recv() call failed.

I also saw recv() returns 0 on EOF.

[Pat Meenan, 2013/03/07 15:11:04]

Is the last error from the recv() here as granular as the error code in the
FD_CLOSE_BIT in the case of a close?  Generally I like this because it does
handle the case of incoming data in a close event which was a bug in my
implementation but I'm just wondering if you also need to keep the FD_CLOSE
error code in the case that the recv fails or if the error in the recv is good
enough.

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

Yes, in fact it is better.

When the peer resets the connection, the error code in the
FD_CLOSE_BIT is WSAECONNABORTED, but the last error from
recv() is WSAECONNRESET, which is more accurate.

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

[wtc, 2013/03/06 17:33:43]

If you prefer a smaller change, I can call DoRead() only
inside this if block. I would replace lines 1035-1037 with
    rv = DoRead(core_->read_iobuffer_, core_->read_buffer_length_,
                read_callback_);
    DCHECK(rv < 0 && rv != ERR_IO_PENDING);

-    } else {
-      rv = 0;
-    }
   } else {
     // This should not happen but I have seen cases where we will get
     // signaled but the network events flags are all clear (0).

[wtc, 2013/03/06 17:33:43]

The WSAResetEvent call I added on line 864 eliminates this
spurious wakeup in my debugging.

I also verified the spurious wakeup was not caused by the
recv(MSG_PEEK) call made in IsConnected() and IsConnectedAndIdle().

[rvargas (doing something else), 2013/03/06 23:08:49]

I'm thinking that we either accept a spurious wake up, or we pay th eprice of an
extra recv that should return would_block.

[Pat Meenan, 2013/03/07 15:11:04]

How often do the Spurious wake-ups happen from IsConnected() and
IsConnectedAndIdle()?  It's light enough in here that I'm much more comfortable
just letting them happen than trying to manually toggle the event.  I'd just
change the comment since you know what causes them to happen.

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

I confirmed that the spurious wake-ups are NOT caused by
IsConnected() and IsConnectedAndIdle().  Under the current
code, spurious wake-ups are to be expected. 
Done.

+    NOTREACHED();

[rvargas (doing something else), 2013/03/06 23:08:49]

same thing here. If this triggers for a developer actively working on this code,
everything is fine. If this triggers for a random developer things are not good,
and this doesn't really check a code invariant (it depends on OS behavior), so I
expect it to hit at some point.

     core_->WatchForRead();
     return;
   }