Patchwork [4,of,4] windows: always work around EINVAL in case of broken pipe for stdout / stderr

login
register
mail settings
Submitter Manuel Jacob
Date July 17, 2020, 2:38 a.m.
Message ID <741837f160677ec24154.1594953530@tmp>
Download mbox | patch
Permalink /patch/46769/
State Accepted
Headers show

Comments

Manuel Jacob - July 17, 2020, 2:38 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1594949332 -7200
#      Fri Jul 17 03:28:52 2020 +0200
# Node ID 741837f160677ec24154b3d5aa209133a3872d53
# Parent  91f6d0508acac2c8eb0fbf8864528f8f584e697c
# EXP-Topic stdio-broken_pipe
windows: always work around EINVAL in case of broken pipe for stdout / stderr

In 29a905fe23ae, I missed the fact that the `winstdout` class works around two
unrelated bugs (size limit when writing to consoles and EINVAL in case of
broken pipe) and that the latter bug happens even when no console is involved.
When writing a test for this, I realized that the same problem applies to
stderr, so I applied the workaround for EINVAL to both stdout and stderr.

The size limit is worked around in the same case as before (consoles on Windows
on Python 2). For that, I changed the `winstdout` class.
Yuya Nishihara - July 17, 2020, 11:24 a.m.
On Fri, 17 Jul 2020 04:38:50 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1594949332 -7200
> #      Fri Jul 17 03:28:52 2020 +0200
> # Node ID 741837f160677ec24154b3d5aa209133a3872d53
> # Parent  91f6d0508acac2c8eb0fbf8864528f8f584e697c
> # EXP-Topic stdio-broken_pipe
> windows: always work around EINVAL in case of broken pipe for stdout / stderr

Queued, thanks.

> diff --git a/mercurial/windows.py b/mercurial/windows.py
> --- a/mercurial/windows.py
> +++ b/mercurial/windows.py
> @@ -197,6 +197,7 @@
>  
>      def __init__(self, fp):
>          self.fp = fp
> +        self.throttle = not pycompat.ispy3 and fp.isatty()

If we need to handle the case where sys.stdout is a pseudo file object on
Python 2, isatty() may be missing.

>      def write(self, s):
> +        if not pycompat.ispy3:
> +            self.softspace = 0

No idea what this softspace is for, but I assume it's py2-only attribute.
Manuel Jacob - July 17, 2020, 1:21 p.m.
On 2020-07-17 13:24, Yuya Nishihara wrote:
> On Fri, 17 Jul 2020 04:38:50 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1594949332 -7200
>> #      Fri Jul 17 03:28:52 2020 +0200
>> # Node ID 741837f160677ec24154b3d5aa209133a3872d53
>> # Parent  91f6d0508acac2c8eb0fbf8864528f8f584e697c
>> # EXP-Topic stdio-broken_pipe
>> windows: always work around EINVAL in case of broken pipe for stdout / 
>> stderr
> 
> Queued, thanks.
> 
>> diff --git a/mercurial/windows.py b/mercurial/windows.py
>> --- a/mercurial/windows.py
>> +++ b/mercurial/windows.py
>> @@ -197,6 +197,7 @@
>> 
>>      def __init__(self, fp):
>>          self.fp = fp
>> +        self.throttle = not pycompat.ispy3 and fp.isatty()
> 
> If we need to handle the case where sys.stdout is a pseudo file object 
> on
> Python 2, isatty() may be missing.

Right, I’ll send another patch. Feel free to fold them together.

>>      def write(self, s):
>> +        if not pycompat.ispy3:
>> +            self.softspace = 0
> 
> No idea what this softspace is for, but I assume it's py2-only 
> attribute.

It’s an obscure implementation artifact of the Python 2 print function: 
https://docs.python.org/2/library/stdtypes.html#file.softspace

I tried to not change the behavior of the existing code on Python 2. 
However, reading from the documentation, it is likely that removing that 
code makes it more correct that leaving it. Also, I think we don’t use 
the Python 2 print function ourselves. Should we remove the two lines?
Yuya Nishihara - July 18, 2020, 10:03 a.m.
On Fri, 17 Jul 2020 15:21:57 +0200, Manuel Jacob wrote:
> On 2020-07-17 13:24, Yuya Nishihara wrote:
> >>      def write(self, s):
> >> +        if not pycompat.ispy3:
> >> +            self.softspace = 0
> > 
> > No idea what this softspace is for, but I assume it's py2-only 
> > attribute.
> 
> It’s an obscure implementation artifact of the Python 2 print function: 
> https://docs.python.org/2/library/stdtypes.html#file.softspace
> 
> I tried to not change the behavior of the existing code on Python 2. 
> However, reading from the documentation, it is likely that removing that 
> code makes it more correct that leaving it. Also, I think we don’t use 
> the Python 2 print function ourselves. Should we remove the two lines?

I prefer removing it. If the writable softspace were so important, I think
our other file wrappers would already be broken.

Patch

diff --git a/mercurial/utils/procutil.py b/mercurial/utils/procutil.py
--- a/mercurial/utils/procutil.py
+++ b/mercurial/utils/procutil.py
@@ -119,18 +119,25 @@ 
     # a silly wrapper to make a bytes stream backed by a unicode one.
     stdin = sys.stdin.buffer
     stdout = _make_write_all(sys.stdout.buffer)
+    stderr = _make_write_all(sys.stderr.buffer)
+    if pycompat.iswindows:
+        # Work around Windows bugs.
+        stdout = platform.winstdout(stdout)
+        stderr = platform.winstdout(stderr)
     if isatty(stdout):
         # The standard library doesn't offer line-buffered binary streams.
         stdout = make_line_buffered(stdout)
-    stderr = _make_write_all(sys.stderr.buffer)
 else:
     # Python 2 uses the I/O streams provided by the C library.
     stdin = sys.stdin
     stdout = sys.stdout
+    stderr = sys.stderr
+    if pycompat.iswindows:
+        # Work around Windows bugs.
+        stdout = platform.winstdout(stdout)
+        stderr = platform.winstdout(stderr)
     if isatty(stdout):
         if pycompat.iswindows:
-            # Work around size limit when writing to console.
-            stdout = platform.winstdout(stdout)
             # The Windows C runtime library doesn't support line buffering.
             stdout = make_line_buffered(stdout)
         else:
@@ -138,7 +145,6 @@ 
             # replace a TTY destined stdout with a pipe destined stdout (e.g.
             # pager), we want line buffering.
             stdout = os.fdopen(stdout.fileno(), 'wb', 1)
-    stderr = sys.stderr
 
 
 findexe = platform.findexe
diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -197,6 +197,7 @@ 
 
     def __init__(self, fp):
         self.fp = fp
+        self.throttle = not pycompat.ispy3 and fp.isatty()
 
     def __getattr__(self, key):
         return getattr(self.fp, key)
@@ -208,13 +209,16 @@ 
             pass
 
     def write(self, s):
+        if not pycompat.ispy3:
+            self.softspace = 0
         try:
+            if not self.throttle:
+                return self.fp.write(s)
             # This is workaround for "Not enough space" error on
             # writing large size of data to console.
             limit = 16000
             l = len(s)
             start = 0
-            self.softspace = 0
             while start < l:
                 end = start + limit
                 self.fp.write(s[start:end])
diff --git a/tests/test-stdio.py b/tests/test-stdio.py
--- a/tests/test-stdio.py
+++ b/tests/test-stdio.py
@@ -7,13 +7,14 @@ 
 import contextlib
 import errno
 import os
+import pickle
 import signal
 import subprocess
 import sys
 import tempfile
 import unittest
 
-from mercurial import pycompat
+from mercurial import pycompat, util
 
 
 if pycompat.ispy3:
@@ -71,6 +72,34 @@ 
 '''
 
 
+TEST_BROKEN_PIPE_CHILD_SCRIPT = r'''
+import os
+import pickle
+
+from mercurial import dispatch
+from mercurial.utils import procutil
+
+dispatch.initstdio()
+procutil.stdin.read(1)  # wait until parent process closed pipe
+try:
+    procutil.{stream}.write(b'test')
+    procutil.{stream}.flush()
+except EnvironmentError as e:
+    with os.fdopen(
+        os.open(
+            {err_fn!r},
+            os.O_WRONLY
+            | getattr(os, 'O_BINARY', 0)
+            | getattr(os, 'O_TEMPORARY', 0),
+        ),
+        'wb',
+    ) as err_f:
+        pickle.dump(e, err_f)
+# Exit early to suppress further broken pipe errors at interpreter shutdown.
+os._exit(0)
+'''
+
+
 @contextlib.contextmanager
 def _closing(fds):
     try:
@@ -148,11 +177,15 @@ 
         check_output,
         python_args=[],
         post_child_check=None,
+        stdin_generator=None,
     ):
         assert stream in ('stdout', 'stderr')
-        with rwpair_generator() as (stream_receiver, child_stream), open(
-            os.devnull, 'rb'
-        ) as child_stdin:
+        if stdin_generator is None:
+            stdin_generator = open(os.devnull, 'rb')
+        with rwpair_generator() as (
+            stream_receiver,
+            child_stream,
+        ), stdin_generator as child_stdin:
             proc = subprocess.Popen(
                 [sys.executable] + python_args + ['-c', child_script],
                 stdin=child_stdin,
@@ -295,6 +328,37 @@ 
     def test_large_write_stderr_ptys_unbuffered(self):
         self._test_large_write('stderr', _ptys, python_args=['-u'])
 
+    def _test_broken_pipe(self, stream):
+        assert stream in ('stdout', 'stderr')
+
+        def check_output(stream_receiver, proc):
+            os.close(stream_receiver)
+            proc.stdin.write(b'x')
+            proc.stdin.close()
+
+        def post_child_check():
+            err = pickle.load(err_f)
+            self.assertEqual(err.errno, errno.EPIPE)
+            self.assertEqual(err.strerror, "Broken pipe")
+
+        with tempfile.NamedTemporaryFile('rb') as err_f:
+            self._test(
+                TEST_BROKEN_PIPE_CHILD_SCRIPT.format(
+                    stream=stream, err_fn=err_f.name
+                ),
+                stream,
+                _pipes,
+                check_output,
+                post_child_check=post_child_check,
+                stdin_generator=util.nullcontextmanager(subprocess.PIPE),
+            )
+
+    def test_broken_pipe_stdout(self):
+        self._test_broken_pipe('stdout')
+
+    def test_broken_pipe_stderr(self):
+        self._test_broken_pipe('stderr')
+
 
 if __name__ == '__main__':
     import silenttestrunner