[PATCH 1 of 8] hgk: stop using util.bytesinput() to read a single line from stdin

Yuya Nishihara yuya at tcha.org
Sat Mar 10 02:49:00 EST 2018


On Fri, 9 Mar 2018 09:25:10 -0800, Gregory Szorc wrote:
> If you have this stdio stuff paged into your brain, you may want to look at
> hook.py and what it is doing with stdio. Essentially, it is doing dup() and
> dup2() to temporarily redirect stdout to stderr such that the wire protocol
> can intercept output and forward it to the wire protocol client (or the
> CLI).

and hook output never be interleaved into the wire protocol channel.

> See hook.redirect(True) in the wire protocol server code and follow
> the trail from there.
> 
> In the case of shell hooks, I believe those processes inherit the parent's
> file descriptors. Which after hook.py mucks with the file descriptors, is
> actually the stderr stream.

I think these dup()s are no longer needed for shell hooks since a subprocess
output is sent to ui.fout which is actually ui.ferr in ssh session. They're
still valid for in-process hooks, though.

> I question the appropriateness of the approach. I think it would be better
> to e.g. send ui.ferr to shell hooks and to temporarily muck with
> sys.stdout/sys.stderr when running Python hooks. But this has implications
> and I haven't thought it through. I'd *really* like to see us not have to
> do the dup()/dup2() dance in the hooks because that is mucking with global
> state and can make it hard to debug server processes.

Yeah. I doubt if the current code would work well in threaded hgweb.

I think we can do a similar trick to commandserver, which basically nullifes
the global sys.stdin/stdout to protect IPC channels from being corrupted by
third-party extensions, and use ui.fin/fout thoroughly.

https://www.mercurial-scm.org/repo/hg/file/4.5.2/mercurial/commandserver.py#l334

Regarding the wire protocol and hooks, this means:

 - do dup()/dup2() business globally, not locally in hook.py
 - isolate wire-protocol streams from ui.fin/fout
   (ssh-wire: dup()ed stdin/stdout, ui.fin -> stdin -> null, stdout -> null,
    ui.fout -> ui.ferr -> stderr)
 - in-process hook must use ui instead of print() because print()s will
   be sent to /dev/null
 - shell hook output is redirected to stderr as long as ui.fout points to
   stderr (the stderr will be dup()ed to stdout after fork)


More information about the Mercurial-devel mailing list