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.

Sign up for GitHub

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

process: kill child on error while spawning #6803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b68da52
process: kill child on error while spawning
Maki325 Aug 27, 2024
9cc1bfb
clippy and FreeBSD fix
Maki325 Aug 27, 2024
34888ce
Merge branch 'tokio-rs:master' into fix/kill-process-on-error
Maki325 Aug 29, 2024
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
94 changes: 85 additions & 9 deletions tokio/src/process/unix/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -115,21 +115,97 @@ 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 {
ChildDropGuard {
child: Some(child),
kill_on_drop: true,
}
}

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

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()
}

#[cfg(all(target_os = "linux", feature = "rt"))]
pub(crate) fn dont_kill_on_drop(&mut self) {
self.kill_on_drop = false;
}

#[cfg(all(target_os = "linux", feature = "rt"))]
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,
Expand All @@ -138,7 +214,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,
Expand All @@ -158,7 +234,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(),
}
}

Expand Down
54 changes: 49 additions & 5 deletions tokio/src/process/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading