[PATCH 1 of 5] tests: proof test-stdio.py against buffer fill-up

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1 of 5] tests: proof test-stdio.py against buffer fill-up

Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1594112797 -7200
#      Tue Jul 07 11:06:37 2020 +0200
# Node ID 7c5896871c7e9e8e510c0436f5c2bcc6018e8177
# Parent  f43bc4ce0d697051135ff21080b7794e6aaea8bf
# EXP-Topic stdio
tests: proof test-stdio.py against buffer fill-up

With the previous code, it could in theory happen that the pipe / PTY buffer of
the child stdout / stderr fills up and the process never finishes.

To prevent that, we read all of the stream before waiting for the end of the
process. To ensure that the stream reaches EOF when the child finishes, we must
close the parent "copy" of the child stdout / stderr.

diff --git a/tests/test-stdio.py b/tests/test-stdio.py
--- a/tests/test-stdio.py
+++ b/tests/test-stdio.py
@@ -5,6 +5,7 @@
 from __future__ import absolute_import
 
 import contextlib
+import errno
 import os
 import subprocess
 import sys
@@ -62,6 +63,23 @@
         yield rwpair
 
 
+def _readall(fd, buffer_size):
+    buf = []
+    while True:
+        try:
+            s = os.read(fd, buffer_size)
+        except OSError as e:
+            if e.errno == errno.EIO:
+                # If the child-facing PTY got closed, reading from the
+                # parent-facing PTY raises EIO.
+                break
+            raise
+        if not s:
+            break
+        buf.append(s)
+    return b''.join(buf)
+
+
 class TestStdio(unittest.TestCase):
     def _test(self, stream, rwpair_generator, expected_output, python_args=[]):
         assert stream in ('stdout', 'stderr')
@@ -76,9 +94,14 @@
                 stdout=child_stream if stream == 'stdout' else None,
                 stderr=child_stream if stream == 'stderr' else None,
             )
-            retcode = proc.wait()
+            try:
+                os.close(child_stream)
+                self.assertEqual(
+                    _readall(stream_receiver, 1024), expected_output
+                )
+            finally:
+                retcode = proc.wait()
             self.assertEqual(retcode, 0)
-            self.assertEqual(os.read(stream_receiver, 1024), expected_output)
 
     def test_stdout_pipes(self):
         self._test('stdout', _pipes, FULLY_BUFFERED)

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1594118129 -7200
#      Tue Jul 07 12:35:29 2020 +0200
# Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
# Parent  7c5896871c7e9e8e510c0436f5c2bcc6018e8177
# EXP-Topic stdio
tests: terminate subprocess in test-stdio.py in case of exception

If an error happened while reading the output of the subprocess, the pipe / TTY
buffer can fill up and prevent that the subprocess ends. Therefore we should
terminate the subprocess in case of an exception.

diff --git a/tests/test-stdio.py b/tests/test-stdio.py
--- a/tests/test-stdio.py
+++ b/tests/test-stdio.py
@@ -99,6 +99,9 @@
                 self.assertEqual(
                     _readall(stream_receiver, 1024), expected_output
                 )
+            except:  # re-raises
+                proc.terminate()
+                raise
             finally:
                 retcode = proc.wait()
             self.assertEqual(retcode, 0)

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 3 of 5] tests: make names in test-stdio.py more distinctive

Manuel Jacob
In reply to this post by Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1594113007 -7200
#      Tue Jul 07 11:10:07 2020 +0200
# Node ID 8d2f25abde563d51bfc6296ba4136b9e7a86ecd8
# Parent  0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
# EXP-Topic stdio
tests: make names in test-stdio.py more distinctive

This way, more tests can be added without name clashes.

diff --git a/tests/test-stdio.py b/tests/test-stdio.py
--- a/tests/test-stdio.py
+++ b/tests/test-stdio.py
@@ -14,7 +14,7 @@
 from mercurial import pycompat
 
 
-CHILD_PROCESS = r'''
+BUFFERING_CHILD_SCRIPT = r'''
 import os
 
 from mercurial import dispatch
@@ -89,7 +89,7 @@
             proc = subprocess.Popen(
                 [sys.executable]
                 + python_args
-                + ['-c', CHILD_PROCESS.format(stream=stream)],
+                + ['-c', BUFFERING_CHILD_SCRIPT.format(stream=stream)],
                 stdin=child_stdin,
                 stdout=child_stream if stream == 'stdout' else None,
                 stderr=child_stream if stream == 'stderr' else None,
@@ -106,36 +106,36 @@
                 retcode = proc.wait()
             self.assertEqual(retcode, 0)
 
-    def test_stdout_pipes(self):
+    def test_buffering_stdout_pipes(self):
         self._test('stdout', _pipes, FULLY_BUFFERED)
 
-    def test_stdout_ptys(self):
+    def test_buffering_stdout_ptys(self):
         self._test('stdout', _ptys, LINE_BUFFERED)
 
-    def test_stdout_pipes_unbuffered(self):
+    def test_buffering_stdout_pipes_unbuffered(self):
         self._test('stdout', _pipes, UNBUFFERED, python_args=['-u'])
 
-    def test_stdout_ptys_unbuffered(self):
+    def test_buffering_stdout_ptys_unbuffered(self):
         self._test('stdout', _ptys, UNBUFFERED, python_args=['-u'])
 
     if not pycompat.ispy3 and not pycompat.iswindows:
         # On Python 2 on non-Windows, we manually open stdout in line-buffered
         # mode if connected to a TTY. We should check if Python was configured
         # to use unbuffered stdout, but it's hard to do that.
-        test_stdout_ptys_unbuffered = unittest.expectedFailure(
-            test_stdout_ptys_unbuffered
+        test_buffering_stdout_ptys_unbuffered = unittest.expectedFailure(
+            test_buffering_stdout_ptys_unbuffered
         )
 
-    def test_stderr_pipes(self):
+    def test_buffering_stderr_pipes(self):
         self._test('stderr', _pipes, UNBUFFERED)
 
-    def test_stderr_ptys(self):
+    def test_buffering_stderr_ptys(self):
         self._test('stderr', _ptys, UNBUFFERED)
 
-    def test_stderr_pipes_unbuffered(self):
+    def test_buffering_stderr_pipes_unbuffered(self):
         self._test('stderr', _pipes, UNBUFFERED, python_args=['-u'])
 
-    def test_stderr_ptys_unbuffered(self):
+    def test_buffering_stderr_ptys_unbuffered(self):
         self._test('stderr', _ptys, UNBUFFERED, python_args=['-u'])
 
 

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 4 of 5] tests: make subprocess handling reusable for different tests in test-stdio.py

Manuel Jacob
In reply to this post by Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1594116820 -7200
#      Tue Jul 07 12:13:40 2020 +0200
# Node ID 034feda7f6afcb0ae973569207a8268487793962
# Parent  8d2f25abde563d51bfc6296ba4136b9e7a86ecd8
# EXP-Topic stdio
tests: make subprocess handling reusable for different tests in test-stdio.py

diff --git a/tests/test-stdio.py b/tests/test-stdio.py
--- a/tests/test-stdio.py
+++ b/tests/test-stdio.py
@@ -14,7 +14,7 @@
 from mercurial import pycompat
 
 
-BUFFERING_CHILD_SCRIPT = r'''
+TEST_BUFFERING_CHILD_SCRIPT = r'''
 import os
 
 from mercurial import dispatch
@@ -81,24 +81,27 @@
 
 
 class TestStdio(unittest.TestCase):
-    def _test(self, stream, rwpair_generator, expected_output, python_args=[]):
+    def _test(
+        self,
+        child_script,
+        stream,
+        rwpair_generator,
+        check_output,
+        python_args=[],
+    ):
         assert stream in ('stdout', 'stderr')
         with rwpair_generator() as (stream_receiver, child_stream), open(
             os.devnull, 'rb'
         ) as child_stdin:
             proc = subprocess.Popen(
-                [sys.executable]
-                + python_args
-                + ['-c', BUFFERING_CHILD_SCRIPT.format(stream=stream)],
+                [sys.executable] + python_args + ['-c', child_script],
                 stdin=child_stdin,
                 stdout=child_stream if stream == 'stdout' else None,
                 stderr=child_stream if stream == 'stderr' else None,
             )
             try:
                 os.close(child_stream)
-                self.assertEqual(
-                    _readall(stream_receiver, 1024), expected_output
-                )
+                check_output(stream_receiver)
             except:  # re-raises
                 proc.terminate()
                 raise
@@ -106,17 +109,31 @@
                 retcode = proc.wait()
             self.assertEqual(retcode, 0)
 
+    def _test_buffering(
+        self, stream, rwpair_generator, expected_output, python_args=[]
+    ):
+        def check_output(stream_receiver):
+            self.assertEqual(_readall(stream_receiver, 1024), expected_output)
+
+        self._test(
+            TEST_BUFFERING_CHILD_SCRIPT.format(stream=stream),
+            stream,
+            rwpair_generator,
+            check_output,
+            python_args,
+        )
+
     def test_buffering_stdout_pipes(self):
-        self._test('stdout', _pipes, FULLY_BUFFERED)
+        self._test_buffering('stdout', _pipes, FULLY_BUFFERED)
 
     def test_buffering_stdout_ptys(self):
-        self._test('stdout', _ptys, LINE_BUFFERED)
+        self._test_buffering('stdout', _ptys, LINE_BUFFERED)
 
     def test_buffering_stdout_pipes_unbuffered(self):
-        self._test('stdout', _pipes, UNBUFFERED, python_args=['-u'])
+        self._test_buffering('stdout', _pipes, UNBUFFERED, python_args=['-u'])
 
     def test_buffering_stdout_ptys_unbuffered(self):
-        self._test('stdout', _ptys, UNBUFFERED, python_args=['-u'])
+        self._test_buffering('stdout', _ptys, UNBUFFERED, python_args=['-u'])
 
     if not pycompat.ispy3 and not pycompat.iswindows:
         # On Python 2 on non-Windows, we manually open stdout in line-buffered
@@ -127,16 +144,16 @@
         )
 
     def test_buffering_stderr_pipes(self):
-        self._test('stderr', _pipes, UNBUFFERED)
+        self._test_buffering('stderr', _pipes, UNBUFFERED)
 
     def test_buffering_stderr_ptys(self):
-        self._test('stderr', _ptys, UNBUFFERED)
+        self._test_buffering('stderr', _ptys, UNBUFFERED)
 
     def test_buffering_stderr_pipes_unbuffered(self):
-        self._test('stderr', _pipes, UNBUFFERED, python_args=['-u'])
+        self._test_buffering('stderr', _pipes, UNBUFFERED, python_args=['-u'])
 
     def test_buffering_stderr_ptys_unbuffered(self):
-        self._test('stderr', _ptys, UNBUFFERED, python_args=['-u'])
+        self._test_buffering('stderr', _ptys, UNBUFFERED, python_args=['-u'])
 
 
 if __name__ == '__main__':

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Manuel Jacob
In reply to this post by Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# 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.

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
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH V2] procutil: ensure that all stdio file objects are flushed at interpreter exit

Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1594346556 -7200
#      Fri Jul 10 04:02:36 2020 +0200
# Node ID 284a80ed05e7f4724062c9eeac95933a86257f38
# Parent  034feda7f6afcb0ae973569207a8268487793962
# EXP-Topic stdio
procutil: ensure that all stdio file objects are flushed at interpreter 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.

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_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_stdio(self, stream, rwpair_generator):
+        def check_output(stream_receiver):
+            self.assertEqual(_readall(stream_receiver, 1024), b'test')
+
+        self._test(
+            TEST_FLUSH_STDIO_CHILD_SCRIPT.format(stream=stream),
+            stream,
+            rwpair_generator,
+            check_output,
+        )
+
+    def test_flush_stdout_pipes(self):
+        self._test_flush_stdio('stdout', _pipes)
+
+    def test_flush_stdout_ptys(self):
+        self._test_flush_stdio('stdout', _ptys)
+
+    def test_flush_stderr_pipes(self):
+        self._test_flush_stdio('stderr', _pipes)
+
+    def test_flush_stderr_ptys(self):
+        self._test_flush_stdio('stderr', _ptys)
+
 
 if __name__ == '__main__':
     import silenttestrunner
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Yuya Nishihara
In reply to this post by Manuel Jacob
On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote:

> # HG changeset patch
> # User Manuel Jacob <[hidden email]>
> # Date 1594118129 -7200
> #      Tue Jul 07 12:35:29 2020 +0200
> # Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
> # Parent  7c5896871c7e9e8e510c0436f5c2bcc6018e8177
> # EXP-Topic stdio
> tests: terminate subprocess in test-stdio.py in case of exception
>
> If an error happened while reading the output of the subprocess, the pipe / TTY
> buffer can fill up and prevent that the subprocess ends. Therefore we should
> terminate the subprocess in case of an exception.
>
> diff --git a/tests/test-stdio.py b/tests/test-stdio.py
> --- a/tests/test-stdio.py
> +++ b/tests/test-stdio.py
> @@ -99,6 +99,9 @@
>                  self.assertEqual(
>                      _readall(stream_receiver, 1024), expected_output
>                  )
> +            except:  # re-raises
> +                proc.terminate()
> +                raise

I think assertEqual() should be moved out of the try block. AssertionError
shouldn't be an unexpected error.

I've queued the patch 1-3, thanks. The patch 4 might need some rework in
order to move assertEqual() out of the try block.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V2] procutil: ensure that all stdio file objects are flushed at interpreter exit

Yuya Nishihara
In reply to this post by Manuel Jacob
On Fri, 10 Jul 2020 07:11:04 +0200, Manuel Jacob wrote:

> # HG changeset patch
> # User Manuel Jacob <[hidden email]>
> # Date 1594346556 -7200
> #      Fri Jul 10 04:02:36 2020 +0200
> # Node ID 284a80ed05e7f4724062c9eeac95933a86257f38
> # Parent  034feda7f6afcb0ae973569207a8268487793962
> # EXP-Topic stdio
> procutil: ensure that all stdio file objects are flushed at interpreter 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.

Yep, but IIUC, the major difference is that the Python interpreter needs
sys.stderr to print traceback, etc. So stderr has to remain open after the
procutil module gets GC-ed. On the other hand, stdout wouldn't have such
problem, and nothing should be printed after dispatch.run(), which flushes
the both streams.

Suppose 8403cc54bc83 wouldn't make a real difference in our codebase, I
suggest reverting the change.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V2] procutil: ensure that all stdio file objects are flushed at interpreter exit

Manuel Jacob
On 2020-07-10 13:19, Yuya Nishihara wrote:

> On Fri, 10 Jul 2020 07:11:04 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <[hidden email]>
>> # Date 1594346556 -7200
>> #      Fri Jul 10 04:02:36 2020 +0200
>> # Node ID 284a80ed05e7f4724062c9eeac95933a86257f38
>> # Parent  034feda7f6afcb0ae973569207a8268487793962
>> # EXP-Topic stdio
>> procutil: ensure that all stdio file objects are flushed at
>> interpreter 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.
>
> Yep, but IIUC, the major difference is that the Python interpreter
> needs
> sys.stderr to print traceback, etc. So stderr has to remain open after
> the
> procutil module gets GC-ed. On the other hand, stdout wouldn't have
> such
> problem, and nothing should be printed after dispatch.run(), which
> flushes
> the both streams.

There are various scripts, especially in contrib, that use the sys.std*
streams.

> Suppose 8403cc54bc83 wouldn't make a real difference in our codebase, I
> suggest reverting the change.

There are various small tools using procutils.stderr directly that could
benefit from it being unbuffered (one was referred to in the changeset
description of 8403cc54bc83).

Furthermore, it would enable to remove the manual flush of stderr in the
ui object that works around the fact the procutil.stderr wasn’t
unbuffered before on Windows (and Python 3).
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Manuel Jacob
In reply to this post by Yuya Nishihara
On 2020-07-10 13:14, Yuya Nishihara wrote:

> On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <[hidden email]>
>> # Date 1594118129 -7200
>> #      Tue Jul 07 12:35:29 2020 +0200
>> # Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
>> # Parent  7c5896871c7e9e8e510c0436f5c2bcc6018e8177
>> # EXP-Topic stdio
>> tests: terminate subprocess in test-stdio.py in case of exception
>>
>> If an error happened while reading the output of the subprocess, the
>> pipe / TTY
>> buffer can fill up and prevent that the subprocess ends. Therefore we
>> should
>> terminate the subprocess in case of an exception.
>>
>> diff --git a/tests/test-stdio.py b/tests/test-stdio.py
>> --- a/tests/test-stdio.py
>> +++ b/tests/test-stdio.py
>> @@ -99,6 +99,9 @@
>>                  self.assertEqual(
>>                      _readall(stream_receiver, 1024), expected_output
>>                  )
>> +            except:  # re-raises
>> +                proc.terminate()
>> +                raise
>
> I think assertEqual() should be moved out of the try block.
> AssertionError
> shouldn't be an unexpected error.

I’m generally in favor of making try blocks as small as possible.
However, this particular try block is more like a finally block, except
that it should only be executed if there was an error.

> I've queued the patch 1-3, thanks. The patch 4 might need some rework
> in
> order to move assertEqual() out of the try block.

Upcoming patches will add tests with check_output() that does more than
simply reading the output once. In one particular case, it reads one
byte from the stream, sends a signal, and reads the rest of the bytes.
Moving the assert out of the try block would be possible (although the
code will become less clear), but I wonder if it’s really worth it to
separate interaction and verification if we anyway have more complicated
logic in check_output().
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V2] procutil: ensure that all stdio file objects are flushed at interpreter exit

Yuya Nishihara
In reply to this post by Manuel Jacob
On Fri, 10 Jul 2020 13:55:50 +0200, Manuel Jacob wrote:

> On 2020-07-10 13:19, Yuya Nishihara wrote:
> > On Fri, 10 Jul 2020 07:11:04 +0200, Manuel Jacob wrote:
> >> # HG changeset patch
> >> # User Manuel Jacob <[hidden email]>
> >> # Date 1594346556 -7200
> >> #      Fri Jul 10 04:02:36 2020 +0200
> >> # Node ID 284a80ed05e7f4724062c9eeac95933a86257f38
> >> # Parent  034feda7f6afcb0ae973569207a8268487793962
> >> # EXP-Topic stdio
> >> procutil: ensure that all stdio file objects are flushed at
> >> interpreter 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.
> >
> > Yep, but IIUC, the major difference is that the Python interpreter
> > needs
> > sys.stderr to print traceback, etc. So stderr has to remain open after
> > the
> > procutil module gets GC-ed. On the other hand, stdout wouldn't have
> > such
> > problem, and nothing should be printed after dispatch.run(), which
> > flushes
> > the both streams.
>
> There are various scripts, especially in contrib, that use the sys.std*
> streams.
>
> > Suppose 8403cc54bc83 wouldn't make a real difference in our codebase, I
> > suggest reverting the change.
>
> There are various small tools using procutils.stderr directly that could
> benefit from it being unbuffered (one was referred to in the changeset
> description of 8403cc54bc83).
>
> Furthermore, it would enable to remove the manual flush of stderr in the
> ui object that works around the fact the procutil.stderr wasn’t
> unbuffered before on Windows (and Python 3).

My feeling is that these improvements are far more trivial compared to the
complexity introduced by atexit.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Yuya Nishihara
In reply to this post by Manuel Jacob
On Fri, 10 Jul 2020 14:19:59 +0200, Manuel Jacob wrote:

> On 2020-07-10 13:14, Yuya Nishihara wrote:
> > On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote:
> >> # HG changeset patch
> >> # User Manuel Jacob <[hidden email]>
> >> # Date 1594118129 -7200
> >> #      Tue Jul 07 12:35:29 2020 +0200
> >> # Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
> >> # Parent  7c5896871c7e9e8e510c0436f5c2bcc6018e8177
> >> # EXP-Topic stdio
> >> tests: terminate subprocess in test-stdio.py in case of exception
> >>
> >> If an error happened while reading the output of the subprocess, the
> >> pipe / TTY
> >> buffer can fill up and prevent that the subprocess ends. Therefore we
> >> should
> >> terminate the subprocess in case of an exception.
> >>
> >> diff --git a/tests/test-stdio.py b/tests/test-stdio.py
> >> --- a/tests/test-stdio.py
> >> +++ b/tests/test-stdio.py
> >> @@ -99,6 +99,9 @@
> >>                  self.assertEqual(
> >>                      _readall(stream_receiver, 1024), expected_output
> >>                  )
> >> +            except:  # re-raises
> >> +                proc.terminate()
> >> +                raise
> >
> > I think assertEqual() should be moved out of the try block.
> > AssertionError
> > shouldn't be an unexpected error.
>
> I’m generally in favor of making try blocks as small as possible.
> However, this particular try block is more like a finally block, except
> that it should only be executed if there was an error.
>
> > I've queued the patch 1-3, thanks. The patch 4 might need some rework
> > in
> > order to move assertEqual() out of the try block.
>
> Upcoming patches will add tests with check_output() that does more than
> simply reading the output once. In one particular case, it reads one
> byte from the stream, sends a signal, and reads the rest of the bytes.
> Moving the assert out of the try block would be possible (although the
> code will become less clear), but I wonder if it’s really worth it to
> separate interaction and verification if we anyway have more complicated
> logic in check_output().

Okay. Adding "except AssertionError: _readall(...); ...; raise" should also
be fine.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V2] procutil: ensure that all stdio file objects are flushed at interpreter exit

Manuel Jacob
In reply to this post by Yuya Nishihara
On 2020-07-10 15:16, Yuya Nishihara wrote:

> On Fri, 10 Jul 2020 13:55:50 +0200, Manuel Jacob wrote:
>> On 2020-07-10 13:19, Yuya Nishihara wrote:
>> > On Fri, 10 Jul 2020 07:11:04 +0200, Manuel Jacob wrote:
>> >> # HG changeset patch
>> >> # User Manuel Jacob <[hidden email]>
>> >> # Date 1594346556 -7200
>> >> #      Fri Jul 10 04:02:36 2020 +0200
>> >> # Node ID 284a80ed05e7f4724062c9eeac95933a86257f38
>> >> # Parent  034feda7f6afcb0ae973569207a8268487793962
>> >> # EXP-Topic stdio
>> >> procutil: ensure that all stdio file objects are flushed at
>> >> interpreter 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.
>> >
>> > Yep, but IIUC, the major difference is that the Python interpreter
>> > needs
>> > sys.stderr to print traceback, etc. So stderr has to remain open after
>> > the
>> > procutil module gets GC-ed. On the other hand, stdout wouldn't have
>> > such
>> > problem, and nothing should be printed after dispatch.run(), which
>> > flushes
>> > the both streams.
>>
>> There are various scripts, especially in contrib, that use the
>> sys.std*
>> streams.
>>
>> > Suppose 8403cc54bc83 wouldn't make a real difference in our codebase, I
>> > suggest reverting the change.
>>
>> There are various small tools using procutils.stderr directly that
>> could
>> benefit from it being unbuffered (one was referred to in the changeset
>> description of 8403cc54bc83).
>>
>> Furthermore, it would enable to remove the manual flush of stderr in
>> the
>> ui object that works around the fact the procutil.stderr wasn’t
>> unbuffered before on Windows (and Python 3).
>
> My feeling is that these improvements are far more trivial compared to
> the
> complexity introduced by atexit.

I’m uncomfortable with the idea of having a block-buffered
mercurial.utils.procutils.stderr on Python 3 and Windows. We should
abstract the platform differences as much as possible. However, it’s
worse to introduce regressions.

I think that we anyway need to have the complexity introduced by atexit,
to flush stdout. But as you mentioned, sys.stderr is sometimes used by
the Python interpreter itself to print stuff; that could even be after
the atexit hook.

There seems to be some weak consensus that Python 2 support should be
dropped for 5.6. Therefore, I would be fine with backing out
8403cc54bc83 (make mercurial.utils.procutil.stderr unbuffered) for now,
and reapply a Python 3-only solution (that doesn’t need the atexit hook)
once Python 2 support was dropped.

What do you think? I might decide to send a patch for backing out
8403cc54bc83 later today. Feel free to go ahead and apply it if you
think it’s a good idea.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Manuel Jacob
In reply to this post by Yuya Nishihara
On 2020-07-10 15:17, Yuya Nishihara wrote:

> On Fri, 10 Jul 2020 14:19:59 +0200, Manuel Jacob wrote:
>> On 2020-07-10 13:14, Yuya Nishihara wrote:
>> > On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote:
>> >> # HG changeset patch
>> >> # User Manuel Jacob <[hidden email]>
>> >> # Date 1594118129 -7200
>> >> #      Tue Jul 07 12:35:29 2020 +0200
>> >> # Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
>> >> # Parent  7c5896871c7e9e8e510c0436f5c2bcc6018e8177
>> >> # EXP-Topic stdio
>> >> tests: terminate subprocess in test-stdio.py in case of exception
>> >>
>> >> If an error happened while reading the output of the subprocess, the
>> >> pipe / TTY
>> >> buffer can fill up and prevent that the subprocess ends. Therefore we
>> >> should
>> >> terminate the subprocess in case of an exception.
>> >>
>> >> diff --git a/tests/test-stdio.py b/tests/test-stdio.py
>> >> --- a/tests/test-stdio.py
>> >> +++ b/tests/test-stdio.py
>> >> @@ -99,6 +99,9 @@
>> >>                  self.assertEqual(
>> >>                      _readall(stream_receiver, 1024), expected_output
>> >>                  )
>> >> +            except:  # re-raises
>> >> +                proc.terminate()
>> >> +                raise
>> >
>> > I think assertEqual() should be moved out of the try block.
>> > AssertionError
>> > shouldn't be an unexpected error.
>>
>> I’m generally in favor of making try blocks as small as possible.
>> However, this particular try block is more like a finally block,
>> except
>> that it should only be executed if there was an error.
>>
>> > I've queued the patch 1-3, thanks. The patch 4 might need some rework
>> > in
>> > order to move assertEqual() out of the try block.
>>
>> Upcoming patches will add tests with check_output() that does more
>> than
>> simply reading the output once. In one particular case, it reads one
>> byte from the stream, sends a signal, and reads the rest of the bytes.
>> Moving the assert out of the try block would be possible (although the
>> code will become less clear), but I wonder if it’s really worth it to
>> separate interaction and verification if we anyway have more
>> complicated
>> logic in check_output().
>
> Okay. Adding "except AssertionError: _readall(...); ...; raise" should
> also
> be fine.

I’m not sure why we should make a difference between AssertionError and
other errors. If there’s any error (expected or unexpected), we should
try to terminate hanging subprocesses.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Yuya Nishihara
On Fri, 10 Jul 2020 23:39:16 +0200, Manuel Jacob wrote:

> On 2020-07-10 15:17, Yuya Nishihara wrote:
> > On Fri, 10 Jul 2020 14:19:59 +0200, Manuel Jacob wrote:
> >> On 2020-07-10 13:14, Yuya Nishihara wrote:
> >> > On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote:
> >> >> # HG changeset patch
> >> >> # User Manuel Jacob <[hidden email]>
> >> >> # Date 1594118129 -7200
> >> >> #      Tue Jul 07 12:35:29 2020 +0200
> >> >> # Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
> >> >> # Parent  7c5896871c7e9e8e510c0436f5c2bcc6018e8177
> >> >> # EXP-Topic stdio
> >> >> tests: terminate subprocess in test-stdio.py in case of exception
> >> >>
> >> >> If an error happened while reading the output of the subprocess, the
> >> >> pipe / TTY
> >> >> buffer can fill up and prevent that the subprocess ends. Therefore we
> >> >> should
> >> >> terminate the subprocess in case of an exception.
> >> >>
> >> >> diff --git a/tests/test-stdio.py b/tests/test-stdio.py
> >> >> --- a/tests/test-stdio.py
> >> >> +++ b/tests/test-stdio.py
> >> >> @@ -99,6 +99,9 @@
> >> >>                  self.assertEqual(
> >> >>                      _readall(stream_receiver, 1024), expected_output
> >> >>                  )
> >> >> +            except:  # re-raises
> >> >> +                proc.terminate()
> >> >> +                raise
> >> >
> >> > I think assertEqual() should be moved out of the try block.
> >> > AssertionError
> >> > shouldn't be an unexpected error.
> >>
> >> I’m generally in favor of making try blocks as small as possible.
> >> However, this particular try block is more like a finally block,
> >> except
> >> that it should only be executed if there was an error.
> >>
> >> > I've queued the patch 1-3, thanks. The patch 4 might need some rework
> >> > in
> >> > order to move assertEqual() out of the try block.
> >>
> >> Upcoming patches will add tests with check_output() that does more
> >> than
> >> simply reading the output once. In one particular case, it reads one
> >> byte from the stream, sends a signal, and reads the rest of the bytes.
> >> Moving the assert out of the try block would be possible (although the
> >> code will become less clear), but I wonder if it’s really worth it to
> >> separate interaction and verification if we anyway have more
> >> complicated
> >> logic in check_output().
> >
> > Okay. Adding "except AssertionError: _readall(...); ...; raise" should
> > also
> > be fine.
>
> I’m not sure why we should make a difference between AssertionError and
> other errors. If there’s any error (expected or unexpected), we should
> try to terminate hanging subprocesses.

Because AssertionError isn't an error in unittest context, I feel it's weird
to catch it as an exception and kill the process. But yeah, it's just a
cosmetic issue.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V2] procutil: ensure that all stdio file objects are flushed at interpreter exit

Yuya Nishihara
In reply to this post by Manuel Jacob
On Fri, 10 Jul 2020 23:27:39 +0200, Manuel Jacob wrote:

> On 2020-07-10 15:16, Yuya Nishihara wrote:
> > On Fri, 10 Jul 2020 13:55:50 +0200, Manuel Jacob wrote:
> >> On 2020-07-10 13:19, Yuya Nishihara wrote:
> >> > On Fri, 10 Jul 2020 07:11:04 +0200, Manuel Jacob wrote:
> >> >> # HG changeset patch
> >> >> # User Manuel Jacob <[hidden email]>
> >> >> # Date 1594346556 -7200
> >> >> #      Fri Jul 10 04:02:36 2020 +0200
> >> >> # Node ID 284a80ed05e7f4724062c9eeac95933a86257f38
> >> >> # Parent  034feda7f6afcb0ae973569207a8268487793962
> >> >> # EXP-Topic stdio
> >> >> procutil: ensure that all stdio file objects are flushed at
> >> >> interpreter 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.
> >> >
> >> > Yep, but IIUC, the major difference is that the Python interpreter
> >> > needs
> >> > sys.stderr to print traceback, etc. So stderr has to remain open after
> >> > the
> >> > procutil module gets GC-ed. On the other hand, stdout wouldn't have
> >> > such
> >> > problem, and nothing should be printed after dispatch.run(), which
> >> > flushes
> >> > the both streams.
> >>
> >> There are various scripts, especially in contrib, that use the
> >> sys.std*
> >> streams.
> >>
> >> > Suppose 8403cc54bc83 wouldn't make a real difference in our codebase, I
> >> > suggest reverting the change.
> >>
> >> There are various small tools using procutils.stderr directly that
> >> could
> >> benefit from it being unbuffered (one was referred to in the changeset
> >> description of 8403cc54bc83).
> >>
> >> Furthermore, it would enable to remove the manual flush of stderr in
> >> the
> >> ui object that works around the fact the procutil.stderr wasn’t
> >> unbuffered before on Windows (and Python 3).
> >
> > My feeling is that these improvements are far more trivial compared to
> > the
> > complexity introduced by atexit.
>
> I’m uncomfortable with the idea of having a block-buffered
> mercurial.utils.procutils.stderr on Python 3 and Windows. We should
> abstract the platform differences as much as possible. However, it’s
> worse to introduce regressions.

I think buffered stderr is acceptable option as we've worked around the
Windows issues for years.

> I think that we anyway need to have the complexity introduced by atexit,
> to flush stdout. But as you mentioned, sys.stderr is sometimes used by
> the Python interpreter itself to print stuff; that could even be after
> the atexit hook.
>
> There seems to be some weak consensus that Python 2 support should be
> dropped for 5.6. Therefore, I would be fine with backing out
> 8403cc54bc83 (make mercurial.utils.procutil.stderr unbuffered) for now,
> and reapply a Python 3-only solution (that doesn’t need the atexit hook)
> once Python 2 support was dropped.

Sounds reasonable.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Manuel Jacob
In reply to this post by Yuya Nishihara
On 2020-07-11 04:28, Yuya Nishihara wrote:

> On Fri, 10 Jul 2020 23:39:16 +0200, Manuel Jacob wrote:
>> On 2020-07-10 15:17, Yuya Nishihara wrote:
>> > On Fri, 10 Jul 2020 14:19:59 +0200, Manuel Jacob wrote:
>> >> On 2020-07-10 13:14, Yuya Nishihara wrote:
>> >> > On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote:
>> >> >> # HG changeset patch
>> >> >> # User Manuel Jacob <[hidden email]>
>> >> >> # Date 1594118129 -7200
>> >> >> #      Tue Jul 07 12:35:29 2020 +0200
>> >> >> # Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b
>> >> >> # Parent  7c5896871c7e9e8e510c0436f5c2bcc6018e8177
>> >> >> # EXP-Topic stdio
>> >> >> tests: terminate subprocess in test-stdio.py in case of exception
>> >> >>
>> >> >> If an error happened while reading the output of the subprocess, the
>> >> >> pipe / TTY
>> >> >> buffer can fill up and prevent that the subprocess ends. Therefore we
>> >> >> should
>> >> >> terminate the subprocess in case of an exception.
>> >> >>
>> >> >> diff --git a/tests/test-stdio.py b/tests/test-stdio.py
>> >> >> --- a/tests/test-stdio.py
>> >> >> +++ b/tests/test-stdio.py
>> >> >> @@ -99,6 +99,9 @@
>> >> >>                  self.assertEqual(
>> >> >>                      _readall(stream_receiver, 1024), expected_output
>> >> >>                  )
>> >> >> +            except:  # re-raises
>> >> >> +                proc.terminate()
>> >> >> +                raise
>> >> >
>> >> > I think assertEqual() should be moved out of the try block.
>> >> > AssertionError
>> >> > shouldn't be an unexpected error.
>> >>
>> >> I’m generally in favor of making try blocks as small as possible.
>> >> However, this particular try block is more like a finally block,
>> >> except
>> >> that it should only be executed if there was an error.
>> >>
>> >> > I've queued the patch 1-3, thanks. The patch 4 might need some rework
>> >> > in
>> >> > order to move assertEqual() out of the try block.
>> >>
>> >> Upcoming patches will add tests with check_output() that does more
>> >> than
>> >> simply reading the output once. In one particular case, it reads one
>> >> byte from the stream, sends a signal, and reads the rest of the bytes.
>> >> Moving the assert out of the try block would be possible (although the
>> >> code will become less clear), but I wonder if it’s really worth it to
>> >> separate interaction and verification if we anyway have more
>> >> complicated
>> >> logic in check_output().
>> >
>> > Okay. Adding "except AssertionError: _readall(...); ...; raise" should
>> > also
>> > be fine.
>>
>> I’m not sure why we should make a difference between AssertionError
>> and
>> other errors. If there’s any error (expected or unexpected), we should
>> try to terminate hanging subprocesses.
>
> Because AssertionError isn't an error in unittest context, I feel it's
> weird
> to catch it as an exception and kill the process. But yeah, it's just a
> cosmetic issue.

Yes, I understand that AssertionError will be shown as a "failure",
while other exceptions will be shown as an "error". But I don’t
understand why that should make a difference for killing the process.

The only cosmetic issue I found is that in the patch description I wrote
"error", while it should be "exception". ;)
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

Yuya Nishihara
On Sat, 11 Jul 2020 06:26:14 +0200, Manuel Jacob wrote:

> On 2020-07-11 04:28, Yuya Nishihara wrote:
> > On Fri, 10 Jul 2020 23:39:16 +0200, Manuel Jacob wrote:
> >> I’m not sure why we should make a difference between AssertionError
> >> and
> >> other errors. If there’s any error (expected or unexpected), we should
> >> try to terminate hanging subprocesses.
> >
> > Because AssertionError isn't an error in unittest context, I feel it's
> > weird
> > to catch it as an exception and kill the process. But yeah, it's just a
> > cosmetic issue.
>
> Yes, I understand that AssertionError will be shown as a "failure",
> while other exceptions will be shown as an "error". But I don’t
> understand why that should make a difference for killing the process.

Maybe because I see it is the last option to recover from the hang.
If we know the process is still alive, proc.terminate() should be perfectly
fine.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel