-
-
Notifications
You must be signed in to change notification settings - Fork 331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
0d6559a
to
afb1d62
Compare
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to say goodbye to async with await
! My comments relate mainly to documentation; the code changes look good to me for whatever that's worth 😉.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you for finishing it up! Just a few small comments
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_process
totrio.lowlevel
I think this gets the basic semantics right? Still need to do
everything around that though.