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

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

Comments

Manuel Jacob - July 10, 2020, 4:46 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# 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.
Yuya Nishihara - July 10, 2020, 11:14 a.m.
On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # 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.
Manuel Jacob - July 10, 2020, 12:19 p.m.
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 <me@manueljacob.de>
>> # 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().
Yuya Nishihara - July 10, 2020, 1:17 p.m.
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 <me@manueljacob.de>
> >> # 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.
Manuel Jacob - July 10, 2020, 9:39 p.m.
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 <me@manueljacob.de>
>> >> # 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.
Yuya Nishihara - July 11, 2020, 2:28 a.m.
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 <me@manueljacob.de>
> >> >> # 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.
Manuel Jacob - July 11, 2020, 4:26 a.m.
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 <me@manueljacob.de>
>> >> >> # 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". ;)
Yuya Nishihara - July 11, 2020, 12:13 p.m.
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.

Patch

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)