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: avoid redundant effort to reap orphan processes #3743

Merged
merged 10 commits into from
May 14, 2021

Conversation

ipetkov
Copy link
Member

@ipetkov ipetkov commented May 3, 2021

  • Child processes are akin to global variables within the parent
    process. Reaping orphaned child processes can be done by any thread
    (so long as someone does it), but repeated efforts from multiple
    threads to reap the same process do not add any additional benefit
  • Previously, each Runtime would register its own SIGCHLD listener and
    check it for updates during every tick. If a signal was received, then
    it will attempt to reap the orphan queue.
  • Signals are also "global resources" meaning that when one signal comes
    in, all listeners within that process are woken up. This means that if
    an application has multiple runtimes, each of them will attempt to
    drain the entire orphan queue any time a SIGCHLD is received (the
    first one to try will reap whichever orphans have exited, and the
    other will perform redundant effort).
  • Now we register a single SIGCHLD listener for the entire global queue
    itself, which allows us to "dedup" signal notifications even if
    multiple runtimes tick at the same time and attempt to drive the
    orphan queue (one will grab a lock and win the race, and everyone else
    will move on).
  • Moreover, the single, global SIGCHLD listener will be registered
    lazily (instead of eagerly on the runtime initialization). This means
    that applications which do not use child processes will not "pollute"
    the signal handler space, even if the feature is enabled at compile
    time

Refs #3520

@ipetkov ipetkov added A-tokio Area: The main tokio crate M-process Module: tokio/process M-signal Module: tokio/signal labels May 3, 2021
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

This seems good to me.

tokio/src/process/unix/orphan.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

Darksonn commented May 5, 2021

How does this PR affect #3521?

@ipetkov
Copy link
Member Author

ipetkov commented May 5, 2021

How does this PR affect #3521?

This change is purely orthogonal to #3521, but this PR should lessen the for it to land for applications which have the process feature turned on but never actually used.

@Darksonn
Copy link
Contributor

Darksonn commented May 7, 2021

The mutex in crate::loom::sync does not have poisoning so you need to remove the unwraps on the lock calls.

ipetkov and others added 10 commits May 8, 2021 09:49
* Child processes are akin to global variables within the parent
  process. Reaping orphaned child processes can be done by any thread
  (so long as someone does it), but repeated efforts from multiple
  threads to reap the same process do not add any additional benefit
* Previously, each Runtime would register its own SIGCHLD listener and
  check it for updates during every tick. If a signal was received, then
  it will attempt to reap the orphan queue.
* Signals are also "global resources" meaning that when one signal comes
  in, all listeners within that process are woken up. This means that if
  an application has multiple runtimes, each of them will attempt to
  drain the entire orphan queue any time a SIGCHLD is received (the
  first one to try will reap whichever orphans have exited, and the
  other will perform redundant effort).
* Now we register a single SIGCHLD listener for the entire global queue
  itself, which allows us to "dedup" signal notifications even if
  multiple runtimes tick at the same time and attempt to drive the
  orphan queue (one will grab a lock and win the race, and everyone else
  will move on).
* Moreover, the single, global SIGCHLD listener will be registered
  lazily (instead of eagerly on the runtime initialization). This means
  that applications which do not use child processes will not "pollute"
  the signal handler space, even if the feature is enabled at compile
  time
Co-authored-by: Alice Ryhl <alice@ryhl.io>
@Darksonn
Copy link
Contributor

Darksonn commented May 8, 2021

Did that force push have any changes, or was it just a merge of master?

(Please try to avoid force pushes — I can't tell what has changed and usually need to review the entire thing again)

@ipetkov
Copy link
Member Author

ipetkov commented May 8, 2021

@Darksonn sorry I rebased on master because there was an unrelated failure (BSD test failed on some warning for an unused return value from sleep)

@Darksonn
Copy link
Contributor

Darksonn commented May 8, 2021

No worries, although it's usually enough to merge in master.

@Darksonn Darksonn merged commit e188e99 into master May 14, 2021
@Darksonn Darksonn deleted the ivan/process-lazy-orphan branch May 14, 2021 08:21
@Darksonn Darksonn mentioned this pull request May 14, 2021
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 M-process Module: tokio/process M-signal Module: tokio/signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants