Patchwork debugwireproto: handle unimplemented util.poll() for Windows

login
register
mail settings
Submitter Matt Harbison
Date March 6, 2018, 1:48 a.m.
Message ID <7a25f6cfebe80802321d.1520300909@Envy>
Download mbox | patch
Permalink /patch/29054/
State Accepted
Headers show

Comments

Matt Harbison - March 6, 2018, 1:48 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1520299354 18000
#      Mon Mar 05 20:22:34 2018 -0500
# Node ID 7a25f6cfebe80802321d2975b97fc15ec38cf8ec
# Parent  2aff6daf779098eee4c350ccd0197dcc2231e197
debugwireproto: handle unimplemented util.poll() for Windows

This is the same logic used in sshpeer.doublepipe.  It doesn't completely fix
test-ssh-proto{,-unbundle}.t ("read(-1) -> X" is changed to "read(X) -> X", the
order of some lines are changed, and abort messages seem to be missing), but it
cuts down a ton on the failure spew.
Gregory Szorc - March 6, 2018, 1:58 a.m.
On Mon, Mar 5, 2018 at 5:48 PM, Matt Harbison <mharbison72@gmail.com> wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1520299354 18000
> #      Mon Mar 05 20:22:34 2018 -0500
> # Node ID 7a25f6cfebe80802321d2975b97fc15ec38cf8ec
> # Parent  2aff6daf779098eee4c350ccd0197dcc2231e197
> debugwireproto: handle unimplemented util.poll() for Windows
>
> This is the same logic used in sshpeer.doublepipe.  It doesn't completely
> fix
> test-ssh-proto{,-unbundle}.t ("read(-1) -> X" is changed to "read(X) ->
> X", the
> order of some lines are changed, and abort messages seem to be missing),
> but it
> cuts down a ton on the failure spew.
>

Queued.

readavailable is intrinsically non-deterministic if there could be data on
both pipes. I think we should rewrite the tests to not use readavailable.

This will likely require new commands in the metalanguage to read specific
amounts from specific pipes. That's probably the only way we make things
deterministic across platforms and various load conditions. I can author
those patches if you want, since I'm actively working in this area and I
did introduce the problem :)


>
> diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
> --- a/mercurial/debugcommands.py
> +++ b/mercurial/debugcommands.py
> @@ -2818,11 +2818,16 @@
>          elif action == 'close':
>              peer.close()
>          elif action == 'readavailable':
> -            fds = util.poll([stdout.fileno(), stderr.fileno()])
> -
> -            if stdout.fileno() in fds:
> +            fds = [stdout.fileno(), stderr.fileno()]
> +            try:
> +                act = util.poll(fds)
> +            except NotImplementedError:
> +                # non supported yet case, assume all have data.
> +                act = fds
> +
> +            if stdout.fileno() in act:
>                  util.readpipe(stdout)
> -            if stderr.fileno() in fds:
> +            if stderr.fileno() in act:
>                  util.readpipe(stderr)
>          elif action == 'readline':
>              stdout.readline()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Matt Harbison - March 6, 2018, 2:08 a.m.
On Mon, 05 Mar 2018 20:58:23 -0500, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> On Mon, Mar 5, 2018 at 5:48 PM, Matt Harbison <mharbison72@gmail.com>  
> wrote:
>
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1520299354 18000
>> #      Mon Mar 05 20:22:34 2018 -0500
>> # Node ID 7a25f6cfebe80802321d2975b97fc15ec38cf8ec
>> # Parent  2aff6daf779098eee4c350ccd0197dcc2231e197
>> debugwireproto: handle unimplemented util.poll() for Windows
>>
>> This is the same logic used in sshpeer.doublepipe.  It doesn't  
>> completely
>> fix
>> test-ssh-proto{,-unbundle}.t ("read(-1) -> X" is changed to "read(X) ->
>> X", the
>> order of some lines are changed, and abort messages seem to be missing),
>> but it
>> cuts down a ton on the failure spew.
>>
>
> Queued.
>
> readavailable is intrinsically non-deterministic if there could be data  
> on
> both pipes. I think we should rewrite the tests to not use readavailable.
>
> This will likely require new commands in the metalanguage to read  
> specific
> amounts from specific pipes. That's probably the only way we make things
> deterministic across platforms and various load conditions. I can author
> those patches if you want, since I'm actively working in this area and I
> did introduce the problem :)

Yes please :)  I can test if you don't have easy access to Windows.

I'd like to get back to the lfs serving work (did you have any comments on  
V2?), but didn't want the decaying Windows tests to be hiding problems.  I  
looked at implementing util.poll(), but I don't think it's possible, at  
least without threads.  (WaitForMultipleObjects() doesn't take all types  
of handle.)
Gregory Szorc - March 6, 2018, 2:15 a.m.
On Mon, Mar 5, 2018 at 6:08 PM, Matt Harbison <mharbison72@gmail.com> wrote:

> On Mon, 05 Mar 2018 20:58:23 -0500, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
>
> On Mon, Mar 5, 2018 at 5:48 PM, Matt Harbison <mharbison72@gmail.com>
>> wrote:
>>
>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1520299354 18000
>>> #      Mon Mar 05 20:22:34 2018 -0500
>>> # Node ID 7a25f6cfebe80802321d2975b97fc15ec38cf8ec
>>> # Parent  2aff6daf779098eee4c350ccd0197dcc2231e197
>>> debugwireproto: handle unimplemented util.poll() for Windows
>>>
>>> This is the same logic used in sshpeer.doublepipe.  It doesn't completely
>>> fix
>>> test-ssh-proto{,-unbundle}.t ("read(-1) -> X" is changed to "read(X) ->
>>> X", the
>>> order of some lines are changed, and abort messages seem to be missing),
>>> but it
>>> cuts down a ton on the failure spew.
>>>
>>>
>> Queued.
>>
>> readavailable is intrinsically non-deterministic if there could be data on
>> both pipes. I think we should rewrite the tests to not use readavailable.
>>
>> This will likely require new commands in the metalanguage to read specific
>> amounts from specific pipes. That's probably the only way we make things
>> deterministic across platforms and various load conditions. I can author
>> those patches if you want, since I'm actively working in this area and I
>> did introduce the problem :)
>>
>
> Yes please :)  I can test if you don't have easy access to Windows.
>
> I'd like to get back to the lfs serving work (did you have any comments on
> V2?), but didn't want the decaying Windows tests to be hiding problems.  I
> looked at implementing util.poll(), but I don't think it's possible, at
> least without threads.  (WaitForMultipleObjects() doesn't take all types of
> handle.)
>

Sorry - I'm a bit behind on code review for things that weren't actively
discussed at the sprint.

We only use util.poll() in the ssh peer. I think it is safe to say that
handles will be pipes (whatever os.pipe creates) or process stdio handles.
Hopefully that makes things easier.
Matt Harbison - March 6, 2018, 2:24 a.m.
On Mon, 05 Mar 2018 21:15:30 -0500, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> On Mon, Mar 5, 2018 at 6:08 PM, Matt Harbison <mharbison72@gmail.com>  
> wrote:
>
>> On Mon, 05 Mar 2018 20:58:23 -0500, Gregory Szorc  
>> <gregory.szorc@gmail.com>
>> wrote:
>>
>> On Mon, Mar 5, 2018 at 5:48 PM, Matt Harbison <mharbison72@gmail.com>
>>> wrote:
>>>
>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1520299354 18000
>>>> #      Mon Mar 05 20:22:34 2018 -0500
>>>> # Node ID 7a25f6cfebe80802321d2975b97fc15ec38cf8ec
>>>> # Parent  2aff6daf779098eee4c350ccd0197dcc2231e197
>>>> debugwireproto: handle unimplemented util.poll() for Windows
>>>>
>>>> This is the same logic used in sshpeer.doublepipe.  It doesn't  
>>>> completely
>>>> fix
>>>> test-ssh-proto{,-unbundle}.t ("read(-1) -> X" is changed to "read(X)  
>>>> ->
>>>> X", the
>>>> order of some lines are changed, and abort messages seem to be  
>>>> missing),
>>>> but it
>>>> cuts down a ton on the failure spew.
>>>>
>>>>
>>> Queued.
>>>
>>> readavailable is intrinsically non-deterministic if there could be  
>>> data on
>>> both pipes. I think we should rewrite the tests to not use  
>>> readavailable.
>>>
>>> This will likely require new commands in the metalanguage to read  
>>> specific
>>> amounts from specific pipes. That's probably the only way we make  
>>> things
>>> deterministic across platforms and various load conditions. I can  
>>> author
>>> those patches if you want, since I'm actively working in this area and  
>>> I
>>> did introduce the problem :)
>>>
>>
>> Yes please :)  I can test if you don't have easy access to Windows.
>>
>> I'd like to get back to the lfs serving work (did you have any comments  
>> on
>> V2?), but didn't want the decaying Windows tests to be hiding  
>> problems.  I
>> looked at implementing util.poll(), but I don't think it's possible, at
>> least without threads.  (WaitForMultipleObjects() doesn't take all  
>> types of
>> handle.)
>>
>
> Sorry - I'm a bit behind on code review for things that weren't actively
> discussed at the sprint.
>
> We only use util.poll() in the ssh peer. I think it is safe to say that
> handles will be pipes (whatever os.pipe creates) or process stdio  
> handles.
> Hopefully that makes things easier.

Sadly, neither of those are in the list[1].  I think I did get a 0 (first  
object signaled) return when I tried it, but I don't want to rely on  
something undocumented when we can just peek() on the pipes.

[1]  
https://msdn.microsoft.com/en-us/library/windows/desktop/ms687025(v=vs.85).aspx

Patch

diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -2818,11 +2818,16 @@ 
         elif action == 'close':
             peer.close()
         elif action == 'readavailable':
-            fds = util.poll([stdout.fileno(), stderr.fileno()])
-
-            if stdout.fileno() in fds:
+            fds = [stdout.fileno(), stderr.fileno()]
+            try:
+                act = util.poll(fds)
+            except NotImplementedError:
+                # non supported yet case, assume all have data.
+                act = fds
+
+            if stdout.fileno() in act:
                 util.readpipe(stdout)
-            if stderr.fileno() in fds:
+            if stderr.fileno() in act:
                 util.readpipe(stderr)
         elif action == 'readline':
             stdout.readline()