Patchwork [2,of,3] debugwireproto: dump server's stderr to temporary file if --noreadstderr

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

Comments

Yuya Nishihara - March 12, 2018, 2:17 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1520858510 -32400
#      Mon Mar 12 21:41:50 2018 +0900
# Node ID d3dd691a3fce0c501a34ed68d1a08b563a78794c
# Parent  2e2a5376d006ad77ba9a07d341f6bc5418289af1
debugwireproto: dump server's stderr to temporary file if --noreadstderr

Otherwise the server process could stall because of writing too many data
to stderr.

No idea if this is strictly better than the original implementation, but
this allows dropping readstderr=False support from sshpeer.
Gregory Szorc - March 12, 2018, 6:15 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 1520858510 -32400
> #      Mon Mar 12 21:41:50 2018 +0900
> # Node ID d3dd691a3fce0c501a34ed68d1a08b563a78794c
> # Parent  2e2a5376d006ad77ba9a07d341f6bc5418289af1
> debugwireproto: dump server's stderr to temporary file if --noreadstderr
>
> Otherwise the server process could stall because of writing too many data
> to stderr.
>
> No idea if this is strictly better than the original implementation, but
> this allows dropping readstderr=False support from sshpeer.
>

I like what this is doing by redirecting stderr to a file so the pipe
doesn't clog.

I'm less keen about losing the ordering of what is read from stderr when.
None of the current tests rely on it, but we will presumably want to have
tests for exchanges performing multiple commands where there could be
multiple writes to stderr. So, we'd want a deterministic way to get all
data on stderr so we can log it. With this change, we'd need to read from a
temp file. That seems complicated.

That being said, we don't actually rely on this behavior yet. So this
change feels strictly better.

Let me think about it for a bit before queuing.


>
> diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
> --- a/mercurial/debugcommands.py
> +++ b/mercurial/debugcommands.py
> @@ -2716,7 +2716,9 @@ def debugwireproto(ui, repo, **opts):
>
>      blocks = list(_parsewirelangblocks(ui.fin))
>
> +    logging = (ui.verbose or opts['peer'] == 'raw')
>      proc = None
> +    stderrfname = None
>
>      if opts['localssh']:
>          # We start the SSH server in its own process so there is process
> @@ -2726,27 +2728,34 @@ def debugwireproto(ui, repo, **opts):
>              '-R', repo.root,
>              'debugserve', '--sshstdio',
>          ]
> +        autoreadstderr = not opts['noreadstderr']
> +        stderrfd = subprocess.PIPE
> +        if not autoreadstderr:
> +            # Dump stderr to file so the server process never stalls
> +            stderrfd, stderrfname = tempfile.mkstemp(prefix='hg-
> debugserve-')
>          proc = subprocess.Popen(args, stdin=subprocess.PIPE,
> -                                stdout=subprocess.PIPE,
> stderr=subprocess.PIPE,
> +                                stdout=subprocess.PIPE, stderr=stderrfd,
>                                  bufsize=0)
> +        if stderrfd != subprocess.PIPE:
> +            os.close(stderrfd)
>
>          stdin = proc.stdin
>          stdout = proc.stdout
> -        stderr = proc.stderr
> +        stderr = proc.stderr  # may be None
>
>          # We turn the pipes into observers so we can log I/O.
> -        if ui.verbose or opts['peer'] == 'raw':
> +        if logging:
>              stdin = util.makeloggingfileobject(ui, proc.stdin, b'i',
>                                                 logdata=True)
>              stdout = util.makeloggingfileobject(ui, proc.stdout, b'o',
>                                                  logdata=True)
> +        if logging and stderr:
>              stderr = util.makeloggingfileobject(ui, proc.stderr, b'e',
>                                                  logdata=True)
>
>          # --localssh also implies the peer connection settings.
>
>          url = 'ssh://localserver'
> -        autoreadstderr = not opts['noreadstderr']
>
>          if opts['peer'] == 'ssh1':
>              ui.write(_('creating ssh peer for wire protocol version 1\n'))
> @@ -2838,7 +2847,8 @@ def debugwireproto(ui, repo, **opts):
>          elif action == 'readavailable':
>              stdin.close()
>              stdout.read()
> -            stderr.read()
> +            if stderr:
> +                stderr.read()
>          elif action == 'readline':
>              stdout.readline()
>          else:
> @@ -2852,3 +2862,10 @@ def debugwireproto(ui, repo, **opts):
>
>      if proc:
>          proc.kill()
> +
> +    if stderrfname:
> +        with open(stderrfname, 'rb') as f:
> +            if logging:
> +                f = util.makeloggingfileobject(ui, f, b'e', logdata=True)
> +            f.read()
> +        os.unlink(stderrfname)
>

Patch

diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -2716,7 +2716,9 @@  def debugwireproto(ui, repo, **opts):
 
     blocks = list(_parsewirelangblocks(ui.fin))
 
+    logging = (ui.verbose or opts['peer'] == 'raw')
     proc = None
+    stderrfname = None
 
     if opts['localssh']:
         # We start the SSH server in its own process so there is process
@@ -2726,27 +2728,34 @@  def debugwireproto(ui, repo, **opts):
             '-R', repo.root,
             'debugserve', '--sshstdio',
         ]
+        autoreadstderr = not opts['noreadstderr']
+        stderrfd = subprocess.PIPE
+        if not autoreadstderr:
+            # Dump stderr to file so the server process never stalls
+            stderrfd, stderrfname = tempfile.mkstemp(prefix='hg-debugserve-')
         proc = subprocess.Popen(args, stdin=subprocess.PIPE,
-                                stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+                                stdout=subprocess.PIPE, stderr=stderrfd,
                                 bufsize=0)
+        if stderrfd != subprocess.PIPE:
+            os.close(stderrfd)
 
         stdin = proc.stdin
         stdout = proc.stdout
-        stderr = proc.stderr
+        stderr = proc.stderr  # may be None
 
         # We turn the pipes into observers so we can log I/O.
-        if ui.verbose or opts['peer'] == 'raw':
+        if logging:
             stdin = util.makeloggingfileobject(ui, proc.stdin, b'i',
                                                logdata=True)
             stdout = util.makeloggingfileobject(ui, proc.stdout, b'o',
                                                 logdata=True)
+        if logging and stderr:
             stderr = util.makeloggingfileobject(ui, proc.stderr, b'e',
                                                 logdata=True)
 
         # --localssh also implies the peer connection settings.
 
         url = 'ssh://localserver'
-        autoreadstderr = not opts['noreadstderr']
 
         if opts['peer'] == 'ssh1':
             ui.write(_('creating ssh peer for wire protocol version 1\n'))
@@ -2838,7 +2847,8 @@  def debugwireproto(ui, repo, **opts):
         elif action == 'readavailable':
             stdin.close()
             stdout.read()
-            stderr.read()
+            if stderr:
+                stderr.read()
         elif action == 'readline':
             stdout.readline()
         else:
@@ -2852,3 +2862,10 @@  def debugwireproto(ui, repo, **opts):
 
     if proc:
         proc.kill()
+
+    if stderrfname:
+        with open(stderrfname, 'rb') as f:
+            if logging:
+                f = util.makeloggingfileobject(ui, f, b'e', logdata=True)
+            f.read()
+        os.unlink(stderrfname)