win: Only retry in UseHandler() loop on ERROR_PIPE_BUSY (issue 1409693011 by scottmg@chromium.org)

[sco...@chromium.org]

Reviewers: Mark Mentovai
CL: https://codereview.chromium.org/1409693011/

Description:
win: Only retry in UseHandler() loop on ERROR_PIPE_BUSY

This is better because now end_to_end_test.py fails immediately with

[1180:9020:20151106,145204.830:ERROR registration_protocol_win.cc:39]
CreateFile: The system cannot find the file specified. (0x2)

R=ma...@chromium.org
BUG=crashpad:75

Base URL: https://chromium.googlesource.com/crashpad/crashpad@master

Affected files (+9, -1 lines):
M util/win/registration_protocol_win.cc


Index: util/win/registration_protocol_win.cc
diff --git a/util/win/registration_protocol_win.cc
b/util/win/registration_protocol_win.cc
index
3e0fdf213985a4bc81ae295bd3377140463045da..09c7f941e6146332e6ebd3ad7d520c2007bda6f8
100644
--- a/util/win/registration_protocol_win.cc
+++ b/util/win/registration_protocol_win.cc
@@ -35,7 +35,15 @@ bool SendToCrashHandlerServer(const base::string16&
pipe_name,
SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION,
nullptr));
if (!pipe.is_valid()) {
- Sleep(10);
+ if (GetLastError() != ERROR_PIPE_BUSY) {
+ PLOG(ERROR) << "CreateFile";
+ return false;
+ }
+
+ if (!WaitNamedPipe(pipe_name.c_str(), 1000)) {
+ PLOG(ERROR) << "WaitNamedPipe";
+ return false;
+ }
--tries;
continue;
}

[ma...@chromium.org]

It looks like you and I collided, but I think our changes can complement
each
other.


https://codereview.chromium.org/1409693011/diff/1/util/win/registration_protocol_win.cc
File util/win/registration_protocol_win.cc (right):

https://codereview.chromium.org/1409693011/diff/1/util/win/registration_protocol_win.cc#newcode43
util/win/registration_protocol_win.cc:43: if
(!WaitNamedPipe(pipe_name.c_str(), 1000)) {
We don’t want to do this at all on the last try.

https://codereview.chromium.org/1409693011/diff/1/util/win/registration_protocol_win.cc#newcode44
util/win/registration_protocol_win.cc:44: PLOG(ERROR) <<
"WaitNamedPipe";
If GetLastError() is ERROR_SEM_TIMEOUT, we should probably keep
retrying.

https://codereview.chromium.org/1409693011/

[sco...@chromium.org]

How about like this?

https://codereview.chromium.org/1409693011/diff/1/util/win/registration_protocol_win.cc
File util/win/registration_protocol_win.cc (right):

https://codereview.chromium.org/1409693011/diff/1/util/win/registration_protocol_win.cc#newcode43
util/win/registration_protocol_win.cc:43: if
(!WaitNamedPipe(pipe_name.c_str(), 1000)) {

On 2015/11/06 23:03:34, Mark Mentovai wrote:
> We don’t want to do this at all on the last try.

Done.

https://codereview.chromium.org/1409693011/diff/1/util/win/registration_protocol_win.cc#newcode44
util/win/registration_protocol_win.cc:44: PLOG(ERROR) <<
"WaitNamedPipe";

On 2015/11/06 23:03:34, Mark Mentovai wrote:
> If GetLastError() is ERROR_SEM_TIMEOUT, we should probably keep
retrying.

Done.

https://codereview.chromium.org/1409693011/

[ma...@chromium.org]

LGTM


https://codereview.chromium.org/1409693011/diff/40001/snapshot/win/end_to_end_test.py
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1409693011/diff/40001/snapshot/win/end_to_end_test.py#newcode107
snapshot/win/end_to_end_test.py:107: print 'Waiting for crashpad_handler
to be ready...'
Only print this the first time. If crashpad_handler never gets ready,
this will spam output.

https://codereview.chromium.org/1409693011/diff/40001/util/win/registration_protocol_win.cc
File util/win/registration_protocol_win.cc (right):

https://codereview.chromium.org/1409693011/diff/40001/util/win/registration_protocol_win.cc#newcode38
util/win/registration_protocol_win.cc:38: if (tries++ == 5 ||
GetLastError() != ERROR_PIPE_BUSY) {
We want ++tries to be equivalent to the previous behavior.

https://codereview.chromium.org/1409693011/diff/40001/util/win/registration_protocol_win.cc#newcode72
util/win/registration_protocol_win.cc:72: LOG(ERROR) <<
"TransactNamedPipe read incorrect number of bytes";
Can you grab the updated LOG() line from
https://codereview.chromium.org/1415453012/, which is otherwise
obsoleted by this change?

https://codereview.chromium.org/1409693011/

[sco...@chromium.org]

https://codereview.chromium.org/1409693011/diff/40001/snapshot/win/end_to_end_test.py
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1409693011/diff/40001/snapshot/win/end_to_end_test.py#newcode107
snapshot/win/end_to_end_test.py:107: print 'Waiting for crashpad_handler
to be ready...'

On 2015/11/06 23:32:35, Mark Mentovai wrote:
> Only print this the first time. If crashpad_handler never gets ready,
this will
> spam output.

Done.

https://codereview.chromium.org/1409693011/diff/40001/util/win/registration_protocol_win.cc
File util/win/registration_protocol_win.cc (right):

https://codereview.chromium.org/1409693011/diff/40001/util/win/registration_protocol_win.cc#newcode38
util/win/registration_protocol_win.cc:38: if (tries++ == 5 ||
GetLastError() != ERROR_PIPE_BUSY) {

On 2015/11/06 23:32:35, Mark Mentovai wrote:
> We want ++tries to be equivalent to the previous behavior.

Done.

https://codereview.chromium.org/1409693011/diff/40001/util/win/registration_protocol_win.cc#newcode72
util/win/registration_protocol_win.cc:72: LOG(ERROR) <<
"TransactNamedPipe read incorrect number of bytes";

On 2015/11/06 23:32:35, Mark Mentovai wrote:
> Can you grab the updated LOG() line from
> https://codereview.chromium.org/1415453012/, which is otherwise
obsoleted by
> this change?

Done.

https://codereview.chromium.org/1409693011/

[scottmg@chromium.org via codereview.chromium.org]

Committed patchset #4 (id:60001) manually as
ff274507dc30cb3b4273a04cd73f3ab5240d02e7 (presubmit successful).

https://codereview.chromium.org/1409693011/