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

Use pidfd on Linux for tokio::process::Child::wait #6141

Closed
NobodyXu opened this issue Nov 11, 2023 · 2 comments · Fixed by #6152
Closed

Use pidfd on Linux for tokio::process::Child::wait #6141

NobodyXu opened this issue Nov 11, 2023 · 2 comments · Fixed by #6152
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-process Module: tokio/process

Comments

@NobodyXu
Copy link
Contributor

NobodyXu commented Nov 11, 2023

Is your feature request related to a problem? Please describe.

tokio::process::Child::wait currently uses signal to decide whether or not the child might have exited.
Since signal can be dropped at arbitrary time, any SIGCHLD signal received would cause all tokio::process::Child::wait() future to be awakened and execute std::process::Child::try_wait to decide whether it's ready.

Describe the solution you'd like

On Linux, a better solution would be to use pidfd.
Since libstd support for pidfd is still unstable rust-lang/rust#82971, tokio can choose to either:

And then the pidfd returned can be epolled and it will awake only one future when the process has exited, then waitid can be used to wait on the child in a race free manner.

(Or tokio can just continue to call std::process::Child::try_wait since holding a pidfd prevents the pid from being recycled.)

For tokio::process::Child::{kill, start_kill}, if we have a pidfd, then pidfd_send_signal is the most robust way of sending the signal to the right process.

According to pidfd_open:

Even if the child has already terminated by the time of the pidfd_open() call, its PID will not have been recycled and the returned file descriptor will refer to the resulting zombie process.

So as long as the pidfd is held, existing implementation of tokio::process::Child::{kill, start_kill} should be race-free.

tokio can also have a new method:

use std::os::fd::BorrowedFd;

impl Child {
    #[cfg(linux)]
    fn pidfd(&self) -> Option<BorrowedFd<'_>>;
}

Describe alternatives you've considered

#6140

Additional context

@NobodyXu NobodyXu added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Nov 11, 2023
@NobodyXu NobodyXu changed the title Use pidfd on Linux for tokio::process::Child::{wait, kill, start_kill} Use pidfd on Linux for tokio::process::Child::wait Nov 11, 2023
@Darksonn Darksonn added the M-process Module: tokio/process label Nov 11, 2023
@Darksonn
Copy link
Contributor

Thoughts @ipetkov ?

@ipetkov
Copy link
Member

ipetkov commented Nov 12, 2023

I don't have any experience with pidfd and I don't have any reservations about someone adding such an implementation, so long as we can gracefully fall back if it isn't available

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-feature-request Category: A feature request. M-process Module: tokio/process
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants