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 ac51915eb0..76c9f543b1 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( @@ -592,41 +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.wait() - except trio.Cancelled: - with trio.CancelScope(shield=True): - killer_cscope = trio.CancelScope(shield=True) + await proc.stdin.send_all(input) + except trio.BrokenResourceError: + pass - async def killer(): - with killer_cscope: + 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) - - nursery.start_soon(killer) - await proc.wait() - killer_cscope.cancel() - raise + 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 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 17f1cb941c..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): @@ -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()