Report the correct os_error in the SOCKET_READ_ERROR event

Retrieve more accurate error code (WSAECONNRESET vs.
WSAECONNABORTED) than what WSAEnumNetworkEvents reported
in network_events.iErrorCode[FD_CLOSE_BIT} by calling
recv() on the socket.

Document why spurious wakeups are expected and tolerated.

Revert r186218: net: also do TLS 1.1 -> 1.0 fallback on
ERR_CONNECTION_ABORTED. The workaround is no longer needed.

R=pmeenan@chromium.org,rvargas@chromium.org,agl@chromium.org
BUG=180313, 178672, 179037
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188278

[wtc, 2013-03-05 20:17:03]

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_win.cc (left):

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:1037: CreateNetLogSocketErrorCallback(rv,
os_error));

This is the bug. Note that we map
network_events.iErrorCode[FD_CLOSE_BIT] to |rv|, but
os_error still has the initial value of 0. This is why
the SOCKET_READ_ERROR event has os_error = 0.

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:977:
WSAResetEvent(core_->read_overlapped_.hEvent);

This change is just defensive programming. Between the
WSAGetOverlappedResult() call that failed and the
WSAGetLastError() call, we should avoid any Win32 function
call that may change the last error.

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:1029: if
(network_events.iErrorCode[FD_READ_BIT]) {

According to MSDN documentation and the sample code in the
book "Network Programming for Microsoft Windows", we also
need to check for a nonzero error code for the FD_READ event.

Note:

1. In my debugging this code path is never taken. But I
think we should add this code anyway.

2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
the order in which we check for FD_READ and FD_CLOSE is
actually important. Perhaps we should add a comment about
that.

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:1053: core_->WatchForRead();

I also saw this in my debugging. This occurs while I am not
browsing. So I suspect this is caused by the non-blocking
recv(MSG_PEEK) calls in IsConnected() and IsConnectedAndIdle().
I did not verify that.

This CL does not address this potential problem.

[Pat Meenan, 2013-03-05 20:55:32]

lgtm

[rvargas (doing something else), 2013-03-05 23:14:07]

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:1029: if
(network_events.iErrorCode[FD_READ_BIT]) {
On 2013/03/05 20:17:03, wtc wrote:
> 
> According to MSDN documentation and the sample code in the

could you point me to that sample?

> book "Network Programming for Microsoft Windows", we also
> need to check for a nonzero error code for the FD_READ event.

My reading of the doc doesn't tell me that this path is needed. On the other
hand, I would not bet against being able to find a client configuration that
actually returns an error here... and adding more error handling is never wrong
:)

> 
> Note:
> 
> 1. In my debugging this code path is never taken. But I
> think we should add this code anyway.
> 
> 2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
> the order in which we check for FD_READ and FD_CLOSE is
> actually important. Perhaps we should add a comment about
> that.

This sounds like the real issue. It is totally fine to report multiple events
simultaneously, and in that case we should pick the one we care the most about
(if we don't go over all of them). In other words, we should invert the logic
and test first for close events.

[wtc, 2013-03-06 00:00:03]

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:1029: if
(network_events.iErrorCode[FD_READ_BIT]) {

On 2013/03/05 23:14:07, rvargas wrote:
>
> could you point me to that sample?

I will bring the book to the office tomorrow.

> My reading of the doc doesn't tell me that this path is needed. On the other
> hand, I would not bet against being able to find a client configuration that
> actually returns an error here... and adding more error handling is never
wrong
> :)

I agree.  The MSDN page for WSAEnumNetworkEvents
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741572%28v=vs.85%29...
only mentions FD_READ_BIT and FD_WRITE_BIT without
providing sample code, but I think that implies FD_READ_BIT
is a valid index for the iErrorCode array. In addition,
the MSDN page also documents WSAENETDOWN as the error code
that can be returned with the FD_READ event.

> > 2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
> > the order in which we check for FD_READ and FD_CLOSE is
> > actually important. Perhaps we should add a comment about
> > that.
> 
> This sounds like the real issue. It is totally fine to report multiple events
> simultaneously, and in that case we should pick the one we care the most about
> (if we don't go over all of them). In other words, we should invert the logic
> and test first for close events.

When I saw 0x21 in the debugger, the iErrorCode[FD_READ_BIT]
and iErrorCode[FD_CLOSE_BIT] were both zero (i.e., no errors).

I interpreted that (without verifying) as the peer sending
us some bytes and then closing the connection gracefully.
This is why I think we should read the bytes first, before
reporting the EOF (rv = 0) event to the caller.

If we report the FD_CLOSE (rv = 0) event first, I am worried
that we will fail to return some bytes to the caller. I'll
need to verify this with experiments.

Does this make sense?

[rvargas (doing something else), 2013-03-06 00:34:47]

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:1029: if
(network_events.iErrorCode[FD_READ_BIT]) {
On 2013/03/06 00:00:03, wtc wrote:
> 
> On 2013/03/05 23:14:07, rvargas wrote:
> >
> > could you point me to that sample?
> 
> I will bring the book to the office tomorrow.

That's ok. We agree that it is better to add the code anyway.

> 
> > My reading of the doc doesn't tell me that this path is needed. On the other
> > hand, I would not bet against being able to find a client configuration that
> > actually returns an error here... and adding more error handling is never
> wrong
> > :)
> 
> I agree.  The MSDN page for WSAEnumNetworkEvents
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741572%2528v=vs.85%...
> only mentions FD_READ_BIT and FD_WRITE_BIT without
> providing sample code, but I think that implies FD_READ_BIT
> is a valid index for the iErrorCode array. In addition,
> the MSDN page also documents WSAENETDOWN as the error code
> that can be returned with the FD_READ event.

I was looking at that page, but I'm reading it as no_error for a bunch of bits,
until FD_WRITE. Of course, a list of returned errors should not be interpreted
as exhaustive :)

The reasoning for my interpretation is that it doesn't make much sense to tell
an app "hey, there's available data for read... not really, there's an error so
you have nothing to read"

> 
> > > 2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
> > > the order in which we check for FD_READ and FD_CLOSE is
> > > actually important. Perhaps we should add a comment about
> > > that.
> > 
> > This sounds like the real issue. It is totally fine to report multiple
events
> > simultaneously, and in that case we should pick the one we care the most
about
> > (if we don't go over all of them). In other words, we should invert the
logic
> > and test first for close events.
> 
> When I saw 0x21 in the debugger, the iErrorCode[FD_READ_BIT]
> and iErrorCode[FD_CLOSE_BIT] were both zero (i.e., no errors).
> 
> I interpreted that (without verifying) as the peer sending
> us some bytes and then closing the connection gracefully.
> This is why I think we should read the bytes first, before
> reporting the EOF (rv = 0) event to the caller.
> 
> If we report the FD_CLOSE (rv = 0) event first, I am worried
> that we will fail to return some bytes to the caller. I'll
> need to verify this with experiments.
> 
> Does this make sense?

Yes... in that both bits should mean that. So assuming there is no error
anywhere, the right thing to do is to read the last few bytes. However, if there
is an error somewhere, the fact that we process the read first and don't look at
the other one means we are ignoring the close related error.

Maybe the right thing to do is to go through both paths and not use an else.

[Pat Meenan, 2013-03-06 02:05:28]

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_win.cc:1029: if
(network_events.iErrorCode[FD_READ_BIT]) {
On 2013/03/06 00:34:47, rvargas wrote:
> On 2013/03/06 00:00:03, wtc wrote:
> > 
> > On 2013/03/05 23:14:07, rvargas wrote:
> > >
> > > could you point me to that sample?
> > 
> > I will bring the book to the office tomorrow.
> 
> That's ok. We agree that it is better to add the code anyway.
> 
> > 
> > > My reading of the doc doesn't tell me that this path is needed. On the
other
> > > hand, I would not bet against being able to find a client configuration
that
> > > actually returns an error here... and adding more error handling is never
> > wrong
> > > :)
> > 
> > I agree.  The MSDN page for WSAEnumNetworkEvents
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741572%252528v=vs.8...
> > only mentions FD_READ_BIT and FD_WRITE_BIT without
> > providing sample code, but I think that implies FD_READ_BIT
> > is a valid index for the iErrorCode array. In addition,
> > the MSDN page also documents WSAENETDOWN as the error code
> > that can be returned with the FD_READ event.
> 
> I was looking at that page, but I'm reading it as no_error for a bunch of
bits,
> until FD_WRITE. Of course, a list of returned errors should not be interpreted
> as exhaustive :)
> 
> The reasoning for my interpretation is that it doesn't make much sense to tell
> an app "hey, there's available data for read... not really, there's an error
so
> you have nothing to read"
> 
> > 
> > > > 2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
> > > > the order in which we check for FD_READ and FD_CLOSE is
> > > > actually important. Perhaps we should add a comment about
> > > > that.
> > > 
> > > This sounds like the real issue. It is totally fine to report multiple
> events
> > > simultaneously, and in that case we should pick the one we care the most
> about
> > > (if we don't go over all of them). In other words, we should invert the
> logic
> > > and test first for close events.
> > 
> > When I saw 0x21 in the debugger, the iErrorCode[FD_READ_BIT]
> > and iErrorCode[FD_CLOSE_BIT] were both zero (i.e., no errors).
> > 
> > I interpreted that (without verifying) as the peer sending
> > us some bytes and then closing the connection gracefully.
> > This is why I think we should read the bytes first, before
> > reporting the EOF (rv = 0) event to the caller.
> > 
> > If we report the FD_CLOSE (rv = 0) event first, I am worried
> > that we will fail to return some bytes to the caller. I'll
> > need to verify this with experiments.
> > 
> > Does this make sense?
> 
> Yes... in that both bits should mean that. So assuming there is no error
> anywhere, the right thing to do is to read the last few bytes. However, if
there
> is an error somewhere, the fact that we process the read first and don't look
at
> the other one means we are ignoring the close related error.
> 
> Maybe the right thing to do is to go through both paths and not use an else.


In theory the FD_CLOSE is supposed to stay signaled and does not get reset:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx

I wasn't comfortable issuing 2 callbacks to a single read call so in the case of
both being signaled the read should complete, call back with the data and the
close would be indicated whenever another socket operation was attempted (likely
another read).

[rvargas (doing something else), 2013-03-06 03:26:51]

On 2013/03/06 02:05:28, pmeenan1 wrote:
>
https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
> File net/socket/tcp_client_socket_win.cc (right):
> 
>
https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_...
> net/socket/tcp_client_socket_win.cc:1029: if
> (network_events.iErrorCode[FD_READ_BIT]) {
> On 2013/03/06 00:34:47, rvargas wrote:
> > On 2013/03/06 00:00:03, wtc wrote:
> > > 
> > > On 2013/03/05 23:14:07, rvargas wrote:
> > > >
> > > > could you point me to that sample?
> > > 
> > > I will bring the book to the office tomorrow.
> > 
> > That's ok. We agree that it is better to add the code anyway.
> > 
> > > 
> > > > My reading of the doc doesn't tell me that this path is needed. On the
> other
> > > > hand, I would not bet against being able to find a client configuration
> that
> > > > actually returns an error here... and adding more error handling is
never
> > > wrong
> > > > :)
> > > 
> > > I agree.  The MSDN page for WSAEnumNetworkEvents
> > >
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741572%25252528v=vs...
> > > only mentions FD_READ_BIT and FD_WRITE_BIT without
> > > providing sample code, but I think that implies FD_READ_BIT
> > > is a valid index for the iErrorCode array. In addition,
> > > the MSDN page also documents WSAENETDOWN as the error code
> > > that can be returned with the FD_READ event.
> > 
> > I was looking at that page, but I'm reading it as no_error for a bunch of
> bits,
> > until FD_WRITE. Of course, a list of returned errors should not be
interpreted
> > as exhaustive :)
> > 
> > The reasoning for my interpretation is that it doesn't make much sense to
tell
> > an app "hey, there's available data for read... not really, there's an error
> so
> > you have nothing to read"
> > 
> > > 
> > > > > 2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
> > > > > the order in which we check for FD_READ and FD_CLOSE is
> > > > > actually important. Perhaps we should add a comment about
> > > > > that.
> > > > 
> > > > This sounds like the real issue. It is totally fine to report multiple
> > events
> > > > simultaneously, and in that case we should pick the one we care the most
> > about
> > > > (if we don't go over all of them). In other words, we should invert the
> > logic
> > > > and test first for close events.
> > > 
> > > When I saw 0x21 in the debugger, the iErrorCode[FD_READ_BIT]
> > > and iErrorCode[FD_CLOSE_BIT] were both zero (i.e., no errors).
> > > 
> > > I interpreted that (without verifying) as the peer sending
> > > us some bytes and then closing the connection gracefully.
> > > This is why I think we should read the bytes first, before
> > > reporting the EOF (rv = 0) event to the caller.
> > > 
> > > If we report the FD_CLOSE (rv = 0) event first, I am worried
> > > that we will fail to return some bytes to the caller. I'll
> > > need to verify this with experiments.
> > > 
> > > Does this make sense?
> > 
> > Yes... in that both bits should mean that. So assuming there is no error
> > anywhere, the right thing to do is to read the last few bytes. However, if
> there
> > is an error somewhere, the fact that we process the read first and don't
look
> at
> > the other one means we are ignoring the close related error.
> > 
> > Maybe the right thing to do is to go through both paths and not use an else.
> 
> 
> In theory the FD_CLOSE is supposed to stay signaled and does not get reset:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741576%28v=vs.85%29...
> 
> I wasn't comfortable issuing 2 callbacks to a single read call so in the case
of
> both being signaled the read should complete, call back with the data and the
> close would be indicated whenever another socket operation was attempted
(likely
> another read).

There's a difference between FD_CLOSE not being resettable by the application,
and the fact that all network notifications are cleared after being copied to
the user:
"The socket's internal record of network events is copied to the structure
referenced by lpNetworkEvents, after which the internal network events record is
cleared. If the hEventObject parameter is not NULL, the indicated event object
is also reset". Where are you reading that FD_CLOSE related data will survive
multiple calls to WSAEnumNetworkEvents?

[pmeenan, 2013-03-06 21:04:50]

Yes, sorry, I was relying on the following recv() (next call) to return an
error in that case and catching the error then.  It looks like FD_CLOSE can
also get signaled without an FD_READ and data could be available for
reading so wtc's other patch is probably going to deal with it all better
(need to look in more detail).


On Tue, Mar 5, 2013 at 10:26 PM, <rvargas@chromium.org> wrote:

> On 2013/03/06 02:05:28, pmeenan1 wrote:
>
> https://codereview.chromium.**org/12468002/diff/1/net/**
>
socket/tcp_client_socket_win.**cc<https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_win.cc>
>
>> File net/socket/tcp_client_socket_**win.cc (right):
>>
>
>
> https://codereview.chromium.**org/12468002/diff/1/net/**
>
socket/tcp_client_socket_win.**cc#newcode1029<https://codereview.chromium.org/12468002/diff/1/net/socket/tcp_client_socket_win.cc#newcode1029>
>
>> net/socket/tcp_client_socket_**win.cc:1029: if
>> (network_events.iErrorCode[FD_**READ_BIT]) {
>> On 2013/03/06 00:34:47, rvargas wrote:
>> > On 2013/03/06 00:00:03, wtc wrote:
>> > >
>> > > On 2013/03/05 23:14:07, rvargas wrote:
>> > > >
>> > > > could you point me to that sample?
>> > >
>> > > I will bring the book to the office tomorrow.
>> >
>> > That's ok. We agree that it is better to add the code anyway.
>> >
>> > >
>> > > > My reading of the doc doesn't tell me that this path is needed. On
>> the
>> other
>> > > > hand, I would not bet against being able to find a client
>> configuration
>> that
>> > > > actually returns an error here... and adding more error handling is
>>
> never
>
>> > > wrong
>> > > > :)
>> > >
>> > > I agree.  The MSDN page for WSAEnumNetworkEvents
>> > >
>> >
>>
>
> http://msdn.microsoft.com/en-**us/library/windows/desktop/**
>
ms741572%25252528v=vs.85%**25252529.aspx<http://msdn.microsoft.com/en-us/library/windows/desktop/ms741572%25252528v=vs.85%25252529.aspx>
>
>  > > only mentions FD_READ_BIT and FD_WRITE_BIT without
>> > > providing sample code, but I think that implies FD_READ_BIT
>> > > is a valid index for the iErrorCode array. In addition,
>> > > the MSDN page also documents WSAENETDOWN as the error code
>> > > that can be returned with the FD_READ event.
>> >
>> > I was looking at that page, but I'm reading it as no_error for a bunch
>> of
>> bits,
>> > until FD_WRITE. Of course, a list of returned errors should not be
>>
> interpreted
>
>> > as exhaustive :)
>> >
>> > The reasoning for my interpretation is that it doesn't make much sense
>> to
>>
> tell
>
>> > an app "hey, there's available data for read... not really, there's an
>> error
>> so
>> > you have nothing to read"
>> >
>> > >
>> > > > > 2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
>> > > > > the order in which we check for FD_READ and FD_CLOSE is
>> > > > > actually important. Perhaps we should add a comment about
>> > > > > that.
>> > > >
>> > > > This sounds like the real issue. It is totally fine to report
>> multiple
>> > events
>> > > > simultaneously, and in that case we should pick the one we care the
>> most
>> > about
>> > > > (if we don't go over all of them). In other words, we should invert
>> the
>> > logic
>> > > > and test first for close events.
>> > >
>> > > When I saw 0x21 in the debugger, the iErrorCode[FD_READ_BIT]
>> > > and iErrorCode[FD_CLOSE_BIT] were both zero (i.e., no errors).
>> > >
>> > > I interpreted that (without verifying) as the peer sending
>> > > us some bytes and then closing the connection gracefully.
>> > > This is why I think we should read the bytes first, before
>> > > reporting the EOF (rv = 0) event to the caller.
>> > >
>> > > If we report the FD_CLOSE (rv = 0) event first, I am worried
>> > > that we will fail to return some bytes to the caller. I'll
>> > > need to verify this with experiments.
>> > >
>> > > Does this make sense?
>> >
>> > Yes... in that both bits should mean that. So assuming there is no error
>> > anywhere, the right thing to do is to read the last few bytes. However,
>> if
>> there
>> > is an error somewhere, the fact that we process the read first and don't
>>
> look
>
>> at
>> > the other one means we are ignoring the close related error.
>> >
>> > Maybe the right thing to do is to go through both paths and not use an
>> else.
>>
>
>
>  In theory the FD_CLOSE is supposed to stay signaled and does not get
>> reset:
>>
>
> http://msdn.microsoft.com/en-**us/library/windows/desktop/**
>
ms741576%28v=vs.85%29.aspx<http://msdn.microsoft.com/en-us/library/windows/desktop/ms741576%28v=vs.85%29.aspx>
>
>
>  I wasn't comfortable issuing 2 callbacks to a single read call so in the
>> case
>>
> of
>
>> both being signaled the read should complete, call back with the data and
>> the
>> close would be indicated whenever another socket operation was attempted
>>
> (likely
>
>> another read).
>>
>
> There's a difference between FD_CLOSE not being resettable by the
> application,
> and the fact that all network notifications are cleared after being copied
> to
> the user:
> "The socket's internal record of network events is copied to the structure
> referenced by lpNetworkEvents, after which the internal network events
> record is
> cleared. If the hEventObject parameter is not NULL, the indicated event
> object
> is also reset". Where are you reading that FD_CLOSE related data will
> survive
> multiple calls to WSAEnumNetworkEvents?
>
>
https://codereview.chromium.**org/12468002/<https://codereview.chromium.org/1...
>

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

Please review patch set 2, which is the new approach that I
showed to you in https://codereview.chromium.org/12546004/
originally.

agl: I included the revert of your CL to make it clear that
if my change to tcp_client_socket_win.cc ever needs to be
reverted, we need to add back your CL.

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1037: rv = 0;

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 can't speak to the TLS fallback reversion but if you remove the special case
on the FD_CLOSE skipping the read then it looks good to me.

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1037: rv = 0;
On 2013/03/14 01:57:24, wtc wrote:
> 
> 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?

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.

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1043: // reset.
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]

Please review patch set 4. I made the changes pmeenan suggested.

The relevant text in the WSAEventSelect MSDN page is garbled.
The sentence "FD_CLOSE being posted after all data is read
from a socket." looks incomplete.  So I quoted the text from
the WSAAsyncSelect MSDN page instead.

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1037: rv = 0;

On 2013/03/14 13:46:47, pmeenan1 wrote:
>
> 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.

Done.

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1043: // reset.

On 2013/03/14 13:46:47, pmeenan1 wrote:
> 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.

Done.

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

lgtm

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1037: rv = 0;
On 2013/03/14 01:57:24, wtc wrote:
> 
> 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?

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.

https://codereview.chromium.org/12468002/diff/16001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1052: // Rather than trying to reset the
event object properly, it seems
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.

[Pat Meenan, 2013-03-14 19:31:49]

lgtm

[commit-bot: I haz the power, 2013-03-14 20:09:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/12468002/39001

[commit-bot: I haz the power, 2013-03-15 07:18:04]

Change committed as 188278