From 1f8e9a49c88dc057c577e60e56564cddd98e03b8 Mon Sep 17 00:00:00 2001 From: Guillermo Rodriguez Date: Tue, 26 May 2020 22:16:09 -0300 Subject: [PATCH 1/2] Started work on issue1532 --- trio/_subprocess.py | 38 +++++++++++++++++------------------ trio/tests/test_subprocess.py | 2 +- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/trio/_subprocess.py b/trio/_subprocess.py index ac51915eb0..689dc4b761 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -1,5 +1,6 @@ import os import subprocess +import logging import sys from typing import Optional from functools import partial @@ -381,28 +382,20 @@ async def open_process( async def _windows_deliver_cancel(p): - try: - p.terminate() - except OSError as exc: - warnings.warn(RuntimeWarning(f"TerminateProcess on {p!r} failed with: {exc!r}")) + p.terminate() async def _posix_deliver_cancel(p): - try: - p.terminate() - await trio.sleep(5) - warnings.warn( - RuntimeWarning( - f"process {p!r} ignored SIGTERM for 5 seconds. " - f"(Maybe you should pass a custom deliver_cancel?) " - f"Trying SIGKILL." - ) - ) - p.kill() - except OSError as exc: - warnings.warn( - RuntimeWarning(f"tried to kill process {p!r}, but failed with: {exc!r}") + p.terminate() + await trio.sleep(5) + warnings.warn( + RuntimeWarning( + f"process {p!r} ignored SIGTERM for 5 seconds. " + f"(Maybe you should pass a custom deliver_cancel?) " + f"Trying SIGKILL." ) + ) + p.kill() async def run_process( @@ -621,7 +614,14 @@ async def read_output(stream, chunks): async def killer(): with killer_cscope: - await deliver_cancel(proc) + try: + await deliver_cancel(proc) + except BaseException as exc: + LOGGER = logging.getLogger("trio.run_process") + LOGGER.exception( + f"tried to kill process {proc!r}, but failed with: {exc!r}" + ) + raise nursery.start_soon(killer) await proc.wait() diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 17f1cb941c..7d9fdb0363 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -464,7 +464,7 @@ def broken_terminate(self): monkeypatch.setattr(Process, "terminate", broken_terminate) - with pytest.warns(RuntimeWarning, match=".*whoops.*"): + with pytest.raises(OSError, match=".*whoops.*"): async with _core.open_nursery() as nursery: nursery.start_soon(run_process, SLEEP(9999)) await wait_all_tasks_blocked() From e3b27634e7b9f83b37e852a67967889ee0dc1fda Mon Sep 17 00:00:00 2001 From: Guillermo Rodriguez Date: Thu, 4 Jun 2020 10:34:04 -0300 Subject: [PATCH 2/2] Repalced context managed process with simple open_process call, added newsfragment, applied black formatter. --- newsfragments/1532.misc.rst | 1 + trio/_subprocess.py | 80 +++++++++++++++++------------------ trio/tests/test_subprocess.py | 2 +- 3 files changed, 42 insertions(+), 41 deletions(-) create mode 100644 newsfragments/1532.misc.rst diff --git a/newsfragments/1532.misc.rst b/newsfragments/1532.misc.rst new file mode 100644 index 0000000000..65258b0b4b --- /dev/null +++ b/newsfragments/1532.misc.rst @@ -0,0 +1 @@ +In `run_process` moved `deliver_cancel` exception handling to the caller code. \ No newline at end of file diff --git a/trio/_subprocess.py b/trio/_subprocess.py index 689dc4b761..76c9f543b1 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -585,48 +585,48 @@ async def my_deliver_cancel(process): stdout_chunks = [] stderr_chunks = [] - async with await open_process(command, **options) as proc: - - async def feed_input(): - async with proc.stdin: - try: - await proc.stdin.send_all(input) - except trio.BrokenResourceError: - pass - - async def read_output(stream, chunks): - async with stream: - async for chunk in stream: - chunks.append(chunk) - - async with trio.open_nursery() as nursery: - if proc.stdin is not None: - nursery.start_soon(feed_input) - if proc.stdout is not None: - nursery.start_soon(read_output, proc.stdout, stdout_chunks) - if proc.stderr is not None: - nursery.start_soon(read_output, proc.stderr, stderr_chunks) + proc = await open_process(command, **options) + + async def feed_input(): + async with proc.stdin: try: + await proc.stdin.send_all(input) + except trio.BrokenResourceError: + pass + + async def read_output(stream, chunks): + async with stream: + async for chunk in stream: + chunks.append(chunk) + + async with trio.open_nursery() as nursery: + if proc.stdin is not None: + nursery.start_soon(feed_input) + if proc.stdout is not None: + nursery.start_soon(read_output, proc.stdout, stdout_chunks) + if proc.stderr is not None: + nursery.start_soon(read_output, proc.stderr, stderr_chunks) + try: + await proc.wait() + except trio.Cancelled: + with trio.CancelScope(shield=True): + killer_cscope = trio.CancelScope(shield=True) + + async def killer(): + with killer_cscope: + try: + await deliver_cancel(proc) + except BaseException as exc: + LOGGER = logging.getLogger("trio.run_process") + LOGGER.exception( + f"tried to kill process {proc!r}, but failed with: {exc!r}" + ) + raise + + nursery.start_soon(killer) await proc.wait() - except trio.Cancelled: - with trio.CancelScope(shield=True): - killer_cscope = trio.CancelScope(shield=True) - - async def killer(): - with killer_cscope: - try: - await deliver_cancel(proc) - except BaseException as exc: - LOGGER = logging.getLogger("trio.run_process") - LOGGER.exception( - f"tried to kill process {proc!r}, but failed with: {exc!r}" - ) - raise - - nursery.start_soon(killer) - await proc.wait() - killer_cscope.cancel() - raise + killer_cscope.cancel() + raise stdout = b"".join(stdout_chunks) if proc.stdout is not None else None stderr = b"".join(stderr_chunks) if proc.stderr is not None else None diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 7d9fdb0363..d6ad04a77e 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -455,7 +455,7 @@ async def custom_deliver_cancel(proc): assert custom_deliver_cancel_called -async def test_warn_on_failed_cancel_terminate(monkeypatch): +async def test_log_on_failed_cancel_terminate(monkeypatch): original_terminate = Process.terminate def broken_terminate(self):