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

login
register
mail settings
Submitter Matt Harbison
Date April 7, 2015, 2:49 a.m.
Message ID <7a8e37db057fb3b1fb77.1428374942@Envy>
Download mbox | patch
Permalink /patch/8526/
State Superseded
Headers show

Comments

Matt Harbison - April 7, 2015, 2:49 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@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
Yuya Nishihara - April 7, 2015, 12:56 p.m.
On Mon, 06 Apr 2015 22:49:02 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@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."

Regards,
Pierre-Yves David - April 14, 2015, 7:48 p.m.
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@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?
Matt Harbison - April 14, 2015, 7:56 p.m.

Matt Harbison - April 14, 2015, 7:59 p.m.

Patch

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)
 
-        s = pipe.read(size)
-        if not s:
-            break
-        chunks.append(s)
+    try:
+        while True:
+            try:
+                s = pipe.read()
+                if not s:
+                    break
+                chunks.append(s)
+            except IOError:
+                break
 
-    return ''.join(chunks)
+        return ''.join(chunks)
+    finally:
+        win32.setpipestate(pipe, oldstate)