From 356db30e901fcde82b8fd0acdd3c109ca61e2156 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Mon, 1 Jun 2020 19:37:43 -0700 Subject: [PATCH 01/16] First pass at implementing nursery.start(run_process, ...) --- trio/_subprocess.py | 72 ++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/trio/_subprocess.py b/trio/_subprocess.py index 876cc0d7c9..886e63edc4 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -424,6 +424,7 @@ async def run_process( capture_stderr=False, check=True, deliver_cancel=None, + task_status=trio.TASK_STATUS_IGNORED, **options, ): """Run ``command`` in a subprocess, wait for it to complete, and @@ -568,7 +569,7 @@ async def my_deliver_cancel(process): if isinstance(stdin, str): raise UnicodeError("process stdin must be bytes, not str") - if stdin == subprocess.PIPE: + if stdin == subprocess.PIPE and task_status is trio.TASK_STATUS_IGNORED: raise ValueError( "stdin=subprocess.PIPE doesn't make sense since the pipe " "is internal to run_process(); pass the actual data you " @@ -603,44 +604,49 @@ 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 feed_input(stream): + async with stream: + try: + await stream.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 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: + async with trio.open_nursery() as nursery: + proc = await open_process(command, **options) + try: + if input is not None: + nursery.start_soon(feed_input, proc.stdin) + proc.stdin = None + proc.stdio = None + if capture_stdout: nursery.start_soon(read_output, proc.stdout, stdout_chunks) - if proc.stderr is not None: + proc.stdout = None + proc.stdio = None + if capture_stderr: nursery.start_soon(read_output, proc.stderr, stderr_chunks) - try: + proc.stderr = None + task_status.started(proc) + await proc.wait() + except BaseException: + with trio.CancelScope(shield=True): + killer_cscope = trio.CancelScope(shield=True) + + async def killer(): + with killer_cscope: + await deliver_cancel(proc) + + 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: - await deliver_cancel(proc) - - 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 + stdout = b"".join(stdout_chunks) if capture_stdout else None + stderr = b"".join(stderr_chunks) if capture_stderr else None if proc.returncode and check: raise subprocess.CalledProcessError( From 0734cad2ce7de8d1f0b710b79ea5ed9cdbec6eae Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 2 Jun 2020 00:10:32 -0700 Subject: [PATCH 02/16] Deprecate Process.aclose and 'async with Process' --- docs/source/reference-io.rst | 2 - newsfragments/1104.removal.rst | 4 + trio/_subprocess.py | 34 ++++++--- trio/tests/test_subprocess.py | 131 +++++++++++++++++++++------------ 4 files changed, 109 insertions(+), 62 deletions(-) create mode 100644 newsfragments/1104.removal.rst diff --git a/docs/source/reference-io.rst b/docs/source/reference-io.rst index ef971f6ea8..a0b8c77879 100644 --- a/docs/source/reference-io.rst +++ b/docs/source/reference-io.rst @@ -735,8 +735,6 @@ with it using the `Process` interface. .. autoattribute:: returncode - .. automethod:: aclose - .. automethod:: wait .. automethod:: poll diff --git a/newsfragments/1104.removal.rst b/newsfragments/1104.removal.rst new file mode 100644 index 0000000000..0ffb50fd3f --- /dev/null +++ b/newsfragments/1104.removal.rst @@ -0,0 +1,4 @@ +The `~Process.aclose` method on `Process` has been deprecated, and +``async with process_obj`` has also been deprecated. We recommend +switching to the new ``nursery.start(trio.run_process, ...)`` API +instead, which is more powerful and predictable. diff --git a/trio/_subprocess.py b/trio/_subprocess.py index 886e63edc4..a4c79a2b8f 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -16,6 +16,7 @@ create_pipe_to_child_stdin, create_pipe_from_child_output, ) +from ._deprecate import deprecated from ._util import NoPublicConstructor import trio @@ -66,22 +67,19 @@ def pidfd_open(fd: int, flags: int) -> int: class Process(AsyncResource, metaclass=NoPublicConstructor): r"""A child process. Like :class:`subprocess.Popen`, but async. - This class has no public constructor. To create a child process, use - `open_process`:: + This class has no public constructor. The most common way to create a + `Process` is to combine `Nursery.start` with `run_process`:: - process = await trio.open_process(...) + process = await nursery.start(run_process, ...) - `Process` implements the `~trio.abc.AsyncResource` interface. In order to - make sure your process doesn't end up getting abandoned by mistake or - after an exception, you can use ``async with``:: + This way, `run_process` supervises the process, and makes sure that it is + cleaned up, while optionally checking the output, feeding it input, and so + on. - async with await trio.open_process(...) as process: - ... + If you need more control – for example, because you want to spawn a child + process that outlives your program – then you can use `open_process`:: - "Closing" a :class:`Process` will close any pipes to the child and wait - for it to exit; if cancelled, the child will be forcibly killed and we - will ensure it has finished exiting before allowing the cancellation to - propagate. + process = await trio.open_process(...) Attributes: args (str or list): The ``command`` passed at construction time, @@ -180,6 +178,18 @@ def returncode(self): self._close_pidfd() return result + @deprecated( + "0.16.0", + thing="using trio.Process as an async context manager", + issue=1104, + instead="run_process or nursery.start(run_process, ...)", + ) + async def __aenter__(self): + return self + + @deprecated( + "0.16.0", issue=1104, instead="run_process or nursery.start(run_process, ...)" + ) async def aclose(self): """Close any pipes we have to the process (both input and output) and wait for it to exit. diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 7ba794a428..1a5cf17196 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -5,6 +5,7 @@ import pytest import random from functools import partial +from async_generator import asynccontextmanager from .. import ( _core, @@ -16,6 +17,7 @@ open_process, run_process, TrioDeprecationWarning, + ClosedResourceError, ) from .._core.tests.tutil import slow, skip_if_fbsd_pipes_broken from ..testing import wait_all_tasks_blocked @@ -47,16 +49,25 @@ def got_signal(proc, sig): return proc.returncode != 0 +@asynccontextmanager +async def killing(proc): + try: + yield proc + finally: + proc.kill() + + async def test_basic(): - async with await open_process(EXIT_TRUE) as proc: - pass + async with killing(await open_process(EXIT_TRUE)) as proc: + await proc.wait() assert isinstance(proc, Process) assert proc._pidfd is None assert proc.returncode == 0 assert repr(proc) == f"" - async with await open_process(EXIT_FALSE) as proc: - pass + async with killing(await open_process(EXIT_FALSE)) as proc: + await proc.wait() + await proc.wait() assert proc.returncode == 1 assert repr(proc) == "".format( EXIT_FALSE, "exited with status 1" @@ -64,19 +75,19 @@ async def test_basic(): async def test_auto_update_returncode(): - p = await open_process(SLEEP(9999)) - assert p.returncode is None - assert "running" in repr(p) - p.kill() - p._proc.wait() - assert p.returncode is not None - assert "exited" in repr(p) - assert p._pidfd is None - assert p.returncode is not None + async with killing(await open_process(SLEEP(9999))) as p: + assert p.returncode is None + assert "running" in repr(p) + p.kill() + p._proc.wait() + assert p.returncode is not None + assert "exited" in repr(p) + assert p._pidfd is None + assert p.returncode is not None async def test_multi_wait(): - async with await open_process(SLEEP(10)) as proc: + async with killing(await open_process(SLEEP(10))) as proc: # Check that wait (including multi-wait) tolerates being cancelled async with _core.open_nursery() as nursery: nursery.start_soon(proc.wait) @@ -94,7 +105,21 @@ async def test_multi_wait(): proc.kill() -async def test_kill_when_context_cancelled(): +# Test for deprecated 'async with process:' semantics +async def test_async_with_basics_deprecated(recwarn): + async with await open_process( + CAT, stdin=subprocess.PIPE, stdout=subprocess.PIPE + ) as proc: + pass + assert proc.returncode == 0 + with pytest.raises(ClosedResourceError): + await proc.stdin.send_all(b"x") + with pytest.raises(ClosedResourceError): + await proc.stdout.receive_some() + + +# Test for deprecated 'async with process:' semantics +async def test_kill_when_context_cancelled(recwarn): with move_on_after(100) as scope: async with await open_process(SLEEP(10)) as proc: assert proc.poll() is None @@ -115,11 +140,13 @@ async def test_kill_when_context_cancelled(): async def test_pipes(): - async with await open_process( - COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + async with killing( + await open_process( + COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) ) as proc: msg = b"the quick brown fox jumps over the lazy dog" @@ -156,20 +183,22 @@ async def test_interactive(): # out: EOF # err: EOF - async with await open_process( - python( - "idx = 0\n" - "while True:\n" - " line = sys.stdin.readline()\n" - " if line == '': break\n" - " request = int(line.strip())\n" - " print(str(idx * 2) * request)\n" - " print(str(idx * 2 + 1) * request * 2, file=sys.stderr)\n" - " idx += 1\n" - ), - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + async with killing( + await open_process( + python( + "idx = 0\n" + "while True:\n" + " line = sys.stdin.readline()\n" + " if line == '': break\n" + " request = int(line.strip())\n" + " print(str(idx * 2) * request)\n" + " print(str(idx * 2 + 1) * request * 2, file=sys.stderr)\n" + " idx += 1\n" + ), + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) ) as proc: newline = b"\n" if posix else b"\r\n" @@ -279,11 +308,13 @@ async def test_run_with_broken_pipe(): async def test_stderr_stdout(): - async with await open_process( - COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + async with killing( + await open_process( + COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + ) ) as proc: assert proc.stdout is not None assert proc.stderr is None @@ -312,23 +343,26 @@ async def test_stderr_stdout(): # this one hits the branch where stderr=STDOUT but stdout # is not redirected - async with await open_process( - CAT, stdin=subprocess.PIPE, stderr=subprocess.STDOUT + async with killing( + await open_process(CAT, stdin=subprocess.PIPE, stderr=subprocess.STDOUT) ) as proc: assert proc.stdout is None assert proc.stderr is None await proc.stdin.aclose() + await proc.wait() assert proc.returncode == 0 if posix: try: r, w = os.pipe() - async with await open_process( - COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, - stdin=subprocess.PIPE, - stdout=w, - stderr=subprocess.STDOUT, + async with killing( + await open_process( + COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, + stdin=subprocess.PIPE, + stdout=w, + stderr=subprocess.STDOUT, + ) ) as proc: os.close(w) assert proc.stdio is None @@ -359,8 +393,9 @@ async def test_errors(): async def test_signals(): async def test_one_signal(send_it, signum): with move_on_after(1.0) as scope: - async with await open_process(SLEEP(3600)) as proc: + async with killing(await open_process(SLEEP(3600))) as proc: send_it(proc) + await proc.wait() assert not scope.cancelled_caught if posix: assert proc.returncode == -signum @@ -387,7 +422,7 @@ async def test_wait_reapable_fails(): # With SIGCHLD disabled, the wait() syscall will wait for the # process to exit but then fail with ECHILD. Make sure we # support this case as the stdlib subprocess module does. - async with await open_process(SLEEP(3600)) as proc: + async with killing(await open_process(SLEEP(3600))) as proc: async with _core.open_nursery() as nursery: nursery.start_soon(proc.wait) await wait_all_tasks_blocked() From de51f9c11f8fd25e5130d586d6ae038963c78548 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 2 Jun 2020 17:35:25 -0700 Subject: [PATCH 03/16] Attempt to fix CI failures --- newsfragments/1104.removal.rst | 8 ++++---- trio/tests/test_subprocess.py | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/newsfragments/1104.removal.rst b/newsfragments/1104.removal.rst index 0ffb50fd3f..406230ed08 100644 --- a/newsfragments/1104.removal.rst +++ b/newsfragments/1104.removal.rst @@ -1,4 +1,4 @@ -The `~Process.aclose` method on `Process` has been deprecated, and -``async with process_obj`` has also been deprecated. We recommend -switching to the new ``nursery.start(trio.run_process, ...)`` API -instead, which is more powerful and predictable. +The ``aclose`` method on `Process` has been deprecated, and ``async +with process_obj`` has also been deprecated. We recommend switching to +the new ``nursery.start(trio.run_process, ...)`` API instead, which is +more powerful and predictable. diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 1a5cf17196..27a2e1985d 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -111,7 +111,7 @@ async def test_async_with_basics_deprecated(recwarn): CAT, stdin=subprocess.PIPE, stdout=subprocess.PIPE ) as proc: pass - assert proc.returncode == 0 + assert proc.returncode is not None with pytest.raises(ClosedResourceError): await proc.stdin.send_all(b"x") with pytest.raises(ClosedResourceError): @@ -238,6 +238,7 @@ async def drain_one(stream, count, digit): await proc.stdin.aclose() assert await proc.stdout.receive_some(1) == b"" assert await proc.stderr.receive_some(1) == b"" + await proc.wait() assert proc.returncode == 0 From 3fae5aa84f43dfcad1b13604f1ceadf58b96c6f6 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Fri, 5 Jun 2020 09:44:02 -0700 Subject: [PATCH 04/16] Redo docs --- docs/source/reference-io.rst | 99 +++++++++--------------- trio/_subprocess.py | 145 ++++++++++++++++++++++++----------- 2 files changed, 135 insertions(+), 109 deletions(-) diff --git a/docs/source/reference-io.rst b/docs/source/reference-io.rst index a0b8c77879..77ae4cbd88 100644 --- a/docs/source/reference-io.rst +++ b/docs/source/reference-io.rst @@ -664,22 +664,43 @@ Spawning subprocesses Trio provides support for spawning other programs as subprocesses, communicating with them via pipes, sending them signals, and waiting -for them to exit. The interface for doing so consists of two layers: +for them to exit. -* :func:`trio.run_process` runs a process from start to - finish and returns a :class:`~subprocess.CompletedProcess` object describing - its outputs and return value. This is what you should reach for if you - want to run a process to completion before continuing, while possibly - sending it some input or capturing its output. It is modelled after - the standard :func:`subprocess.run` with some additional features - and safer defaults. +Most of the time, this is done through our high-level interface, +`trio.run_process`. It lets you either run a process to completion +while optionally capturing the output, or else run it in a background +task and interact with it while it's running: -* `trio.open_process` starts a process in the background and returns a - `Process` object to let you interact with it. Using it requires a - bit more code than `run_process`, but exposes additional - capabilities: back-and-forth communication, processing output as - soon as it is generated, and so forth. It is modelled after the - standard library :class:`subprocess.Popen`. +.. autofunction:: trio.run_process + +.. autoclass:: trio.Process + + .. autoattribute:: returncode + + .. automethod:: wait + + .. automethod:: poll + + .. automethod:: kill + + .. automethod:: terminate + + .. automethod:: send_signal + + .. note:: :meth:`~subprocess.Popen.communicate` is not provided as a + method on :class:`~trio.Process` objects; call :func:`~trio.run_process` + normally for simple capturing, or write the loop yourself if you + have unusual needs. :meth:`~subprocess.Popen.communicate` has + quite unusual cancellation behavior in the standard library (on + some platforms it spawns a background thread which continues to + read from the child process even after the timeout has expired) + and we wanted to provide an interface with fewer surprises. + +If `trio.run_process` is too limiting, we also offer a low-level API, +`trio.open_process`. For example, use `open_process` if you want to +spawn a child process that outlives the parent process: + +.. autofunction:: trio.open_process .. _subprocess-options: @@ -705,56 +726,6 @@ with a process, so it does not support the ``encoding``, ``errors``, options. -Running a process and waiting for it to finish -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -The basic interface for running a subprocess start-to-finish is -:func:`trio.run_process`. It always waits for the subprocess to exit -before returning, so there's no need to worry about leaving a process -running by mistake after you've gone on to do other things. -:func:`~trio.run_process` is similar to the standard library -:func:`subprocess.run` function, but tries to have safer defaults: -with no options, the subprocess's input is empty rather than coming -from the user's terminal, and a failure in the subprocess will be -propagated as a :exc:`subprocess.CalledProcessError` exception. Of -course, these defaults can be changed where necessary. - -.. autofunction:: trio.run_process - - -Interacting with a process as it runs -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -If you want more control than :func:`~trio.run_process` affords, you -can use `trio.open_process` to spawn a subprocess, and then interact -with it using the `Process` interface. - -.. autofunction:: trio.open_process - -.. autoclass:: trio.Process - - .. autoattribute:: returncode - - .. automethod:: wait - - .. automethod:: poll - - .. automethod:: kill - - .. automethod:: terminate - - .. automethod:: send_signal - - .. note:: :meth:`~subprocess.Popen.communicate` is not provided as a - method on :class:`~trio.Process` objects; use :func:`~trio.run_process` - instead, or write the loop yourself if you have unusual - needs. :meth:`~subprocess.Popen.communicate` has quite unusual - cancellation behavior in the standard library (on some platforms it - spawns a background thread which continues to read from the child - process even after the timeout has expired) and we wanted to - provide an interface with fewer surprises. - - .. _subprocess-quoting: Quoting: more than you wanted to know diff --git a/trio/_subprocess.py b/trio/_subprocess.py index a4c79a2b8f..aab9b84266 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -67,19 +67,20 @@ def pidfd_open(fd: int, flags: int) -> int: class Process(AsyncResource, metaclass=NoPublicConstructor): r"""A child process. Like :class:`subprocess.Popen`, but async. - This class has no public constructor. The most common way to create a - `Process` is to combine `Nursery.start` with `run_process`:: + This class has no public constructor. The most common way to get a + `Process` object is to combine `Nursery.start` with `run_process`:: - process = await nursery.start(run_process, ...) + process_object = await nursery.start(run_process, ...) - This way, `run_process` supervises the process, and makes sure that it is - cleaned up, while optionally checking the output, feeding it input, and so - on. + This way, `run_process` supervises the process and makes sure that it is + cleaned up properly, while optionally checking the return value, feeding + it input, and so on. If you need more control – for example, because you want to spawn a child - process that outlives your program – then you can use `open_process`:: + process that outlives your program – then another option is to use + `open_process`:: - process = await trio.open_process(...) + process_object = await trio.open_process(...) Attributes: args (str or list): The ``command`` passed at construction time, @@ -437,21 +438,39 @@ async def run_process( task_status=trio.TASK_STATUS_IGNORED, **options, ): - """Run ``command`` in a subprocess, wait for it to complete, and - return a :class:`subprocess.CompletedProcess` instance describing - the results. - - If cancelled, :func:`run_process` terminates the subprocess and - waits for it to exit before propagating the cancellation, like - :meth:`Process.aclose`. - - **Input:** The subprocess's standard input stream is set up to - receive the bytes provided as ``stdin``. Once the given input has - been fully delivered, or if none is provided, the subprocess will - receive end-of-file when reading from its standard input. - Alternatively, if you want the subprocess to read its - standard input from the same place as the parent Trio process, you - can pass ``stdin=None``. + """Run ``command`` in a subprocess and wait for it to complete. + + This function can be called in two different ways. + + One option is a direct call, like:: + + completed_process_info = await trio.run_process(...) + + In this case, it returns a :class:`subprocess.CompletedProcess` instance + describing the results. Use this if you want to treat a process like a + function call. + + The other option is to run it as a task using `Nursery.start` – the enhanced version + of `~Nursery.start_soon` that lets a task pass back a value during startup:: + + process = await nursery.start(trio.run_process, ...) + + In this case, `~Nursery.start` returns a `Process` object that you can use + to interact with the process while it's running. Use this if you want to + treat a process like a background task. + + Either way, `run_process` makes sure that the process has exited before + returning, handles cancellation, optionally checks for errors, and + provides some convenient shorthands for dealing with the child's + input/output. + + **Input:** `run_process` supports all the same ``stdin=`` arguments as + `subprocess.Popen`. In addition, if you simply want to pass in some fixed + data, you can pass a plain `bytes` object, and `run_process` will take + care of setting up a pipe, feeding in the data you gave, and then sending + end-of-file. The default is ``b""``, which means that the child will receive + an empty stdin. If you want the child to instead read from the parent's + stdin, use ``stdin=None``. **Output:** By default, any output produced by the subprocess is passed through to the standard output and error streams of the @@ -481,8 +500,28 @@ async def run_process( the :attr:`~subprocess.CalledProcessError.stdout` and :attr:`~subprocess.CalledProcessError.stderr` attributes of that exception. To disable this behavior, so that :func:`run_process` - returns normally even if the subprocess exits abnormally, pass - ``check=False``. + returns normally even if the subprocess exits abnormally, pass ``check=False``. + + Note that this can make the ``capture_stdout`` and ``capture_stderr`` + arguments useful even when starting `run_process` as a task: if you only + care about the output if the process fails, then you can enable capturing + and then read the output off of the `~subprocess.CalledProcessError`. + + **Cancellation:** If cancelled, `run_process` sends a termination + request to the subprocess, then waits for it to fully exit. The + ``deliver_cancel`` argument lets you control how the process is terminated. + + .. note:: `run_process` is intentionally similar to the standard library + `subprocess.run`, but some of the defaults are different. Specifically, we + default to: + + - ``check=True``, because `"errors should never pass silently / unless + explicitly silenced "`__. + + - ``stdin=b""``, because it produces less-confusing results if a subprocess + unexpectedly tries to read from stdin. + + To get the `subprocess.run` semantics, use ``check=False, stdin=None``. Args: command (list or str): The command to run. Typically this is a @@ -493,24 +532,26 @@ async def run_process( be a string, which will be parsed following platform-dependent :ref:`quoting rules `. - stdin (:obj:`bytes`, file descriptor, or None): The bytes to provide to - the subprocess on its standard input stream, or ``None`` if the - subprocess's standard input should come from the same place as - the parent Trio process's standard input. As is the case with - the :mod:`subprocess` module, you can also pass a - file descriptor or an object with a ``fileno()`` method, - in which case the subprocess's standard input will come from - that file. + stdin (:obj:`bytes`, subprocess.PIPE, file descriptor, or None): The + bytes to provide to the subprocess on its standard input stream, or + ``None`` if the subprocess's standard input should come from the + same place as the parent Trio process's standard input. As is the + case with the :mod:`subprocess` module, you can also pass a file + descriptor or an object with a ``fileno()`` method, in which case + the subprocess's standard input will come from that file. And when + starting `run_process` as a background task, you can use + ``stdin=subprocess.PIPE``, in which case `Process.stdin` will be a + `~trio.abc.SendStream` that you can use to send data to the child. capture_stdout (bool): If true, capture the bytes that the subprocess writes to its standard output stream and return them in the - :attr:`~subprocess.CompletedProcess.stdout` attribute - of the returned :class:`~subprocess.CompletedProcess` object. + `~subprocess.CompletedProcess.stdout` attribute of the returned + `subprocess.CompletedProcess` or `subprocess.CalledProcessError`. capture_stderr (bool): If true, capture the bytes that the subprocess writes to its standard error stream and return them in the - :attr:`~subprocess.CompletedProcess.stderr` attribute - of the returned :class:`~subprocess.CompletedProcess` object. + `~subprocess.CompletedProcess.stderr` attribute of the returned + `~subprocess.CompletedProcess` or `subprocess.CalledProcessError`. check (bool): If false, don't validate that the subprocess exits successfully. You should be sure to check the @@ -555,8 +596,11 @@ async def my_deliver_cancel(process): ``stdout=subprocess.DEVNULL``, or file descriptors. Returns: - A :class:`subprocess.CompletedProcess` instance describing the - return code and outputs. + + When called normally – a `subprocess.CompletedProcess` instance + describing the return code and outputs. + + When called via `Nursery.start` – a `trio.Process` instance. Raises: UnicodeError: if ``stdin`` is specified as a Unicode string, rather @@ -579,12 +623,23 @@ async def my_deliver_cancel(process): if isinstance(stdin, str): raise UnicodeError("process stdin must be bytes, not str") - if stdin == subprocess.PIPE and task_status is trio.TASK_STATUS_IGNORED: - raise ValueError( - "stdin=subprocess.PIPE doesn't make sense since the pipe " - "is internal to run_process(); pass the actual data you " - "want to send over that pipe instead" - ) + if task_status is trio.TASK_STATUS_IGNORED: + if stdin is subprocess.PIPE: + raise ValueError( + "stdin=subprocess.PIPE doesn't make sense without " + "nursery.start, since there's no way to access the " + "pipe; pass the data you want to send or use nursery.start" + ) + if options.get("stdout") is subprocess.PIPE: + raise ValueError( + "stdout=subprocess.PIPE doesn't make sense without " + "nursery.start, since there's no way to access the pipe" + ) + if options.get("stderr") is subprocess.PIPE: + raise ValueError( + "stderr=subprocess.PIPE doesn't make sense without " + "nursery.start, since there's no way to access the pipe" + ) if isinstance(stdin, (bytes, bytearray, memoryview)): input = stdin options["stdin"] = subprocess.PIPE From 5bd9600666961c4820d9fffc5d3f9aef2cfd66d1 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 09:24:10 -0700 Subject: [PATCH 05/16] fix ReST syntax --- trio/_subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trio/_subprocess.py b/trio/_subprocess.py index aab9b84266..44ebc0997c 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -516,7 +516,7 @@ async def run_process( default to: - ``check=True``, because `"errors should never pass silently / unless - explicitly silenced "`__. + explicitly silenced" `__. - ``stdin=b""``, because it produces less-confusing results if a subprocess unexpectedly tries to read from stdin. From 14a4b916ec624c77497da8aaae41bff3e8105d82 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 09:27:16 -0700 Subject: [PATCH 06/16] In test helper, wait for process to be fully dead before continuing Fixes some race conditions in tests that make assertions about proc.returncode after exiting the `async with killing` block. --- trio/tests/test_subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 27a2e1985d..2fdd5a2e45 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -55,6 +55,7 @@ async def killing(proc): yield proc finally: proc.kill() + await proc.wait() async def test_basic(): @@ -67,7 +68,6 @@ async def test_basic(): async with killing(await open_process(EXIT_FALSE)) as proc: await proc.wait() - await proc.wait() assert proc.returncode == 1 assert repr(proc) == "".format( EXIT_FALSE, "exited with status 1" From 7e742f310febb925d9428af614e0c690991f0a1f Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 10:59:34 -0700 Subject: [PATCH 07/16] Tests for nursery.start(run_subprocess, ...) --- trio/tests/test_subprocess.py | 109 +++++++++++++++++++++------------- 1 file changed, 68 insertions(+), 41 deletions(-) diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 2fdd5a2e45..86cde0773e 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -50,7 +50,8 @@ def got_signal(proc, sig): @asynccontextmanager -async def killing(proc): +async def open_process_then_kill(*args, **kwargs): + proc = await open_process(*args, **kwargs) try: yield proc finally: @@ -58,15 +59,32 @@ async def killing(proc): await proc.wait() -async def test_basic(): - async with killing(await open_process(EXIT_TRUE)) as proc: +@asynccontextmanager +async def run_process_in_nursery(*args, **kwargs): + async with _core.open_nursery() as nursery: + kwargs.setdefault("check", False) + proc = await nursery.start(partial(run_process, *args, **kwargs)) + yield proc + nursery.cancel_scope.cancel() + + +background_process_param = pytest.mark.parametrize( + "background_process", + [open_process_then_kill, run_process_in_nursery], + ids=["open_process", "run_process in nursery"] +) + + +@background_process_param +async def test_basic(background_process): + async with background_process(EXIT_TRUE) as proc: await proc.wait() assert isinstance(proc, Process) assert proc._pidfd is None assert proc.returncode == 0 assert repr(proc) == f"" - async with killing(await open_process(EXIT_FALSE)) as proc: + async with background_process(EXIT_FALSE) as proc: await proc.wait() assert proc.returncode == 1 assert repr(proc) == "".format( @@ -74,8 +92,9 @@ async def test_basic(): ) -async def test_auto_update_returncode(): - async with killing(await open_process(SLEEP(9999))) as p: +@background_process_param +async def test_auto_update_returncode(background_process): + async with background_process(SLEEP(9999)) as p: assert p.returncode is None assert "running" in repr(p) p.kill() @@ -86,8 +105,9 @@ async def test_auto_update_returncode(): assert p.returncode is not None -async def test_multi_wait(): - async with killing(await open_process(SLEEP(10))) as proc: +@background_process_param +async def test_multi_wait(background_process): + async with background_process(SLEEP(10)) as proc: # Check that wait (including multi-wait) tolerates being cancelled async with _core.open_nursery() as nursery: nursery.start_soon(proc.wait) @@ -139,14 +159,13 @@ async def test_kill_when_context_cancelled(recwarn): ) -async def test_pipes(): - async with killing( - await open_process( - COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) +@background_process_param +async def test_pipes(background_process): + async with background_process( + COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, ) as proc: msg = b"the quick brown fox jumps over the lazy dog" @@ -171,7 +190,8 @@ async def check_output(stream, expected): assert 0 == await proc.wait() -async def test_interactive(): +@background_process_param +async def test_interactive(background_process): # Test some back-and-forth with a subprocess. This one works like so: # in: 32\n # out: 0000...0000\n (32 zeroes) @@ -183,8 +203,7 @@ async def test_interactive(): # out: EOF # err: EOF - async with killing( - await open_process( + async with background_process( python( "idx = 0\n" "while True:\n" @@ -198,7 +217,6 @@ async def test_interactive(): stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, - ) ) as proc: newline = b"\n" if posix else b"\r\n" @@ -239,6 +257,7 @@ async def drain_one(stream, count, digit): assert await proc.stdout.receive_some(1) == b"" assert await proc.stderr.receive_some(1) == b"" await proc.wait() + assert proc.returncode == 0 @@ -308,14 +327,13 @@ async def test_run_with_broken_pipe(): assert result.stdout is result.stderr is None -async def test_stderr_stdout(): - async with killing( - await open_process( - COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - ) +@background_process_param +async def test_stderr_stdout(background_process): + async with background_process( + COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, ) as proc: assert proc.stdout is not None assert proc.stderr is None @@ -344,8 +362,8 @@ async def test_stderr_stdout(): # this one hits the branch where stderr=STDOUT but stdout # is not redirected - async with killing( - await open_process(CAT, stdin=subprocess.PIPE, stderr=subprocess.STDOUT) + async with background_process( + CAT, stdin=subprocess.PIPE, stderr=subprocess.STDOUT ) as proc: assert proc.stdout is None assert proc.stderr is None @@ -357,13 +375,11 @@ async def test_stderr_stdout(): try: r, w = os.pipe() - async with killing( - await open_process( - COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, - stdin=subprocess.PIPE, - stdout=w, - stderr=subprocess.STDOUT, - ) + async with background_process( + COPY_STDIN_TO_STDOUT_AND_BACKWARD_TO_STDERR, + stdin=subprocess.PIPE, + stdout=w, + stderr=subprocess.STDOUT, ) as proc: os.close(w) assert proc.stdio is None @@ -391,10 +407,11 @@ async def test_errors(): await open_process("ls", shell=False) -async def test_signals(): +@background_process_param +async def test_signals(background_process): async def test_one_signal(send_it, signum): with move_on_after(1.0) as scope: - async with killing(await open_process(SLEEP(3600))) as proc: + async with background_process(SLEEP(3600)) as proc: send_it(proc) await proc.wait() assert not scope.cancelled_caught @@ -417,13 +434,14 @@ async def test_one_signal(send_it, signum): @pytest.mark.skipif(not posix, reason="POSIX specific") -async def test_wait_reapable_fails(): +@background_process_param +async def test_wait_reapable_fails(background_process): old_sigchld = signal.signal(signal.SIGCHLD, signal.SIG_IGN) try: # With SIGCHLD disabled, the wait() syscall will wait for the # process to exit but then fail with ECHILD. Make sure we # support this case as the stdlib subprocess module does. - async with killing(await open_process(SLEEP(3600))) as proc: + async with background_process(SLEEP(3600)) as proc: async with _core.open_nursery() as nursery: nursery.start_soon(proc.wait) await wait_all_tasks_blocked() @@ -516,3 +534,12 @@ async def test_warn_on_cancel_SIGKILL_escalation(autojump_clock, monkeypatch): nursery.start_soon(run_process, SLEEP(9999)) await wait_all_tasks_blocked() nursery.cancel_scope.cancel() + + +# the background_process_param exercises a lot of run_process cases, but it uses +# check=False, so lets have a test that uses check=True as well +async def test_run_process_background_fail(): + with pytest.raises(subprocess.CalledProcessError): + async with _core.open_nursery() as nursery: + proc = await nursery.start(run_process, EXIT_FALSE) + assert proc.returncode == 1 From c48123abbe8f8c11b2b4a50dcc3ace000e430905 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 11:02:08 -0700 Subject: [PATCH 08/16] blacken --- trio/tests/test_subprocess.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 86cde0773e..776510d9d5 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -71,7 +71,7 @@ async def run_process_in_nursery(*args, **kwargs): background_process_param = pytest.mark.parametrize( "background_process", [open_process_then_kill, run_process_in_nursery], - ids=["open_process", "run_process in nursery"] + ids=["open_process", "run_process in nursery"], ) @@ -204,19 +204,19 @@ async def test_interactive(background_process): # err: EOF async with background_process( - python( - "idx = 0\n" - "while True:\n" - " line = sys.stdin.readline()\n" - " if line == '': break\n" - " request = int(line.strip())\n" - " print(str(idx * 2) * request)\n" - " print(str(idx * 2 + 1) * request * 2, file=sys.stderr)\n" - " idx += 1\n" - ), - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + python( + "idx = 0\n" + "while True:\n" + " line = sys.stdin.readline()\n" + " if line == '': break\n" + " request = int(line.strip())\n" + " print(str(idx * 2) * request)\n" + " print(str(idx * 2 + 1) * request * 2, file=sys.stderr)\n" + " idx += 1\n" + ), + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, ) as proc: newline = b"\n" if posix else b"\r\n" From a7e771000acf9050bbe8177d4baead9626178fff Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 11:03:54 -0700 Subject: [PATCH 09/16] Add feature newsfragment --- newsfragments/1104.feature.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 newsfragments/1104.feature.rst diff --git a/newsfragments/1104.feature.rst b/newsfragments/1104.feature.rst new file mode 100644 index 0000000000..b2d4ceaa11 --- /dev/null +++ b/newsfragments/1104.feature.rst @@ -0,0 +1,3 @@ +You can now conveniently spawn a child process in a background task +and interact it with on the fly using ``process = await +nursery.start(run_process, ...)``. See `run_process` for more details. From 32b1c13a2b91e0af282bfb8c201837d5a5fd2479 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 11:59:04 -0700 Subject: [PATCH 10/16] Add a bit more coverage --- trio/tests/test_subprocess.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 776510d9d5..5666ded8e8 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -294,6 +294,10 @@ async def test_run(): await run_process(CAT, stdin="oh no, it's text") with pytest.raises(ValueError): await run_process(CAT, stdin=subprocess.PIPE) + with pytest.raises(ValueError): + await run_process(CAT, stdout=subprocess.PIPE) + with pytest.raises(ValueError): + await run_process(CAT, stderr=subprocess.PIPE) with pytest.raises(ValueError): await run_process(CAT, capture_stdout=True, stdout=subprocess.DEVNULL) with pytest.raises(ValueError): From 6f98761f826427777cebdc4d00288a5eb7039307 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 18:47:56 -0700 Subject: [PATCH 11/16] =?UTF-8?q?Rename=20trio.open=5Fprocess=20=E2=86=92?= =?UTF-8?q?=20trio.lowlevel.open=5Fprocess?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/source/history.rst | 6 +++--- docs/source/reference-io.rst | 7 +++---- docs/source/reference-lowlevel.rst | 6 ++++++ newsfragments/1104.removal.rst | 9 +++++---- trio/__init__.py | 11 ++++++++++- trio/_subprocess.py | 8 ++++++-- trio/_subprocess_platform/__init__.py | 7 +++---- trio/lowlevel.py | 2 ++ trio/tests/test_subprocess.py | 2 +- 9 files changed, 39 insertions(+), 19 deletions(-) diff --git a/docs/source/history.rst b/docs/source/history.rst index 0579d4420d..3936df867e 100644 --- a/docs/source/history.rst +++ b/docs/source/history.rst @@ -213,7 +213,7 @@ Features worry: you can now pass a custom ``deliver_cancel=`` argument to define your own process killing policy. (`#1104 `__) - It turns out that creating a subprocess can block the parent process - for a surprisingly long time. So `trio.open_process` now uses a worker + for a surprisingly long time. So ``trio.open_process`` now uses a worker thread to avoid blocking the event loop. (`#1109 `__) - We've added FreeBSD to the list of platforms we support and test on. (`#1118 `__) - On Linux kernels v5.3 or newer, `trio.Process.wait` now uses `the @@ -267,7 +267,7 @@ Deprecations and Removals alternatives or make a case for why some particular class should be designed to support subclassing. (`#1044 `__) - If you want to create a `trio.Process` object, you now have to call - `trio.open_process`; calling ``trio.Process()`` directly was + ``trio.open_process``; calling ``trio.Process()`` directly was deprecated in v0.12.0 and has now been removed. (`#1109 `__) - Remove ``clear`` method on `trio.Event`: it was deprecated in 0.12.0. (`#1498 `__) @@ -495,7 +495,7 @@ Deprecations and Removals deprecated. (`#878 `__) - It turns out that it's better to treat subprocess spawning as an async operation. Therefore, direct construction of `Process` objects has - been deprecated. Use `trio.open_process` instead. (`#1109 `__) + been deprecated. Use ``trio.open_process`` instead. (`#1109 `__) Miscellaneous internal changes diff --git a/docs/source/reference-io.rst b/docs/source/reference-io.rst index 77ae4cbd88..3a082982e0 100644 --- a/docs/source/reference-io.rst +++ b/docs/source/reference-io.rst @@ -697,10 +697,9 @@ task and interact with it while it's running: and we wanted to provide an interface with fewer surprises. If `trio.run_process` is too limiting, we also offer a low-level API, -`trio.open_process`. For example, use `open_process` if you want to -spawn a child process that outlives the parent process: - -.. autofunction:: trio.open_process +`trio.lowlevel.open_process`. For example, use +`~trio.lowlevel.open_process` if you want to spawn a child process +that outlives the parent process: .. _subprocess-options: diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index 2fe773cbb7..e0a604bebc 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -106,6 +106,12 @@ The tutorial has a :ref:`fully-worked example Trio's internal scheduling decisions. +Low-level process spawning +========================== + +.. autofunction:: trio.lowlevel.open_process + + Low-level I/O primitives ======================== diff --git a/newsfragments/1104.removal.rst b/newsfragments/1104.removal.rst index 406230ed08..f2a5a0a997 100644 --- a/newsfragments/1104.removal.rst +++ b/newsfragments/1104.removal.rst @@ -1,4 +1,5 @@ -The ``aclose`` method on `Process` has been deprecated, and ``async -with process_obj`` has also been deprecated. We recommend switching to -the new ``nursery.start(trio.run_process, ...)`` API instead, which is -more powerful and predictable. +``trio.open_process`` has been renamed to +`trio.lowlevel.open_process`, and the ``aclose`` method on `Process` +has been deprecated, along with ``async with process_obj``. We +recommend most users switch to the new +``nursery.start(trio.run_process, ...)`` API instead. diff --git a/trio/__init__.py b/trio/__init__.py index d66ffceea9..6a43c42d2a 100644 --- a/trio/__init__.py +++ b/trio/__init__.py @@ -70,7 +70,7 @@ from ._path import Path -from ._subprocess import Process, open_process, run_process +from ._subprocess import Process, run_process from ._ssl import SSLStream, SSLListener, NeedHandshakeError @@ -106,6 +106,15 @@ _deprecate.enable_attribute_deprecations(__name__) +__deprecated_attributes__ = { + "open_process": _deprecate.DeprecatedAttribute( + value=_subprocess.open_process, + version="0.20.0", + issue=1104, + instead="trio.lowlevel.open_process", + ), +} + # Having the public path in .__module__ attributes is important for: # - exception names in printed tracebacks # - sphinx :show-inheritance: diff --git a/trio/_subprocess.py b/trio/_subprocess.py index 44ebc0997c..ec623f19c7 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -78,9 +78,9 @@ class Process(AsyncResource, metaclass=NoPublicConstructor): If you need more control – for example, because you want to spawn a child process that outlives your program – then another option is to use - `open_process`:: + `trio.lowlevel.open_process`:: - process_object = await trio.open_process(...) + process_object = await trio.lowlevel.open_process(...) Attributes: args (str or list): The ``command`` passed at construction time, @@ -304,6 +304,10 @@ async def open_process( can write to the `~Process.stdin` stream, else `~Process.stdin` will be ``None``. + Unlike `trio.run_process`, this function doesn't do any kind of automatic + management of the child process. It's up to you to implement whatever semantics you + want. + Args: command (list or str): The command to run. Typically this is a sequence of strings such as ``['ls', '-l', 'directory with spaces']``, diff --git a/trio/_subprocess_platform/__init__.py b/trio/_subprocess_platform/__init__.py index 37797424ab..4caa27b47c 100644 --- a/trio/_subprocess_platform/__init__.py +++ b/trio/_subprocess_platform/__init__.py @@ -4,6 +4,7 @@ import sys from typing import Optional, Tuple, TYPE_CHECKING +import trio from .. import _core, _subprocess from .._abc import SendStream, ReceiveStream @@ -72,15 +73,13 @@ def create_pipe_from_child_output() -> Tuple[ReceiveStream, int]: pass elif os.name == "posix": - from ..lowlevel import FdStream - def create_pipe_to_child_stdin(): # noqa: F811 rfd, wfd = os.pipe() - return FdStream(wfd), rfd + return trio.lowlevel.FdStream(wfd), rfd def create_pipe_from_child_output(): # noqa: F811 rfd, wfd = os.pipe() - return FdStream(rfd), wfd + return trio.lowlevel.FdStream(rfd), wfd elif os.name == "nt": from .._windows_pipes import PipeSendStream, PipeReceiveStream diff --git a/trio/lowlevel.py b/trio/lowlevel.py index 8e6dfc5ee4..f30fdcc4bd 100644 --- a/trio/lowlevel.py +++ b/trio/lowlevel.py @@ -46,6 +46,8 @@ start_guest_run, ) +from ._subprocess import open_process + if sys.platform == "win32": # Windows symbols from ._core import ( diff --git a/trio/tests/test_subprocess.py b/trio/tests/test_subprocess.py index 5666ded8e8..28783303a9 100644 --- a/trio/tests/test_subprocess.py +++ b/trio/tests/test_subprocess.py @@ -14,11 +14,11 @@ sleep, sleep_forever, Process, - open_process, run_process, TrioDeprecationWarning, ClosedResourceError, ) +from ..lowlevel import open_process from .._core.tests.tutil import slow, skip_if_fbsd_pipes_broken from ..testing import wait_all_tasks_blocked From 35aa94b217f4c3606f2c5d2c1fcfca98e12e1181 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 18:52:36 -0700 Subject: [PATCH 12/16] blacken --- trio/_subprocess_platform/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/trio/_subprocess_platform/__init__.py b/trio/_subprocess_platform/__init__.py index 4caa27b47c..418808a8ac 100644 --- a/trio/_subprocess_platform/__init__.py +++ b/trio/_subprocess_platform/__init__.py @@ -73,6 +73,7 @@ def create_pipe_from_child_output() -> Tuple[ReceiveStream, int]: pass elif os.name == "posix": + def create_pipe_to_child_stdin(): # noqa: F811 rfd, wfd = os.pipe() return trio.lowlevel.FdStream(wfd), rfd From 69238c43f34747b03edb0a76365a8c741734c8e7 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 18:55:53 -0700 Subject: [PATCH 13/16] Try to make mypy happy --- trio/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trio/__init__.py b/trio/__init__.py index 6a43c42d2a..a50ec33310 100644 --- a/trio/__init__.py +++ b/trio/__init__.py @@ -108,7 +108,7 @@ __deprecated_attributes__ = { "open_process": _deprecate.DeprecatedAttribute( - value=_subprocess.open_process, + value=lowlevel.open_process, version="0.20.0", issue=1104, instead="trio.lowlevel.open_process", From ce42d72b8bb8f378cc8e738793f204983cb76eb2 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Tue, 13 Jul 2021 18:57:05 -0700 Subject: [PATCH 14/16] Fix sphinx crossrefs --- trio/_subprocess.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/trio/_subprocess.py b/trio/_subprocess.py index ec623f19c7..25d95aea24 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -292,17 +292,16 @@ async def open_process( ) -> Process: r"""Execute a child program in a new process. - After construction, you can interact with the child process by writing - data to its `~Process.stdin` stream (a `~trio.abc.SendStream`), reading - data from its `~Process.stdout` and/or `~Process.stderr` streams (both - `~trio.abc.ReceiveStream`\s), sending it signals using - `~Process.terminate`, `~Process.kill`, or `~Process.send_signal`, and - waiting for it to exit using `~Process.wait`. See `Process` for details. - - Each standard stream is only available if you specify that a pipe should - be created for it. For example, if you pass ``stdin=subprocess.PIPE``, you - can write to the `~Process.stdin` stream, else `~Process.stdin` will be - ``None``. + After construction, you can interact with the child process by writing data to its + `~trio.Process.stdin` stream (a `~trio.abc.SendStream`), reading data from its + `~trio.Process.stdout` and/or `~trio.Process.stderr` streams (both + `~trio.abc.ReceiveStream`\s), sending it signals using `~trio.Process.terminate`, + `~trio.Process.kill`, or `~trio.Process.send_signal`, and waiting for it to exit + using `~trio.Process.wait`. See `trio.Process` for details. + + Each standard stream is only available if you specify that a pipe should be created + for it. For example, if you pass ``stdin=subprocess.PIPE``, you can write to the + `~trio.Process.stdin` stream, else `~trio.Process.stdin` will be ``None``. Unlike `trio.run_process`, this function doesn't do any kind of automatic management of the child process. It's up to you to implement whatever semantics you @@ -334,7 +333,7 @@ async def open_process( are also accepted. Returns: - A new `Process` object. + A new `trio.Process` object. Raises: OSError: if the process spawning fails, for example because the From 76034fab5be24176730a93eb2e46302b43418761 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Mon, 13 Sep 2021 19:43:38 -0700 Subject: [PATCH 15/16] Update docs in response to reviews --- docs/source/reference-io.rst | 7 +++--- trio/_subprocess.py | 47 +++++++++++++++++++++--------------- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/docs/source/reference-io.rst b/docs/source/reference-io.rst index 3a082982e0..26a25f0500 100644 --- a/docs/source/reference-io.rst +++ b/docs/source/reference-io.rst @@ -697,9 +697,10 @@ task and interact with it while it's running: and we wanted to provide an interface with fewer surprises. If `trio.run_process` is too limiting, we also offer a low-level API, -`trio.lowlevel.open_process`. For example, use -`~trio.lowlevel.open_process` if you want to spawn a child process -that outlives the parent process: +`trio.lowlevel.open_process`. For example, if you want to spawn a +child process that will and outlive the parent process and be +orphaned, then `~trio.run_process` can't do that, but +`~trio.lowlevel.open_process` can. .. _subprocess-options: diff --git a/trio/_subprocess.py b/trio/_subprocess.py index 25d95aea24..f836e45e95 100644 --- a/trio/_subprocess.py +++ b/trio/_subprocess.py @@ -180,7 +180,7 @@ def returncode(self): return result @deprecated( - "0.16.0", + "0.20.0", thing="using trio.Process as an async context manager", issue=1104, instead="run_process or nursery.start(run_process, ...)", @@ -189,7 +189,7 @@ async def __aenter__(self): return self @deprecated( - "0.16.0", issue=1104, instead="run_process or nursery.start(run_process, ...)" + "0.20.0", issue=1104, instead="run_process or nursery.start(run_process, ...)" ) async def aclose(self): """Close any pipes we have to the process (both input and output) @@ -477,15 +477,16 @@ async def run_process( **Output:** By default, any output produced by the subprocess is passed through to the standard output and error streams of the - parent Trio process. If you would like to capture this output and - do something with it, you can pass ``capture_stdout=True`` to - capture the subprocess's standard output, and/or - ``capture_stderr=True`` to capture its standard error. Captured - data is provided as the + parent Trio process. + + When calling `run_process` directly, you can capture the subprocess's output by + passing ``capture_stdout=True`` to capture the subprocess's standard output, and/or + ``capture_stderr=True`` to capture its standard error. Captured data is collected up + by Trio into an in-memory buffer, and then provided as the :attr:`~subprocess.CompletedProcess.stdout` and/or - :attr:`~subprocess.CompletedProcess.stderr` attributes of the - returned :class:`~subprocess.CompletedProcess` object. The value - for any stream that was not captured will be ``None``. + :attr:`~subprocess.CompletedProcess.stderr` attributes of the returned + :class:`~subprocess.CompletedProcess` object. The value for any stream that was not + captured will be ``None``. If you want to capture both stdout and stderr while keeping them separate, pass ``capture_stdout=True, capture_stderr=True``. @@ -496,6 +497,13 @@ async def run_process( output will be available in the `~subprocess.CompletedProcess.stdout` attribute. + If you're using ``await nursery.start(trio.run_process, ...)`` and want to capture + the subprocess's output for further processing, then use ``stdout=subprocess.PIPE`` + and then make sure to read the data out of the `Process.stdout` stream. If you want + to capture stderr separately, use ``stderr=subprocess.PIPE``. If you want to capture + both, but mixed together in the correct order, use ``stdout=subproces.PIPE, + stderr=subprocess.STDOUT``. + **Error checking:** If the subprocess exits with a nonzero status code, indicating failure, :func:`run_process` raises a :exc:`subprocess.CalledProcessError` exception rather than @@ -541,8 +549,9 @@ async def run_process( same place as the parent Trio process's standard input. As is the case with the :mod:`subprocess` module, you can also pass a file descriptor or an object with a ``fileno()`` method, in which case - the subprocess's standard input will come from that file. And when - starting `run_process` as a background task, you can use + the subprocess's standard input will come from that file. + + When starting `run_process` as a background task, you can also use ``stdin=subprocess.PIPE``, in which case `Process.stdin` will be a `~trio.abc.SendStream` that you can use to send data to the child. @@ -629,19 +638,19 @@ async def my_deliver_cancel(process): if task_status is trio.TASK_STATUS_IGNORED: if stdin is subprocess.PIPE: raise ValueError( - "stdin=subprocess.PIPE doesn't make sense without " - "nursery.start, since there's no way to access the " - "pipe; pass the data you want to send or use nursery.start" + "stdout=subprocess.PIPE is only valid with nursery.start, " + "since that's the only way to access the pipe; use nursery.start " + "or pass the data you want to write directly" ) if options.get("stdout") is subprocess.PIPE: raise ValueError( - "stdout=subprocess.PIPE doesn't make sense without " - "nursery.start, since there's no way to access the pipe" + "stdout=subprocess.PIPE is only valid with nursery.start, " + "since that's the only way to access the pipe" ) if options.get("stderr") is subprocess.PIPE: raise ValueError( - "stderr=subprocess.PIPE doesn't make sense without " - "nursery.start, since there's no way to access the pipe" + "stderr=subprocess.PIPE is only valid with nursery.start, " + "since that's the only way to access the pipe" ) if isinstance(stdin, (bytes, bytearray, memoryview)): input = stdin From 1c4c99c3f01049f242f11f98722a3675122bd3c5 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Sun, 19 Sep 2021 00:26:06 -0700 Subject: [PATCH 16/16] Remove stray 'and' --- docs/source/reference-io.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/reference-io.rst b/docs/source/reference-io.rst index 26a25f0500..81a615e039 100644 --- a/docs/source/reference-io.rst +++ b/docs/source/reference-io.rst @@ -698,7 +698,7 @@ task and interact with it while it's running: If `trio.run_process` is too limiting, we also offer a low-level API, `trio.lowlevel.open_process`. For example, if you want to spawn a -child process that will and outlive the parent process and be +child process that will outlive the parent process and be orphaned, then `~trio.run_process` can't do that, but `~trio.lowlevel.open_process` can.