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

[Gregory Szorc]

On Mon, Mar 12, 2018 at 7:17 AM, Yuya Nishihara <yuya at tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya at 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)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20180312/fc98094d/attachment.html>