process = await nursery.start(run_process, ...) #1568
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1568 +/- ##
==========================================
- Coverage 99.56% 99.41% -0.15%
==========================================
Files 114 114
Lines 14634 15404 +770
Branches 1119 1279 +160
==========================================
+ Hits 14570 15314 +744
- Misses 43 60 +17
- Partials 21 30 +9
|
|
This looks like a good approach so far! |
trio/_subprocess.py
Outdated
| 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`:: |
There was a problem hiding this comment.
Wait is this true?
Can open_process() outlive the "program" given the subprocess routine is launched using .to_thread.run()`?
Maybe "program" isn't the ideal term here?
There was a problem hiding this comment.
We use a thread to spin up the process, but once it's started it's an independent entity.
There was a problem hiding this comment.
Right, my quiff is on the use of the term program.
I presume no thread outlives the trio program given the deamon=True flag is used iirc?
Maybe I'm misunderstanding terminology.
There was a problem hiding this comment.
open_process doesn't spawn a thread, it spawns a process... there are no threads here (except very briefly as an implementation detail that isn't mentioned in the docs)
There was a problem hiding this comment.
Ahh i see. I didn't realize you could intentionally leak processes; gtk.
| task_status.started(proc) | ||
| await proc.wait() | ||
| except BaseException: | ||
| with trio.CancelScope(shield=True): |
There was a problem hiding this comment.
Yeah this is great; love how the spawning maps 1-to-1 with a task now.
This is exactly what we ended up with in tractor since we also needed a shielded Process.wait() before the call to Process.__aexit__() (internal IPC protocol takes care of inter-process cancellation) and the task mapping pattern was the only way to reliably collect sub-process errors and get cancellation logic right (even when using multiprocessing).
So the only motivation now for open_process() use directly is a non-SC process-lives-longer then "current nursery" type scenario yes? This run_process() call (presuming .start() use) is effectively the same but with guaranteed shielded supervision on teardown.
My only last question is if the async context mng interface is deprecated then is there now no longer a way to manually kill only a specific process?
Afaict here, you can't force the teardown (634 onwards) without cancelling the task (but that usually requires cancelling a group scope since, nursery, and a new cancel scope isn't created internally here per process). So if cancellation is the new __aexit__(), then we somewhat lose granular control of per-process cancellation and/or its completion detection right? Unless of course you want to use open_process() directly (but then in that case shouldn't __aexit__() code get moved to the post-yield body of open_process())?
The streams closing and process killing in .aclose() is something we currently depend on in tractor.
With that last question, forgive me if I'm missing something.
There was a problem hiding this comment.
My only last question is if the async context mng interface is deprecated then is there now no longer a way to manually kill only a specific process?
You can still kill a process. Process.terminate/Process.kill/Process.send_signal are all still available, and you can still close the streams if you want. Process.aclose was always just a convenience wrapper around other existing public APIs, and that hasn't changed.
There was a problem hiding this comment.
Process.aclose was always just a convenience wrapper around other existing public APIs, and that hasn't changed.
Ok so it will henceforth follow that users can roll their own version of .aclose() but that won't be default behavior.
njsmith
force-pushed
the
run-process-in-task
branch
from
0d6559a to
afb1d62
Compare
njsmith
force-pushed
the
run-process-in-task
branch
from
afb1d62 to
3fae5aa
Compare
Fixes some race conditions in tests that make assertions about proc.returncode after exiting the `async with killing` block.
|
At long last, this is ready for review! |
|
The 3.10-dev failure appears to be some kind of incompatibility between IPython and 3.10-dev, nothing in our code. |
Fixes #1104
Todo:
open_processtotrio.lowlevelI think this gets the basic semantics right? Still need to do
everything around that though.