[PATCH 3 of 3] windows: convert readpipe() to nonblocking like 331cbf088c4c does for posix

[Pierre-Yves David]

On 04/07/2015 08:56 AM, Yuya Nishihara wrote:
> On Mon, 06 Apr 2015 22:49:02 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison at yahoo.com>
>> # Date 1428373243 14400
>> #      Mon Apr 06 22:20:43 2015 -0400
>> # Node ID 7a8e37db057fb3b1fb77ee08df83076e1a527af7
>> # Parent  5ad3c1f7fa75a06307f430ee51c19d85e6c78d14
>> windows: convert readpipe() to nonblocking like 331cbf088c4c does for posix
>>
>> There are several places in the Windows SSH tests where the order of local
>> output vs remote output differ from the other platforms.  This only fixes one of
>> those cases (and interstingly, not the one added in order to test 331cbf088c4c),
>> so there is more investigation needed.  However, without this patch, test-ssl.t
>> also has this diff:
>>
>>      --- c:/Users/Matt/Projects/hg/tests/test-ssh.t
>>      +++ c:/Users/Matt/Projects/hg/tests/test-ssh.t.err
>>      @@ -397,11 +397,11 @@
>>         $ hg push --ssh "sh ../ssh.sh"
>>         pushing to ssh://user@dummy/*/remote (glob)
>>         searching for changes
>>      -  remote: Permission denied
>>      -  remote: abort: prechangegroup.hg-ssh hook failed
>>      -  remote: Permission denied
>>      -  remote: pushkey-abort: prepushkey.hg-ssh hook failed
>>         updating 6c0482d977a3 to public failed!
>>      +  remote: Permission denied
>>      +  remote: abort: prechangegroup.hg-ssh hook failed
>>      +  remote: Permission denied
>>      +  remote: pushkey-abort: prepushkey.hg-ssh hook failed
>>         [1]
>>
>>         $ cd ..
>>
>> Output with this change was stable over 700+ runs of test-ssl.t.  I initially
>> tried a background thread to read the pipe[1], but this was simpler and the test
>> results were exactly the same.
>>
>> [1] http://eyalarubas.com/python-subproc-nonblock.html
>>
>> diff --git a/mercurial/win32.py b/mercurial/win32.py
>> --- a/mercurial/win32.py
>> +++ b/mercurial/win32.py
>> @@ -243,6 +243,8 @@
>>       finally:
>>           _kernel32.CloseHandle(fh)
>>
>> +PIPE_NOWAIT = 1
>> +
>>   def getpipestate(pipe):
>>       handle = msvcrt.get_osfhandle(pipe.fileno())
>>       if handle == -1:
>> diff --git a/mercurial/windows.py b/mercurial/windows.py
>> --- a/mercurial/windows.py
>> +++ b/mercurial/windows.py
>> @@ -362,14 +362,19 @@
>>   def readpipe(pipe):
>>       """Read all available data from a pipe."""
>>       chunks = []
>> -    while True:
>> -        size = os.fstat(pipe.fileno()).st_size
>> -        if not size:
>> -            break
>> +    oldstate = win32.getpipestate(pipe)
>> +    win32.setpipestate(pipe, win32.PIPE_NOWAIT)
>
> The doc says "Note that nonblocking mode [...] should not be used to achieve
> asynchronous input and output (I/O) with named pipes."
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365787%28v=vs.85%29.aspx
>
> IIRC, PeekNamedPipe(h, NULL, 0, NULL, &availsize, NULL) can be used to get
> the available size without blocking.
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365779(v=vs.85).aspx
>
> "The function always returns immediately in a single-threaded application, even
> if there is no data in the pipe. The wait mode of a named pipe handle (blocking
> or nonblocking) has no effect on the function."

Does this mean we should drop this three patches from patchwork and 
expect a V2? Or does the patches looks good the yuya and we should queue 
them?

-- 
Pierre-Yves David