Call WSAGetLastError immediately after the potentially failing call.

Call WSAGetLastError immediately after the potentially failing call.

BUG=669593
Committed: https://crrev.com/6c48fe45a7573eef954f031a0ce4e42264427761
Cr-Commit-Position: refs/heads/master@{#435080}

[Sigurður Ásgeirsson, 2016-11-29 18:54:49]

The CQ bit was checked by siggi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 18:55:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2016-11-29 19:10:05]

siggi@chromium.org changed reviewers:
+ eroman@chromium.org

[Sigurður Ásgeirsson, 2016-11-29 19:10:06]

Hey Eric,

The heap allocator instrumentation I'm adding to tracked_objects::ScopedTracker
uses TLS, which seems to occasionally stomp the per-thread last error. I've
changed every call to WSAGetLastError to explicitly load the value into a local
variable, and be as close to the potentially failing call as I could make it.
IMHO the use of base::SetNonBlocking is also brittle to maintenance, though I
haven't touched that.

Siggi

[commit-bot: I haz the power, 2016-11-29 19:50:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 19:50:06]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-11-29 19:51:00]

LGTM

Is there any performance implication to calling WSAGetLastError()
unconditionally? (I don't expect there would be).

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.cc
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:52: int os_error = WSAGetLastError();
For these two cases, does the re-ordering make a difference, or is it purely for
consistency? I am OK with this either way

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:282: int os_error = WSAGetLastError();
Same issue as for base::SetNonBlocking() -- relies on CreatePlatformSocket() not
clobbering the last error.

A reasonable follow-up could be to add an "int* os_error" out-parameter to
CreatePlatformSocket().

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:307: int os_error = WSAGetLastError();
Given this is called 3 times, I would suggest adding a file-local wrapper
function, say
    bool SetNonBlocking(..., int* os_error);

Similar to CreatePlatformSocket(), could push this interface to the base
implementation too.

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:947: int os_error = WSAGetLastError();
Based on the conditional where this is used, I think it should be called
immediately after WSAGetOverlappedResult().

[Sigurður Ásgeirsson, 2016-11-29 20:23:12]

The CQ bit was checked by siggi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 20:23:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2016-11-29 20:27:55]

Thanks - I don't think there should be any performance hit due to this. This is
merely accessing a TLS slot. The memory will be cache-hot for sure, as pretty
much every Win32 function reads and/or sets the dang GLE value.

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.cc
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:52: int os_error = WSAGetLastError();
On 2016/11/29 19:51:00, eroman (slow) wrote:
> For these two cases, does the re-ordering make a difference, or is it purely
for
> consistency? I am OK with this either way

This is purely for consistency. Note that C++ can be a trifling implicit, so in
the unlikely case if e.g. MapSystemError in the future starts constructing
temporaries around the argument, that temporary could smite your last error
value.

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:282: int os_error = WSAGetLastError();
On 2016/11/29 19:51:00, eroman (slow) wrote:
> Same issue as for base::SetNonBlocking() -- relies on CreatePlatformSocket()
not
> clobbering the last error.
> 
> A reasonable follow-up could be to add an "int* os_error" out-parameter to
> CreatePlatformSocket().

I don't know whether this works the same on other platforms, and in any case I'm
mostly concerned with fixing the one real problem(TM) here to get my CL landed.

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:307: int os_error = WSAGetLastError();
On 2016/11/29 19:51:00, eroman (slow) wrote:
> Given this is called 3 times, I would suggest adding a file-local wrapper
> function, say
>     bool SetNonBlocking(..., int* os_error);
> 
> Similar to CreatePlatformSocket(), could push this interface to the base
> implementation too.

Done.

https://codereview.chromium.org/2539833003/diff/1/net/socket/tcp_socket_win.c...
net/socket/tcp_socket_win.cc:947: int os_error = WSAGetLastError();
On 2016/11/29 19:51:00, eroman (slow) wrote:
> Based on the conditional where this is used, I think it should be called
> immediately after WSAGetOverlappedResult().

Done, though this probably does change semantics in the unlikely case where
WSAResetEvent fails. I don't know whether that's better or worse, as
WSAResetEvent is unlikely to fail except on handle abuse or programming error...

[Sigurður Ásgeirsson, 2016-11-29 20:29:37]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2016-11-29 20:29:37]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2539833003/#ps40001
(title: "No global namespace ::WSAGetLastError for consistency with rest of
file.")

[commit-bot: I haz the power, 2016-11-29 20:30:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 21:34:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480451377657750,
"parent_rev": "30b2f01fabead5bbbc32bd72086780bf8b300058", "commit_rev":
"4328079461185093bb0e2f229849bf4674de7bec"}

[commit-bot: I haz the power, 2016-11-29 21:34:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-29 21:38:04]

Description was changed from

==========
Call WSAGetLastError immediately after the potentially failing call.

BUG=669593
==========

to

==========
Call WSAGetLastError immediately after the potentially failing call.

BUG=669593

Committed: https://crrev.com/6c48fe45a7573eef954f031a0ce4e42264427761
Cr-Commit-Position: refs/heads/master@{#435080}
==========

[commit-bot: I haz the power, 2016-11-29 21:38:04]

Patchset 3 (id:??) landed as
https://crrev.com/6c48fe45a7573eef954f031a0ce4e42264427761
Cr-Commit-Position: refs/heads/master@{#435080}