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: kill child on error while spawning #6803

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
process: kill child on error while spawning
There's a chance that between spawning of the `std Child`, and wrapping
it into a tokio impl an error can occurs, which would leave the child
alive, with no way to access it from the user side and not being part
of the tokio runtime.

This commit fixes that issue by wrapping the `std Child` right after
spawning with a struct that implements Drop, where the child would
be killed if it's dropped.

Fixes: #6797
Maki325 committed Aug 27, 2024
commit b68da520d6f765b17907a9c886a66487e8cdc70b
92 changes: 83 additions & 9 deletions tokio/src/process/unix/mod.rs
Original file line number Diff line number Diff line change
@@ -106,7 +106,7 @@ impl OrphanQueue<StdChild> for GlobalOrphanQueue {
pub(crate) enum Child {
SignalReaper(Reaper<StdChild, GlobalOrphanQueue, Signal>),
#[cfg(all(target_os = "linux", feature = "rt"))]
PidfdReaper(pidfd_reaper::PidfdReaper<StdChild, GlobalOrphanQueue>),
PidfdReaper(pidfd_reaper::PidfdReaper<ChildDropGuard, GlobalOrphanQueue>),
}

impl fmt::Debug for Child {
@@ -115,21 +115,95 @@ impl fmt::Debug for Child {
}
}

pub(crate) struct ChildDropGuard {
Copy link
Member

Choose a reason for hiding this comment

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

We already define ChildDropGuard in tokio/src/process/mod.rs. Can we:

  • reuse that type instead of having to redefine it in the unix and windows modules?
  • re-structure how/where the StdChild is wrapped so we don't end up double wrapping?
    • as this PR is currently laid out we will end up having two kill-on-drop wrappers which will have different behavior (namely trying to kill the process twice which is different behavior than we have today)

Copy link
Author

Choose a reason for hiding this comment

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

We can maybe combine ChildDropGuard from tokio/src/process/unix/mod.rs and DroppableChild from tokio/src/process/windows.rs, and have it in tokio/src/process/mod.rs, but we cant use ChildDropGuard from tokio/src/process/mod.rs. The reason being that we want the ability to take the child out from the Wapper struct, and ChildDropGuard from tokio/src/process/mod.rs doesn't allow that. The reason being that implementing Drop doesn't allow destructuring, so in the other implementations we use Options to circumvent that.

The double wrapped child won't make the process be killed twice, as we set the inner wrapper(ChildDropGuard from tokio/src/process/unix/mod.rs) to not kill the child on drop (tokio/src/process/unix/mod.rs line 202), so only the outer one(ChildDropGuard from tokio/src/process/mod.rs) will do that if so set.

I tried to make it so there's no double wrapping, but I couldn't make the PidfdReaper::new impl work. Because I would have to some way extract the child from the wrapper in the new function. And I couldn't get it to work.

Copy link
Member

Choose a reason for hiding this comment

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

It is okay if the definition of ChildDropGuard needs to move out of tokio/src/process/mod.rs and into the platform-specific implementation modules, but there is no need to have two wrappers which attempt to perform a kill-on-drop (and bloat the data structures with redundant bools).

child: Option<StdChild>,
kill_on_drop: bool,
}

impl Drop for ChildDropGuard {
fn drop(&mut self) {
if let Some(child) = &mut self.child {
if self.kill_on_drop {
drop(child.kill());
}
}
}
}

impl ChildDropGuard {
pub(crate) fn new(child: StdChild) -> ChildDropGuard {
return ChildDropGuard {
child: Some(child),
kill_on_drop: true,
};
}

pub(crate) fn extract(mut self) -> StdChild {
self.child.take().unwrap()
}

pub(crate) fn dont_kill_on_drop(&mut self) {
self.kill_on_drop = false;
}

pub(crate) fn take_stdin(&mut self) -> Option<std::process::ChildStdin> {
self.child
.as_mut()
.expect("child has gone away")
.stdin
.take()
}
pub(crate) fn take_stdout(&mut self) -> Option<std::process::ChildStdout> {
self.child
.as_mut()
.expect("child has gone away")
.stdout
.take()
}
pub(crate) fn take_stderr(&mut self) -> Option<std::process::ChildStderr> {
self.child
.as_mut()
.expect("child has gone away")
.stderr
.take()
}

pub(crate) fn inner_mut(&mut self) -> &mut StdChild {
self.child.as_mut().expect("child has gone away")
}
}

impl Wait for ChildDropGuard {
fn id(&self) -> u32 {
self.child.as_ref().expect("child has gone away").id()
}
fn try_wait(&mut self) -> io::Result<Option<ExitStatus>> {
self.child.as_mut().expect("child has gone away").try_wait()
}
}

impl OrphanQueue<ChildDropGuard> for GlobalOrphanQueue {
fn push_orphan(&self, orphan: ChildDropGuard) {
get_orphan_queue().push_orphan(orphan.extract());
}
}

pub(crate) fn spawn_child(cmd: &mut std::process::Command) -> io::Result<SpawnedChild> {
let mut child = cmd.spawn()?;
let stdin = child.stdin.take().map(stdio).transpose()?;
let stdout = child.stdout.take().map(stdio).transpose()?;
let stderr = child.stderr.take().map(stdio).transpose()?;
let mut child = ChildDropGuard::new(cmd.spawn()?);
let stdin = child.take_stdin().map(stdio).transpose()?;
let stdout = child.take_stdout().map(stdio).transpose()?;
let stderr = child.take_stderr().map(stdio).transpose()?;

#[cfg(all(target_os = "linux", feature = "rt"))]
match pidfd_reaper::PidfdReaper::new(child, GlobalOrphanQueue) {
Ok(pidfd_reaper) => {
Ok(mut pidfd_reaper) => {
pidfd_reaper.inner_mut().dont_kill_on_drop();
return Ok(SpawnedChild {
child: Child::PidfdReaper(pidfd_reaper),
stdin,
stdout,
stderr,
})
});
}
Err((Some(err), _child)) => return Err(err),
Err((None, child_returned)) => child = child_returned,
@@ -138,7 +212,7 @@ pub(crate) fn spawn_child(cmd: &mut std::process::Command) -> io::Result<Spawned
let signal = signal(SignalKind::child())?;

Ok(SpawnedChild {
child: Child::SignalReaper(Reaper::new(child, GlobalOrphanQueue, signal)),
child: Child::SignalReaper(Reaper::new(child.extract(), GlobalOrphanQueue, signal)),
stdin,
stdout,
stderr,
@@ -158,7 +232,7 @@ impl Child {
match self {
Self::SignalReaper(signal_reaper) => signal_reaper.inner_mut(),
#[cfg(all(target_os = "linux", feature = "rt"))]
Self::PidfdReaper(pidfd_reaper) => pidfd_reaper.inner_mut(),
Self::PidfdReaper(pidfd_reaper) => pidfd_reaper.inner_mut().inner_mut(),
}
}

54 changes: 49 additions & 5 deletions tokio/src/process/windows.rs
Original file line number Diff line number Diff line change
@@ -66,15 +66,59 @@ struct Waiting {
unsafe impl Sync for Waiting {}
unsafe impl Send for Waiting {}

struct DroppableChild {
child: Option<StdChild>,
}

impl Drop for DroppableChild {
fn drop(&mut self) {
if let Some(child) = &mut self.child {
drop(child.kill());
}
}
}

impl DroppableChild {
pub(crate) fn new(child: StdChild) -> DroppableChild {
return DroppableChild { child: Some(child) };
}

pub(crate) fn take_stdin(&mut self) -> Option<std::process::ChildStdin> {
self.child
.as_mut()
.expect("child has gone away")
.stdin
.take()
}
pub(crate) fn take_stdout(&mut self) -> Option<std::process::ChildStdout> {
self.child
.as_mut()
.expect("child has gone away")
.stdout
.take()
}
pub(crate) fn take_stderr(&mut self) -> Option<std::process::ChildStderr> {
self.child
.as_mut()
.expect("child has gone away")
.stderr
.take()
}

pub(crate) fn take(mut self) -> StdChild {
self.child.take().expect("child has gone away")
}
}

pub(crate) fn spawn_child(cmd: &mut StdCommand) -> io::Result<SpawnedChild> {
let mut child = cmd.spawn()?;
let stdin = child.stdin.take().map(stdio).transpose()?;
let stdout = child.stdout.take().map(stdio).transpose()?;
let stderr = child.stderr.take().map(stdio).transpose()?;
let mut child = DroppableChild::new(cmd.spawn()?);
let stdin = child.take_stdin().map(stdio).transpose()?;
let stdout = child.take_stdout().map(stdio).transpose()?;
let stderr = child.take_stderr().map(stdio).transpose()?;

Ok(SpawnedChild {
child: Child {
child,
child: child.take(),
waiting: None,
},
stdin,