Subprocess support: high-level interface #833
Conversation
- Add `NullStream` to the generic streams interface, which acts like `/dev/null` - Improvements to `Process`, mostly needed or wanted by higher-level functions: - Move from `trio.subprocess` to the base `trio` module, so `trio.subprocess` is just a reexport of constants and exceptions - Forbid lists with `shell=True` and strings with `shell=False` on UNIX due to the likelihood of user confusion - Add constructor options `shutdown_signal` and `shutdown_timeout` to allow orderly shutdown of subprocesses - Fix support for multiple tasks calling `wait()` simultaneously - `cancelled` attribute to track whether a subprocess was terminated due to `aclose`/etc cancellation - `failed` attribute to track whether a subprocess exit failure was "our fault" (if we killed it or closed its output streams before it had exited) - `join` method to do `aclose` without closing the pipes - `shutdown` method to do a cancelled `join`, since with `shutdown_signal`/`shutdown_timeout` this might be more complex than just `kill()` - Add `ProcessStream`, a single `HalfCloseableStream` wrapper around `Process` - Add `open_process`, a context manager that provides a `ProcessStream` and can raise `CalledProcessError` if the process fails - Add our own `CompletedProcess` with `cancelled` and `failed` attributes and consistent naming of the command (we use `command` vs the stdlib `args`) - Add `run_process`, like `subprocess.run` but with better defaults - Instead of the previous `timeout` and `deadline` arguments, add `preserve_result=True` option to return the `CompletedProcess` even if cancelled; this seems less invasive
Codecov Report
@@ Coverage Diff @@
## master #833 +/- ##
==========================================
- Coverage 99.23% 98.43% -0.81%
==========================================
Files 101 101
Lines 12182 12646 +464
Branches 882 944 +62
==========================================
+ Hits 12089 12448 +359
- Misses 72 174 +102
- Partials 21 24 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #833 +/- ##
==========================================
+ Coverage 99.21% 99.28% +0.06%
==========================================
Files 101 101
Lines 12221 12812 +591
Branches 894 1007 +113
==========================================
+ Hits 12125 12720 +595
+ Misses 75 71 -4
Partials 21 21
Continue to review full report at Codecov.
|
| def failed(self): | ||
| """Whether this process should be considered to have failed. | ||
|
|
||
| If the process hasn't exited yet, this is None. Otherwise, it is False |
There was a problem hiding this comment.
This seems tricky and error-prone (having it be True, False or None)
There was a problem hiding this comment.
I feel like this is the convention I usually see for "optional bool", and it mirrors the returncode attribute (which is None or an integer). Would you prefer that accessing .failed throw an exception if the process is still running?
|
The reported coverage miss is on the blank line after |
|
I'll start with a pass through the high-level description. I haven't looked at the code at all :-). You know that trope of kids growing up and realizing they're turning into their parents? While writing this I'm having some definite flashbacks to Guido demanding I pare down my beautiful proposals to the simplest possible thing, and then a bit further. It don't remember it being very fun to be on the receiving end. So, uh... sorry?
Do we really need this? Why?
Let's split this out into #852 so we can discuss the trade-offs properly.
Good idea. Maybe submit this as a standalone PR?
Oh yeah we definitely want this! Again, maybe a good quick standalone PR?
Maybe
This is... a lot of stuff. What is it all for? Why should we believe that people will use it?
Good idea :-)
Huh, that's a really interesting idea! Normally we're all dogmatic, Thou Shalt Not Catch A async def outer_function():
with move_on_after(...):
...
# Eventually called by outer_function, separated by like 10 stack frames or something
async def inner_function():
with move_on_after(...):
completed_process = await trio.run_process(..., preserve_result=True)
# Here if completed_process says it failed, we expect it's because of our move_on_after
# but maybe it's actually because of the one way out in outer_function?
Hmm. I see the point, but it also seems a bit awkward to deal with. Where do we put it? Do we need to keep the |
|
Thanks for the feedback. I understand that there's a lot here and I appreciate the backpressure looking for cleaner ways to do things. Can I ask though that you read the documentation changes included in this PR before making decisions about whether something is worth the user-facing complexity or not? I put most of my explanatory effort there, not in the commit message, because docs reach a good deal more people than the commit messages do. :-)
NullStream can be removed if we remove ProcessStream (I only needed it for
Sure. There doesn't seem to be anyone opposed, though, so maybe there's not much to discuss?
This sort of goes along with the section I added to the docs about quoting, and when trying to split it off I realized it's going to be troublesome to isolate those doc changes from the "we're not just reexporting
We can use that in place of
Shutdown stuff: If you start a subprocess that does nontrivial work, depending on the application it might be considered pretty rude to SIGKILL it without giving it any prior notice that you'd like it to clean up and exit. You might wind up with temporary files still on disk, sub-subprocesses getting reparented to init and running forever, etc. Probably whatever mechanism we use for grace periods (#147) could (should?) replace the explicit specification of a
It's definitely a power user feature, and might merit a warning in the docs, and/or a suggestion to put an explicit checkpoint after the call. I don't mind going back to timeout/deadline if you think that's less confusing, but I do think we should have some way to support this use case. Maybe that works out to "we need a general way to report partial progress before a cancellation, and then run_process() can just use that". I appreciate the simplicity of Trio's stance that "a cancelled operation didn't happen", but it doesn't necessarily compose very well -- if an operation is built out of multiple other underlying operations that can't readily be rolled back, either the "cancelled = didn't happen" rule has to break or the entire higher-level operation has to be uncancellable once started. I don't think we want to propose the latter, so maybe we should think about a user-friendly way to talk about the circumstances in which the rule gets bent? |
Ah cool, thanks for the heads up :-). Read them now. At a high level: I think it's great you put this mega-PR together. Like you say in the first comment, it's great for identifying issues. Now that we have this laid out though, I think a divide-and-conquer strategy is going to work best for actually getting changes in :-). So I'm going to be looking for all the uncontroversial bits we can carve off and merge on their own. In that vein, I see three "obvious" PRs, beyond the ones you already submitted. I'll discuss the other topics more below too, but these are probably the next things to actually work on :-):
I feel like I still have a lot of questions here:
So to me this feels like something to come back to after we've dealt with all the easier stuff...
Okay yeah this is interesting and complicated. I feel like there are a few interlocking cases here: what
It sounds like these are all mostly helpers for other features, so we can defer worrying about them until we figure out those other features :-)
Yeah, that stance is good (necessary) for fundamental operations like |
Sometimes. If I'm a user-facing program that starts some subprocess: probably not, let it go to /dev/stderr. If I'm a OS-level job controller or whatever: yeah, failure reports need to include stderr, you can't let them go to syslog (or, worse, /dev/null) because that is associated with my job controller, not with the job. The most common case is probably the former, so that should be the default IMHO. |
Closes #822.
NullStreamto the generic streams interface, which acts like/dev/nullProcess, mostly needed or wanted by higher-level functions:trio.subprocessto the basetriomodule, sotrio.subprocessis just a reexport of constants and exceptionsshell=Trueand strings withshell=Falseon UNIX due to the likelihood of user confusionshutdown_signalandshutdown_timeoutto allow orderly shutdown of subprocesseswait()simultaneouslycancelledattribute to track whether a subprocess was terminated due toaclose/etc cancellationfailedattribute to track whether a subprocess exit failure was "our fault" (if we killed it or closed its output streams before it had exited)joinmethod to doaclosewithout closing the pipesshutdownmethod to do a cancelledjoin, since withshutdown_signal/shutdown_timeoutthis might be more complex than justkill()ProcessStream, a singleHalfCloseableStreamwrapper aroundProcessopen_process, a context manager that provides aProcessStreamand can raiseCalledProcessErrorif the process failsCompletedProcesswithcancelledandfailedattributes and consistent naming of the command (we usecommandvs the stdlibargs)run_process, likesubprocess.runbut with better defaultstimeoutanddeadlinearguments, addpreserve_result=Trueoption to return theCompletedProcesseven if cancelled; this seems less invasiveI don't necessarily expect to merge this wholesale, but I wanted to provide a complete and polished interface as an example input to debate about what the interface ought to be. :-) Especial points of uncertainty for me include the advisability of adding
ProcessStreaminstead of just havingopen_processreturn aProcess; whether the process exiting failure should cancel theopen_processblock; and whetherpreserve_resultis in fact a less invasive API choice than the previously proposedtimeout/deadlinearguments.