feature(process): Use pidfd on Linux for `tokio::process::Child::wait` #6152

NobodyXu · 2023-11-14T12:37:16Z

Resolve #6141

Motivation

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.

Solution

On Linux, a better solution would be to use pidfd and fallback to waiting on SIGCHLD signal.

Since libstd support for pidfd is still unstable rust-lang/rust#82971, this PR uses pidfd_open in parent to open the pid, this isn't 100% race free but it might be a good start, it will be simple to implement and vfork can continue to be used.

The pidfd returned can be epolled and it will awake only one future when the process has exited, and then std::process::Child::try_wait is called to get the exit status since holding a pidfd prevents the pid from being recycled.

NobodyXu · 2023-11-16T13:25:31Z

cc @Darksonn @ipetkov

tokio/Cargo.toml

tokio/src/process/unix/pidfd_reaper.rs

Darksonn · 2023-12-03T13:06:47Z

So, I think the situation for this PR is as follows: I think this is a good idea, and I would be happy to see it happen. However, right now, I just don't have the bandwidth to take on this PR.

To move forward with this PR, you will need to find a different maintainer who is willing to review it.

Sorry.

NobodyXu · 2023-12-03T13:38:29Z

Thank you for reviewing my PR!

I would ping others to review this, or I can wait until someone is free to do this.

NobodyXu · 2023-12-03T13:41:18Z

Pinging @taiki-e , can you review this please?
Sorry if I was coming as rude.

Signed-off-by: Jiahao XU <[email protected]>

fallback to signal-based `Reaper` if pidfd cannot be created. Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

Since miri does not support process spawning Signed-off-by: Jiahao XU <[email protected]>

Only Linux kernel >= 5.10 support pidfd Signed-off-by: Jiahao XU <[email protected]>

And fixed `test::is_pidfd_available` Signed-off-by: Jiahao XU <[email protected]>

To avoid potential headaches such as: - The drop being run outside of a Tokio runtime context - The runtime being present but not actually running Since this is the initial implementation, I chose to use the `OrphanQueue` also used by the signal-driven `Reaper` impl. It's simple and easy to implement without having to add runtime driver and guaranteed to work. Further improvement can be done on the basis of this PR. Signed-off-by: Jiahao XU <[email protected]>

NobodyXu · 2023-12-31T00:49:58Z

@Darksonn Do you know which maintainer might be willing to pick this PR?
I'm not familiar with tokio maintainers and not sure who's willing to pick this up,

Darksonn · 2023-12-31T11:53:38Z

Could I get you to open a thread for discussing this in #tokio-dev on our Discord? Preferably after new years so that the others are around to respond.

NobodyXu · 2023-12-31T12:02:47Z

Could I get you to open a thread for discussing this in #tokio-dev on our Discord? Preferably after new years so that the others are around to respond.

Sure I will do that tomorrow.

NobodyXu · 2024-01-01T05:07:14Z

@Darksonn It seems that there's already a thread there (seems to be opened by you).

Sorry it's my first time trying to open a thread on discord and I thought commenting on tokio-dev would automatically makes a thread.

carllerche

Looks pretty good to me. I left some smallish comments, but this should be pretty close to mergable.

tokio/src/process/unix/mod.rs

tokio/Cargo.toml

Instead of automatically enabling feature `rt` if `process` is enabled, even if the target os is not `linux`. Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

carllerche

LGTM, I'll give @Darksonn an opportunity to provide final thoughts before merging.

Darksonn

Overall looks pretty reasonable. A few comments:

tokio/src/process/unix/pidfd_reaper.rs

tokio/tests/process_spawned_and_wait_in_different_runtime.rs

Also remove invocation of one `unsafe` function `errno_location`. Signed-off-by: Jiahao XU <[email protected]>

Move `err.kind() == io::ErrorKind::Other` checking into `is_rt_shutdown_err` Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

tokio/tests/process_change_of_runtime.rs

tokio/src/process/unix/pidfd_reaper.rs

Signed-off-by: Jiahao XU <[email protected]>

Darksonn

Thank you.

sunshowers · 2024-02-04T01:00:31Z

Thanks for working on this!

this PR uses pidfd_open in parent to open the pid, this isn't 100% race free but it might be a good start, it will be simple to implement and vfork can continue to be used.

For nextest it would be a pretty big problem if we can't use posix_spawn/vfork -- in some situations it would lead to a performance loss of 100-200% along with some pretty bad race conditions. I see that in rust-lang/rust#82971 there's a discussion about pidfd_spawn, which would be ideal. But if not, it would be nice to not get forced down the fork/exec path, especially on machines that have glibc older than 2.39 (when pidfd_spawn was added).

NobodyXu · 2024-02-04T01:53:41Z

@sunshowers This PR calls pidfd_open after spawning in parent, it doesn't use the unstable pidfd API provided by libstd, so it would continue to use vfork if it originally uses vfork.

This PR would improve Child::wait() performance and also make sure the pid doesn't get recycled given that the child isn't reaped before pidfd_open() is called, this is why I called it racy.

NobodyXu · 2024-02-04T01:54:21Z

@sunshowers would be great if you could upgrade nextest to tokio v1.36.0 and see if there's any performance improve on Linux.

sunshowers · 2024-02-04T03:02:16Z

@sunshowers This PR calls pidfd_open after spawning in parent, it doesn't use the unstable pidfd API provided by libstd, so it would continue to use vfork if it originally uses vfork.

Right, and thanks for that! Just wanted to mention this for future work -- the fallback to pidfd_spawn ideally wouldn't be fork/exec, it would be this implementation.

@sunshowers would be great if you could upgrade nextest to tokio v1.36.0 and see if there's any performance improve on Linux.

Not seeing any perf difference with a standard release build, though I need to do an LTO build to really measure it.

Bumps tokio from 1.35.1 to 1.36.0. Release notes Sourced from tokio's releases. Tokio v1.36.0 1.36.0 (February 2nd, 2024) Added io: add tokio::io::Join (#6220) io: implement AsyncWrite for Empty (#6235) net: add support for anonymous unix pipes (#6127) net: add UnixSocket (#6290) net: expose keepalive option on TcpSocket (#6311) sync: add {Receiver,UnboundedReceiver}::poll_recv_many (#6236) sync: add Sender::{try_,}reserve_many (#6205) sync: add watch::Receiver::mark_unchanged (#6252) task: add JoinSet::try_join_next (#6280) Changed io: make copy cooperative (#6265) io: make repeat and sink cooperative (#6254) io: simplify check for empty slice (#6293) process: use pidfd on Linux when available (#6152) sync: use AtomicBool in broadcast channel future (#6298) Documented io: clarify clear_ready docs (#6304) net: document that *Fd traits on TcpSocket are unix-only (#6294) sync: document FIFO behavior of tokio::sync::Mutex (#6279) chore: typographic improvements (#6262) runtime: remove obsolete comment (#6303) task: fix typo (#6261) #6220: tokio-rs/tokio#6220 #6235: tokio-rs/tokio#6235 #6127: tokio-rs/tokio#6127 #6290: tokio-rs/tokio#6290 #6311: tokio-rs/tokio#6311 #6236: tokio-rs/tokio#6236 #6205: tokio-rs/tokio#6205 #6252: tokio-rs/tokio#6252 #6280: tokio-rs/tokio#6280 #6265: tokio-rs/tokio#6265 #6254: tokio-rs/tokio#6254 #6293: tokio-rs/tokio#6293 #6238: tokio-rs/tokio#6238 #6152: tokio-rs/tokio#6152 #6298: tokio-rs/tokio#6298 #6262: tokio-rs/tokio#6262 #6303: tokio-rs/tokio#6303 #6261: tokio-rs/tokio#6261 ... (truncated) Commits eaf81ed chore: prepare Tokio v1.36.0 (#6312) 53f9e5a ci: make sure dictionary words are sorted and unique (#6316) 9077762 net: expose keepalive option on TcpSocket (#6311) 131e7b4 ci: add spellchecking (#6297) e53b92a io: clarify clear_ready docs (#6304) 7536132 sync: use AtomicBool in broadcast channel future (#6298) b6d0c90 macros: fix trait_method breaking change detection (#6308) 4846959 runtime: remove obsolete comment (#6303) ec30383 net: add UnixSocket (#6290) f80bbec io: simplify check for empty slice (#6293) Additional commits viewable in compare view Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase. Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: @dependabot rebase will rebase this PR @dependabot recreate will recreate this PR, overwriting any edits that have been made to it @dependabot merge will merge this PR after your CI passes on it @dependabot squash and merge will squash and merge this PR after your CI passes on it @dependabot cancel merge will cancel a previously requested merge and block automerging @dependabot reopen will reopen this PR if it is closed @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

NobodyXu force-pushed the feature/pidfd branch 3 times, most recently from bd8371b to 92b8831 Compare November 15, 2023 12:05

Darksonn added A-tokio Area: The main tokio crate M-process Module: tokio/process labels Nov 24, 2023

Darksonn reviewed Nov 24, 2023

View reviewed changes

tokio/Cargo.toml Outdated Show resolved Hide resolved

ipetkov reviewed Nov 27, 2023

View reviewed changes

tokio/src/process/unix/pidfd_reaper.rs Outdated Show resolved Hide resolved

NobodyXu requested review from Darksonn and ipetkov November 27, 2023 13:42

NobodyXu force-pushed the feature/pidfd branch from 5ab99d4 to 5fc07d2 Compare December 8, 2023 12:45

NobodyXu force-pushed the feature/pidfd branch from 5fc07d2 to 9f387cf Compare December 23, 2023 01:21

NobodyXu added 9 commits December 31, 2023 10:34

feat(process) pidfd: Implement initial PidfdReaper impl

d3b6551

Signed-off-by: Jiahao XU <[email protected]>

Use PidfdReaper in unix::Child if possible

106d9c8

fallback to signal-based `Reaper` if pidfd cannot be created. Signed-off-by: Jiahao XU <[email protected]>

Fix compilation on non-linux targets

a3945d0

Signed-off-by: Jiahao XU <[email protected]>

Add testing for pidfd_reaper

c6a869f

Signed-off-by: Jiahao XU <[email protected]>

Fix running tests in pidfd_reaper

5f62be6

Signed-off-by: Jiahao XU <[email protected]>

Disable pidfd_reaper test for miri

92bef5a

Since miri does not support process spawning Signed-off-by: Jiahao XU <[email protected]>

Skip pidfd testing on unsupported linux kernel

80a1cd7

Only Linux kernel >= 5.10 support pidfd Signed-off-by: Jiahao XU <[email protected]>

Optimization: Skip pidfd_open if last call failed with ENOSYS

c38faca

And fixed `test::is_pidfd_available` Signed-off-by: Jiahao XU <[email protected]>

NobodyXu force-pushed the feature/pidfd branch from 9f387cf to 3b89981 Compare December 31, 2023 00:34

carllerche reviewed Jan 8, 2024

View reviewed changes

tokio/src/process/unix/mod.rs Show resolved Hide resolved

tokio/Cargo.toml Outdated Show resolved Hide resolved

Only pull in PidfdReaper if feature rt is enabled on Linux

bf14726

Instead of automatically enabling feature `rt` if `process` is enabled, even if the target os is not `linux`. Signed-off-by: Jiahao XU <[email protected]>

NobodyXu requested a review from carllerche January 9, 2024 09:39

NobodyXu added 3 commits January 9, 2024 21:17

Add test process_spawned_outside_runtime

523117a

Signed-off-by: Jiahao XU <[email protected]>

Add process_spawned_and_wait_in_different_runtime

66a2f4e

Signed-off-by: Jiahao XU <[email protected]>

Fix process_spawned_and_wait_in_different_runtime on linux with pidfd

ef05617

Signed-off-by: Jiahao XU <[email protected]>

carllerche approved these changes Jan 10, 2024

View reviewed changes

Darksonn reviewed Jan 10, 2024

View reviewed changes

tokio/src/process/unix/pidfd_reaper.rs Outdated Show resolved Hide resolved

tokio/src/process/unix/pidfd_reaper.rs Outdated Show resolved Hide resolved

tokio/tests/process_spawned_and_wait_in_different_runtime.rs Outdated Show resolved Hide resolved

NobodyXu added 3 commits January 11, 2024 09:01

Improve safety comments in Pidfd::open

e89a5ec

Also remove invocation of one `unsafe` function `errno_location`. Signed-off-by: Jiahao XU <[email protected]>

Refactor <PidfdReaperInner as Future>::poll

f093c44

Move `err.kind() == io::ErrorKind::Other` checking into `is_rt_shutdown_err` Signed-off-by: Jiahao XU <[email protected]>

Refactor: Mv process_spawned_* into process_change_of_runtime.rs

7ee4509

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu requested a review from Darksonn January 10, 2024 22:29

Darksonn reviewed Jan 11, 2024

View reviewed changes

tokio/tests/process_change_of_runtime.rs Outdated Show resolved Hide resolved

tokio/src/process/unix/pidfd_reaper.rs Outdated Show resolved Hide resolved

Fix leaked sleep process in test_pidfd_reaper_drop

dc889e3

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu requested a review from Darksonn January 11, 2024 12:52

Darksonn approved these changes Jan 11, 2024

View reviewed changes

Darksonn merged commit e4f9bcb into tokio-rs:master Jan 11, 2024
71 checks passed

NobodyXu deleted the feature/pidfd branch January 11, 2024 13:00

This was referenced Jan 11, 2024

Add new linux-only API process::Child::pidfd #6281

Open

Signals do not add SA_ONSTACK which may cause linked Go applications to crash. #3520

Open

maminrayej mentioned this pull request Jan 30, 2024

chore: prepare Tokio v1.36.0 #6312

Merged

sunshowers mentioned this pull request Feb 4, 2024

Add pidfd-based implementation of tokio::process #4016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(process): Use pidfd on Linux for `tokio::process::Child::wait` #6152

feature(process): Use pidfd on Linux for `tokio::process::Child::wait` #6152

NobodyXu commented Nov 14, 2023 •

edited

Loading

NobodyXu commented Nov 16, 2023

Darksonn commented Dec 3, 2023

NobodyXu commented Dec 3, 2023

NobodyXu commented Dec 3, 2023

NobodyXu commented Dec 31, 2023

Darksonn commented Dec 31, 2023

NobodyXu commented Dec 31, 2023

NobodyXu commented Jan 1, 2024 •

edited

Loading

carllerche left a comment

carllerche left a comment

Darksonn left a comment

Darksonn left a comment

sunshowers commented Feb 4, 2024 •

edited

Loading

NobodyXu commented Feb 4, 2024

NobodyXu commented Feb 4, 2024 •

edited

Loading

sunshowers commented Feb 4, 2024

feature(process): Use pidfd on Linux for tokio::process::Child::wait #6152

feature(process): Use pidfd on Linux for tokio::process::Child::wait #6152

Conversation

NobodyXu commented Nov 14, 2023 • edited Loading

Motivation

Solution

NobodyXu commented Nov 16, 2023

Darksonn commented Dec 3, 2023

NobodyXu commented Dec 3, 2023

NobodyXu commented Dec 3, 2023

NobodyXu commented Dec 31, 2023

Darksonn commented Dec 31, 2023

NobodyXu commented Dec 31, 2023

NobodyXu commented Jan 1, 2024 • edited Loading

carllerche left a comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

sunshowers commented Feb 4, 2024 • edited Loading

NobodyXu commented Feb 4, 2024

NobodyXu commented Feb 4, 2024 • edited Loading

sunshowers commented Feb 4, 2024

feature(process): Use pidfd on Linux for `tokio::process::Child::wait` #6152

feature(process): Use pidfd on Linux for `tokio::process::Child::wait` #6152

NobodyXu commented Nov 14, 2023 •

edited

Loading

NobodyXu commented Jan 1, 2024 •

edited

Loading

sunshowers commented Feb 4, 2024 •

edited

Loading

NobodyXu commented Feb 4, 2024 •

edited

Loading