hook: stdout/stderr corruption with redirection (e.g. ssh repo)

Thomas De Schampheleire patrickdepinguin+mercurial at gmail.com
Mon Nov 14 06:25:50 CST 2011


Hi,

I have been experiencing cloning problems in particular situations,
reproducibly. The problem only occurs when cloning over ssh (even from
localhost), with particular repositories/revisions, when any
non-python outgoing hook was present.

Comparison of data sent over the connection with and without outgoing
hook, showed that in the bad case (with hook) there was source code
appearing on stderr. This source code was supposed to be sent on
stdout, as it is part of the clone data.

The problem manifests itself as:

requesting all changes
adding changesets
adding manifests
adding file changes
transaction abort!
rollback completed
abort: consistency error in delta!

This is consistent with our stdout/stderr observation, as part of the
clone data would be missing from stdout due to a bad redirection to
stderr.

Looking at the relevant code from mercurial/hook.py:

    oldstdout = -1
    if _redirect:
        try:
            stdoutno = sys.__stdout__.fileno()
            stderrno = sys.__stderr__.fileno()
            # temporarily redirect stdout to stderr, if possible
            if stdoutno >= 0 and stderrno >= 0:
                oldstdout = os.dup(stdoutno)
                os.dup2(stderrno, stdoutno)
        except AttributeError:
            # __stdout/err__ doesn't have fileno(), it's not a real file
            pass

The stdout descriptor is redirected to stderr, but no flushing of
stdout is attempted.
If we add flushing of stdout, as in following patch, the problem disappears.

diff --git a/mercurial/hook.py b/mercurial/hook.py
--- a/mercurial/hook.py
+++ b/mercurial/hook.py
@@ -139,6 +139,7 @@ def hook(ui, repo, name, throw=False, **
             stderrno = sys.__stderr__.fileno()
             # temporarily redirect stdout to stderr, if possible
             if stdoutno >= 0 and stderrno >= 0:
+                sys.__stdout__.flush()
                 oldstdout = os.dup(stdoutno)
                 os.dup2(stderrno, stdoutno)
         except AttributeError:

I'm not sure whether this is airtight though. I can not assess if
there are other situations in which flushing would be required, or
maybe if there is a better solution.
If you agree that the above change is the right one, I can send a
proper patch if you like (or feel free to just apply it).

The problem is still present in 2.0.

Best regards,
Thomas


More information about the Mercurial-devel mailing list