Patchwork [1,of,3] debugwireproto: close the write end before consuming all available data

login
register
mail settings
Submitter Yuya Nishihara
Date March 12, 2018, 2:17 p.m.
Message ID <2e2a5376d006ad77ba9a.1520864262@mimosa>
Download mbox | patch
Permalink /patch/29312/
State Accepted
Headers show

Comments

Yuya Nishihara - March 12, 2018, 2:17 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1520862453 -32400
#      Mon Mar 12 22:47:33 2018 +0900
# Node ID 2e2a5376d006ad77ba9a07d341f6bc5418289af1
# Parent  ff541b8cdee0cf9b75874639388bdc8b9854ac20
debugwireproto: close the write end before consuming all available data

And make it read all available data deterministically. Otherwise util.poll()
may deadlock because both stdout and stderr could have no data.

Spotted by the next patch which removes stderr from the fds.
Gregory Szorc - March 12, 2018, 6:09 p.m.
On Mon, Mar 12, 2018 at 7:17 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1520862453 -32400
> #      Mon Mar 12 22:47:33 2018 +0900
> # Node ID 2e2a5376d006ad77ba9a07d341f6bc5418289af1
> # Parent  ff541b8cdee0cf9b75874639388bdc8b9854ac20
> debugwireproto: close the write end before consuming all available data
>

I queued this one.


>
> And make it read all available data deterministically. Otherwise
> util.poll()
> may deadlock because both stdout and stderr could have no data.
>
> Spotted by the next patch which removes stderr from the fds.
>
> diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
> --- a/mercurial/debugcommands.py
> +++ b/mercurial/debugcommands.py
> @@ -2690,7 +2690,8 @@ def debugwireproto(ui, repo, **opts):
>      readavailable
>      -------------
>
> -    Read all available data from the server.
> +    Close the write end of the connection and read all available data from
> +    the server.
>
>      If the connection to the server encompasses multiple pipes, we poll
> both
>      pipes and read available data.
> @@ -2835,17 +2836,9 @@ def debugwireproto(ui, repo, **opts):
>          elif action == 'close':
>              peer.close()
>          elif action == 'readavailable':
> -            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 act:
> -                util.readpipe(stderr)
> +            stdin.close()
> +            stdout.read()
> +            stderr.read()
>          elif action == 'readline':
>              stdout.readline()
>          else:
> diff --git a/tests/test-ssh-proto-unbundle.t b/tests/test-ssh-proto-
> unbundle.t
> --- a/tests/test-ssh-proto-unbundle.t
> +++ b/tests/test-ssh-proto-unbundle.t
> @@ -93,6 +93,7 @@ Test pushing bundle1 payload to a server
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 115:
>    e>     abort: incompatible Mercurial client; bundle2 required\n
>    e>     (see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n
> @@ -143,6 +144,7 @@ Test pushing bundle1 payload to a server
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 115:
>    e>     abort: incompatible Mercurial client; bundle2 required\n
>    e>     (see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n
> @@ -260,6 +262,7 @@ ui.write() in hook is redirected to stde
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 196:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -316,6 +319,7 @@ ui.write() in hook is redirected to stde
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 196:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -386,6 +390,7 @@ And a variation that writes multiple lin
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 218:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -443,6 +448,7 @@ And a variation that writes multiple lin
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 218:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -514,6 +520,7 @@ And a variation that does a ui.flush() a
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 202:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -570,6 +577,7 @@ And a variation that does a ui.flush() a
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 202:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -640,6 +648,7 @@ Multiple writes + flush
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 206:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -697,6 +706,7 @@ Multiple writes + flush
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 206:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -768,6 +778,7 @@ ui.write() + ui.write_err() output is ca
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 232:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -827,6 +838,7 @@ ui.write() + ui.write_err() output is ca
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 232:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -900,6 +912,7 @@ print() output is captured
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 193:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -956,6 +969,7 @@ print() output is captured
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 193:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1026,6 +1040,7 @@ Mixed print() and ui.write() are both ca
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 218:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1085,6 +1100,7 @@ Mixed print() and ui.write() are both ca
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 218:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1158,6 +1174,7 @@ print() to stdout and stderr both get ca
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 216:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1217,6 +1234,7 @@ print() to stdout and stderr both get ca
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 216:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1296,6 +1314,7 @@ Shell hook writing to stdout has output
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 212:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1353,6 +1372,7 @@ Shell hook writing to stdout has output
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 212:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1425,6 +1445,7 @@ Shell hook writing to stderr has output
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 212:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1482,6 +1503,7 @@ Shell hook writing to stderr has output
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 212:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1556,6 +1578,7 @@ Shell hook writing to stdout and stderr
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 230:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1615,6 +1638,7 @@ Shell hook writing to stdout and stderr
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 230:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1697,6 +1721,7 @@ Shell and Python hooks writing to stdout
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 273:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1760,6 +1785,7 @@ Shell and Python hooks writing to stdout
>    o> read(1) -> 1: 0
>    result: 0
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 273:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1837,6 +1863,7 @@ Pushing a bundle1 with no output
>    o> read(1) -> 1: 1
>    result: 1
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 100:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1889,6 +1916,7 @@ Pushing a bundle1 with no output
>    o> read(1) -> 1: 1
>    result: 1
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 100:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -1967,6 +1995,7 @@ Pushing a bundle1 with ui.write() and ui
>    o> read(1) -> 1: 1
>    result: 1
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 152:
>    e>     adding changesets\n
>    e>     adding manifests\n
> @@ -2023,6 +2052,7 @@ Pushing a bundle1 with ui.write() and ui
>    o> read(1) -> 1: 1
>    result: 1
>    remote output:
> +  o> read(-1) -> 0:
>    e> read(-1) -> 152:
>    e>     adding changesets\n
>    e>     adding manifests\n
> diff --git a/tests/test-ssh-proto.t b/tests/test-ssh-proto.t
> --- a/tests/test-ssh-proto.t
> +++ b/tests/test-ssh-proto.t
> @@ -1138,6 +1138,7 @@ Multiple upgrades is not allowed
>    i>     hello\n
>    o> readline() -> 1:
>    o>     \n
> +  o> read(-1) -> 0:
>    e> read(-1) -> 42:
>    e>     cannot upgrade protocols multiple times\n
>    e>     -\n
> @@ -1229,6 +1230,7 @@ Upgrade request must be followed by hell
>    i>     invalid\n
>    o> readline() -> 1:
>    o>     \n
> +  o> read(-1) -> 0:
>    e> read(-1) -> 46:
>    e>     malformed handshake protocol: missing hello\n
>    e>     -\n
> @@ -1248,6 +1250,7 @@ Upgrade request must be followed by hell
>    i>     invalid\n
>    o> readline() -> 1:
>    o>     \n
> +  o> read(-1) -> 0:
>    e> read(-1) -> 48:
>    e>     malformed handshake protocol: missing between\n
>    e>     -\n
> @@ -1269,6 +1272,7 @@ Upgrade request must be followed by hell
>    i>     invalid\n
>    o> readline() -> 1:
>    o>     \n
> +  o> read(-1) -> 0:
>    e> read(-1) -> 49:
>    e>     malformed handshake protocol: missing pairs 81\n
>    e>     -\n
>

Patch

diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -2690,7 +2690,8 @@  def debugwireproto(ui, repo, **opts):
     readavailable
     -------------
 
-    Read all available data from the server.
+    Close the write end of the connection and read all available data from
+    the server.
 
     If the connection to the server encompasses multiple pipes, we poll both
     pipes and read available data.
@@ -2835,17 +2836,9 @@  def debugwireproto(ui, repo, **opts):
         elif action == 'close':
             peer.close()
         elif action == 'readavailable':
-            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 act:
-                util.readpipe(stderr)
+            stdin.close()
+            stdout.read()
+            stderr.read()
         elif action == 'readline':
             stdout.readline()
         else:
diff --git a/tests/test-ssh-proto-unbundle.t b/tests/test-ssh-proto-unbundle.t
--- a/tests/test-ssh-proto-unbundle.t
+++ b/tests/test-ssh-proto-unbundle.t
@@ -93,6 +93,7 @@  Test pushing bundle1 payload to a server
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 115:
   e>     abort: incompatible Mercurial client; bundle2 required\n
   e>     (see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n
@@ -143,6 +144,7 @@  Test pushing bundle1 payload to a server
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 115:
   e>     abort: incompatible Mercurial client; bundle2 required\n
   e>     (see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n
@@ -260,6 +262,7 @@  ui.write() in hook is redirected to stde
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 196:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -316,6 +319,7 @@  ui.write() in hook is redirected to stde
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 196:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -386,6 +390,7 @@  And a variation that writes multiple lin
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 218:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -443,6 +448,7 @@  And a variation that writes multiple lin
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 218:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -514,6 +520,7 @@  And a variation that does a ui.flush() a
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 202:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -570,6 +577,7 @@  And a variation that does a ui.flush() a
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 202:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -640,6 +648,7 @@  Multiple writes + flush
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 206:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -697,6 +706,7 @@  Multiple writes + flush
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 206:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -768,6 +778,7 @@  ui.write() + ui.write_err() output is ca
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 232:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -827,6 +838,7 @@  ui.write() + ui.write_err() output is ca
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 232:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -900,6 +912,7 @@  print() output is captured
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 193:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -956,6 +969,7 @@  print() output is captured
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 193:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1026,6 +1040,7 @@  Mixed print() and ui.write() are both ca
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 218:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1085,6 +1100,7 @@  Mixed print() and ui.write() are both ca
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 218:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1158,6 +1174,7 @@  print() to stdout and stderr both get ca
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 216:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1217,6 +1234,7 @@  print() to stdout and stderr both get ca
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 216:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1296,6 +1314,7 @@  Shell hook writing to stdout has output 
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 212:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1353,6 +1372,7 @@  Shell hook writing to stdout has output 
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 212:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1425,6 +1445,7 @@  Shell hook writing to stderr has output 
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 212:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1482,6 +1503,7 @@  Shell hook writing to stderr has output 
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 212:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1556,6 +1578,7 @@  Shell hook writing to stdout and stderr 
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 230:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1615,6 +1638,7 @@  Shell hook writing to stdout and stderr 
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 230:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1697,6 +1721,7 @@  Shell and Python hooks writing to stdout
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 273:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1760,6 +1785,7 @@  Shell and Python hooks writing to stdout
   o> read(1) -> 1: 0
   result: 0
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 273:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1837,6 +1863,7 @@  Pushing a bundle1 with no output
   o> read(1) -> 1: 1
   result: 1
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 100:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1889,6 +1916,7 @@  Pushing a bundle1 with no output
   o> read(1) -> 1: 1
   result: 1
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 100:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -1967,6 +1995,7 @@  Pushing a bundle1 with ui.write() and ui
   o> read(1) -> 1: 1
   result: 1
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 152:
   e>     adding changesets\n
   e>     adding manifests\n
@@ -2023,6 +2052,7 @@  Pushing a bundle1 with ui.write() and ui
   o> read(1) -> 1: 1
   result: 1
   remote output: 
+  o> read(-1) -> 0: 
   e> read(-1) -> 152:
   e>     adding changesets\n
   e>     adding manifests\n
diff --git a/tests/test-ssh-proto.t b/tests/test-ssh-proto.t
--- a/tests/test-ssh-proto.t
+++ b/tests/test-ssh-proto.t
@@ -1138,6 +1138,7 @@  Multiple upgrades is not allowed
   i>     hello\n
   o> readline() -> 1:
   o>     \n
+  o> read(-1) -> 0: 
   e> read(-1) -> 42:
   e>     cannot upgrade protocols multiple times\n
   e>     -\n
@@ -1229,6 +1230,7 @@  Upgrade request must be followed by hell
   i>     invalid\n
   o> readline() -> 1:
   o>     \n
+  o> read(-1) -> 0: 
   e> read(-1) -> 46:
   e>     malformed handshake protocol: missing hello\n
   e>     -\n
@@ -1248,6 +1250,7 @@  Upgrade request must be followed by hell
   i>     invalid\n
   o> readline() -> 1:
   o>     \n
+  o> read(-1) -> 0: 
   e> read(-1) -> 48:
   e>     malformed handshake protocol: missing between\n
   e>     -\n
@@ -1269,6 +1272,7 @@  Upgrade request must be followed by hell
   i>     invalid\n
   o> readline() -> 1:
   o>     \n
+  o> read(-1) -> 0: 
   e> read(-1) -> 49:
   e>     malformed handshake protocol: missing pairs 81\n
   e>     -\n