Patchwork [5,of,5] procutil: flush both procutil.std{out,err} and sys.std{out,err} at exit

login
register
mail settings
Submitter Manuel Jacob
Date July 10, 2020, 4:46 a.m.
Message ID <5a2c9bccd57f3e2c411c.1594356402@tmp>
Download mbox | patch
Permalink /patch/46683/
State Accepted
Headers show

Comments

Manuel Jacob - July 10, 2020, 4:46 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1594346556 -7200
#      Fri Jul 10 04:02:36 2020 +0200
# Node ID 5a2c9bccd57f3e2c411c8464ef5f402e4206aa12
# Parent  034feda7f6afcb0ae973569207a8268487793962
# EXP-Topic stdio
procutil: flush both procutil.std{out,err} and sys.std{out,err} at exit

Changeset 8403cc54bc83 introduced code that opens a second file object
referring to the stderr file descriptor. This broke tests on Windows. The
reason is that on Windows, sys.stderr is buffered and procutil.stderr closed
the file descriptor when it got garbage collected before sys.stderr had the
chance to flush buffered data.

Possibly, stdout had the same problem for a long time, but we didn’t realize,
as in CI test runs, stdout is not a TTY and therefore no second file object was
opened.

Patch

diff --git a/mercurial/utils/procutil.py b/mercurial/utils/procutil.py
--- a/mercurial/utils/procutil.py
+++ b/mercurial/utils/procutil.py
@@ -50,6 +50,27 @@ 
         return False
 
 
+def reopen_stream(orig, mode, buffering):
+    import atexit
+
+    assert not pycompat.ispy3
+    # On Python 2, there's no way to create a file such that the file
+    # descriptor doesn't get closed if the file object gets garbage collected.
+    # Therefore, when reopening an already open stream, the file descriptor
+    # will be closed twice. The error message for that will be swallowed during
+    # interpreter shutdown.
+    new = os.fdopen(orig.fileno(), mode, buffering)
+
+    @atexit.register
+    def flush_streams():
+        # Ensure that one stream gets flushed before the other closes the file
+        # descriptor.
+        new.flush()
+        orig.flush()
+
+    return new
+
+
 class LineBufferedWrapper(object):
     def __init__(self, orig):
         self.orig = orig
@@ -104,7 +125,7 @@ 
         # Therefore we have a wrapper that implements line buffering.
         stdout = make_line_buffered(stdout)
     else:
-        stdout = os.fdopen(stdout.fileno(), 'wb', 1)
+        stdout = reopen_stream(stdout, 'wb', 1)
 
 # stderr should be unbuffered
 if pycompat.ispy3:
@@ -115,7 +136,7 @@ 
     stderr = getattr(stderr, 'raw', stderr)
 elif pycompat.iswindows:
     # On Windows, stderr is buffered at least when connected to a pipe.
-    stderr = os.fdopen(stderr.fileno(), 'wb', 0)
+    stderr = reopen_stream(stderr, 'wb', 0)
 # On other platforms, stderr is always unbuffered.
 
 
diff --git a/tests/test-stdio.py b/tests/test-stdio.py
--- a/tests/test-stdio.py
+++ b/tests/test-stdio.py
@@ -30,6 +30,16 @@ 
 LINE_BUFFERED = b'[written aaa]aaabbb\n[written bbb\\n]'
 FULLY_BUFFERED = b'[written aaa][written bbb\\n]aaabbb\n'
 
+TEST_FLUSH_SYS_STDIO_CHILD_SCRIPT = r'''
+import sys
+
+from mercurial import dispatch
+from mercurial.utils import procutil
+
+dispatch.initstdio()
+sys.{stream}.write('test')
+'''
+
 
 @contextlib.contextmanager
 def _closing(fds):
@@ -155,6 +165,29 @@ 
     def test_buffering_stderr_ptys_unbuffered(self):
         self._test_buffering('stderr', _ptys, UNBUFFERED, python_args=['-u'])
 
+    def _test_flush_sys_stdout(self, stream, rwpair_generator):
+        def check_output(stream_receiver):
+            self.assertEqual(_readall(stream_receiver, 1024), b'test')
+
+        self._test(
+            TEST_FLUSH_SYS_STDIO_CHILD_SCRIPT.format(stream=stream),
+            stream,
+            rwpair_generator,
+            check_output,
+        )
+
+    def test_flush_sys_stdout_pipes(self):
+        self._test_flush_sys_stdout('stdout', _pipes)
+
+    def test_flush_sys_stdout_ptys(self):
+        self._test_flush_sys_stdout('stdout', _ptys)
+
+    def test_flush_sys_stderr_pipes(self):
+        self._test_flush_sys_stdout('stderr', _pipes)
+
+    def test_flush_sys_stderr_ptys(self):
+        self._test_flush_sys_stdout('stderr', _ptys)
+
 
 if __name__ == '__main__':
     import silenttestrunner