Skip to content
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: add ProcessDriver to handle orphan reaping #2907

Merged
merged 1 commit into from Oct 6, 2020

Conversation

ipetkov
Copy link
Member

@ipetkov ipetkov commented Oct 3, 2020

Motivation

  • Dropping/canceling child processes cannot be done cleanly since we can neither afford to perform a blocking wait call in the destructor, nor do we have something like async-destructors to perform the clean up for us
  • Although the kill_on_drop flag will terminate the child process on drop, there is no guarantee that the caller will call child.await which will leave a zombie process floating around and can lead to exhaustion of the pid space
  • We do try to clean up orphaned processes on a best-effort-basis, as long as additional child processes are being spawned/polled, but this isn't good enough since enough zombie processes can accumulate through one way or another

Solution

  • Introduce a ProcessDriver (only on Unix platforms) which will reap the orphaned processes spawned by tokio, gradually eliminating them as they finally exit.
  • This also avoids having every child process wrapper attempt to drain the orphan queue, which should lead to faster poll times and fewer contentions for the orphan queue lock.

Refs #2685
Refs #2841

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-process Module: tokio/process labels Oct 3, 2020
// ===== process driver =====

cfg_process_driver! {
type ProcessDriver = crate::park::Either<crate::process::unix::driver::Driver, SignalDriver>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not have to happen in this PR. I believe that, instead of defining an Either for each individual component (I/O, signal, process), you probably can have a single Either for the entire "I/O driver stack". This is because, at runtime, you can either enable I/O, signal, and process or disable all of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, this was all left over from before we decided to group the I/O stack together. I'll have to think a bit about how to implement this and I'll open a follow up PR

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Added a thought for cleanup that can be done in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-process Module: tokio/process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants