-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add pidfd-based implementation of tokio::process
#4016
Comments
Worth noting that we may need to wait for our MSRV includes this change before we can roll it out as a feature |
I think this could be gated behind some kind of |
The feature has already been around in a crate, so there's no need for wait for a Rust compiler update. Trivial to implement by scratch as well. |
Getting the full benefits of this feature (eliminating the issue of pidfd wraparound, and removing the dependency on signal handlers) requires going through the standard library, so that we can obtain a pidfd at the same time the child process is spawned. |
It is safe to obtain a pidfd using the |
Yes, but calling I recognize that this is an edge case - however, recent versions of Linux now have all of the tools needed to wait for child processes in a truly 'self-contained' manner, which does not need to make any assumptions about other code running in the same process. I think it would be great to be able to take advantage of that in Tokio, when the kernel version in use supports it. |
There's nothing that they can do that isn't already being done in the two pidfd crates. You obtain a pidfd after a process has been spawned, the kernel guarantees that the child's PID will remain valid until three child is reaped by waiting on the process. I use this crate in a lot of my projects and have never had to use signal handlers. |
Which Rust version was this released in? |
The reaping can happen by some other code outside of the user's control (e.g. a background thread). Creating the pidfd along with the process is the only approach that is guaranteed to be free from race conditions in the face of additional calls to |
Tokio already has its own File, Command, and Child types, so I don't see how this would require std support. Tokio can create the pidfd at the same it creates the child (or after because that's perfectly fine), with a stable version of Rust from 1+ years ago. |
The tokio/tokio/src/process/unix/mod.rs Line 103 in cf38ba6
As long as the standard library |
We don't have to rely on the standard library to add convenience methods for this. It could take another year or two before they stabilize it, and longer to get into a stable release. We can do what you want with functionality already readily available. Because this is a Linux-only feature, we don't have to avoid using the CommandExt unix module for access to pre_exec, and we don't have to shy away from using libc for the pidfd and clone3 syscalls. What Tokio is already doing is already unreliable. Using SIGCHLD handlers to monitor a process is even worse than upgrading the PID to a PidFd after it has spawned. |
Note that using a pidfd will cause libstd to fall back to the relatively slow |
I believe #6152 addresses this, in a way that still uses vfork. |
Is your feature request related to a problem? Please describe.
The current unix implementation of
tokio::process
relies on handlingSIGCHLD
signals. This requires that the corresponding signal handler be properly set up - if another library in the process modifies it, it will be impossible to properly wait on child processes.Describe the solution you'd like
On recent versions of Linux, it's possible to use
pidfd
s to avoid this problem. Using the recently merged libstd support (rust-lang/rust#81825), we can create a pidfd at the same time that a child is spawned. The pidfd can then be waited on usingepoll
orselect
(with an optional timeout).If the running kernel version does not support pidfds, we can fall back to the existing unix implementation.
Describe alternatives you've considered
Do nothing, and continue to use the unix implementation in all cases.
The text was updated successfully, but these errors were encountered: