This repository has been archived by the owner on Jul 1, 2021. It is now read-only.
Every component's process is now its own process group leader #1666
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gsalgado
force-pushed
the
one-process-group-per-process
branch
4 times, most recently
from
50ddc7d
to
9dd62ca
Compare
gsalgado
force-pushed
the
one-process-group-per-process
branch
from
9dd62ca
to
2b3ae8c
Compare
pipermerriam
approved these changes
Apr 15, 2020
There was a problem hiding this comment.
I am ok going forward with this but I'm curious whether it would make more sense to do this by default under the hood in asyncio_run_in_process rather than injecting it here at the application level. Since asyncio_open_in_process is designed to handle SIGINT it seems like it would make sense for it to do this by default.
|
|
||
| async def run(self) -> None: | ||
| start_new_session = True | ||
| if os.getenv('TRINITY_SINGLE_PROCESS_GROUP') == "1": |
There was a problem hiding this comment.
Maybe rather than putting this logic in the run method here it could just live inside the get_subprocess_kwargs method?
There was a problem hiding this comment.
Makes sense! I'd like to think a bit more about the implications about making this behaviour the default in asyncio-run-in-process, though, so I've filed an issue for that
gsalgado
force-pushed
the
one-process-group-per-process
branch
from
2b3ae8c
to
ec2d5b6
Compare
IOW, for every process we start, we create a new process group. This way a Ctrl-C in the terminal won't cause a SIGINT to be sent to each of our processes, instead a single one will be sent to the parent process, which will then send SIGINTs to every child (via asyncio-run-in-process' open_in_process()).
gsalgado
force-pushed
the
one-process-group-per-process
branch
from
ec2d5b6
to
668b9a9
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IOW, for every process we start, we create a new process group.
This way a Ctrl-C in the terminal won't cause a SIGINT to be sent
to each of our processes, instead a single one will be sent to the
parent process, which will then send SIGINTs to every child (via
asyncio-run-in-process' open_in_process()).
Turns out our integration tests rely on all processes belonging to a single process group so that they can be correctly terminated between tests, so I had to add this new env var to control whether or not to use separate process groups.