From 842d5565bdd4310cd96386a8ffa9949b24c5856f Mon Sep 17 00:00:00 2001 From: Ivan Petkov Date: Sun, 6 Sep 2020 20:30:40 -0700 Subject: process: add Child::{wait,try_wait} (#2796) * add Child::try_wait to mirror the std API * replace Future impl on Child with `.wait()` method to bring our APIs closer to those in std and it allow us to internally fuse the future so that repeated calls to `wait` result in the same value (similar to std) without forcing the caller to fuse the outer future * Also change `Child::id` to return an Option result to avoid allowing the caller to accidentally use the pid on Unix systems after the child has been reaped * Also remove deprecated Child methods --- tokio/src/process/mod.rs | 147 +++++++++++++++++++++++++++-------------- tokio/src/process/unix/mod.rs | 4 ++ tokio/src/process/unix/reap.rs | 2 +- tokio/src/process/windows.rs | 34 +++------- 4 files changed, 109 insertions(+), 78 deletions(-) (limited to 'tokio/src/process') diff --git a/tokio/src/process/mod.rs b/tokio/src/process/mod.rs index a3b7c384..057755c9 100644 --- a/tokio/src/process/mod.rs +++ b/tokio/src/process/mod.rs @@ -18,16 +18,15 @@ //! //! #[tokio::main] //! async fn main() -> Result<(), Box> { -//! // The usage is the same as with the standard library's `Command` type, however the value -//! // returned from `spawn` is a `Result` containing a `Future`. -//! let child = Command::new("echo").arg("hello").arg("world") -//! .spawn(); +//! // The usage is similar as with the standard library's `Command` type +//! let mut child = Command::new("echo") +//! .arg("hello") +//! .arg("world") +//! .spawn() +//! .expect("failed to spawn"); //! -//! // Make sure our child succeeded in spawning and process the result -//! let future = child.expect("failed to spawn"); -//! -//! // Await until the future (and the command) completes -//! let status = future.await?; +//! // Await until the command completes +//! let status = child.wait().await?; //! println!("the command exited with: {}", status); //! Ok(()) //! } @@ -83,8 +82,8 @@ //! //! // Ensure the child process is spawned in the runtime so it can //! // make progress on its own while we await for any output. -//! tokio::spawn(async { -//! let status = child.await +//! tokio::spawn(async move { +//! let status = child.wait().await //! .expect("child process encountered an error"); //! //! println!("child status was: {}", status); @@ -555,16 +554,17 @@ impl Command { /// Command::new("ls") /// .spawn() /// .expect("ls command failed to start") + /// .wait() /// .await /// .expect("ls command failed to run") /// } /// ``` pub fn spawn(&mut self) -> io::Result { imp::spawn_child(&mut self.std).map(|spawned_child| Child { - child: ChildDropGuard { + child: FusedChild::Child(ChildDropGuard { inner: spawned_child.child, kill_on_drop: self.kill_on_drop, - }, + }), stdin: spawned_child.stdin.map(|inner| ChildStdin { inner }), stdout: spawned_child.stdout.map(|inner| ChildStdout { inner }), stderr: spawned_child.stderr.map(|inner| ChildStderr { inner }), @@ -615,7 +615,7 @@ impl Command { child.stdout.take(); child.stderr.take(); - child.await + child.wait().await } } @@ -725,12 +725,16 @@ where } } +/// Keeps track of the exit status of a child process without worrying about +/// polling the underlying futures even after they have completed. +#[derive(Debug)] +enum FusedChild { + Child(ChildDropGuard), + Done(ExitStatus), +} + /// Representation of a child process spawned onto an event loop. /// -/// This type is also a future which will yield the `ExitStatus` of the -/// underlying child process. A `Child` here also provides access to information -/// like the OS-assigned identifier and the stdio streams. -/// /// # Caveats /// Similar to the behavior to the standard library, and unlike the futures /// paradigm of dropping-implies-cancellation, a spawned process will, by @@ -739,10 +743,9 @@ where /// The `Command::kill_on_drop` method can be used to modify this behavior /// and kill the child process if the `Child` wrapper is dropped before it /// has exited. -#[must_use = "futures do nothing unless polled"] #[derive(Debug)] pub struct Child { - child: ChildDropGuard, + child: FusedChild, /// The handle for writing to the child's standard input (stdin), if it has /// been captured. @@ -758,9 +761,17 @@ pub struct Child { } impl Child { - /// Returns the OS-assigned process identifier associated with this child. - pub fn id(&self) -> u32 { - self.child.inner.id() + /// Returns the OS-assigned process identifier associated with this child + /// while it is still running. + /// + /// Once the child has been polled to completion this will return `None`. + /// This is done to avoid confusion on platforms like Unix where the OS + /// identifier could be reused once the process has completed. + pub fn id(&self) -> Option { + match &self.child { + FusedChild::Child(child) => Some(child.inner.id()), + FusedChild::Done(_) => None, + } } /// Forces the child to exit. @@ -783,35 +794,77 @@ impl Child { /// let mut child = Command::new("sleep").arg("1").spawn().unwrap(); /// tokio::spawn(async move { send.send(()) }); /// tokio::select! { - /// _ = &mut child => {} + /// _ = child.wait() => {} /// _ = recv => { - /// &mut child.kill(); + /// child.kill().expect("kill failed"); /// // NB: await the child here to avoid a zombie process on Unix platforms - /// child.await.unwrap(); + /// child.wait().await.unwrap(); /// } /// } /// } - pub fn kill(&mut self) -> io::Result<()> { - self.child.kill() - } - - #[doc(hidden)] - #[deprecated(note = "please use `child.stdin` instead")] - pub fn stdin(&mut self) -> &mut Option { - &mut self.stdin + match &mut self.child { + FusedChild::Child(child) => child.kill(), + FusedChild::Done(_) => Err(io::Error::new( + io::ErrorKind::InvalidInput, + "invalid argument: can't kill an exited process", + )), + } } - #[doc(hidden)] - #[deprecated(note = "please use `child.stdout` instead")] - pub fn stdout(&mut self) -> &mut Option { - &mut self.stdout + /// Waits for the child to exit completely, returning the status that it + /// exited with. This function will continue to have the same return value + /// after it has been called at least once. + /// + /// The stdin handle to the child process, if any, will be closed + /// before waiting. This helps avoid deadlock: it ensures that the + /// child does not block waiting for input from the parent, while + /// the parent waits for the child to exit. + pub async fn wait(&mut self) -> io::Result { + match &mut self.child { + FusedChild::Done(exit) => Ok(*exit), + FusedChild::Child(child) => { + let ret = child.await; + + if let Ok(exit) = ret { + self.child = FusedChild::Done(exit); + } + + ret + } + } } - #[doc(hidden)] - #[deprecated(note = "please use `child.stderr` instead")] - pub fn stderr(&mut self) -> &mut Option { - &mut self.stderr + /// Attempts to collect the exit status of the child if it has already + /// exited. + /// + /// This function will not block the calling thread and will only + /// check to see if the child process has exited or not. If the child has + /// exited then on Unix the process ID is reaped. This function is + /// guaranteed to repeatedly return a successful exit status so long as the + /// child has already exited. + /// + /// If the child has exited, then `Ok(Some(status))` is returned. If the + /// exit status is not available at this time then `Ok(None)` is returned. + /// If an error occurs, then that error is returned. + /// + /// Note that unlike `wait`, this function will not attempt to drop stdin, + /// nor will it wake the current task if the child exits. + pub fn try_wait(&mut self) -> io::Result> { + match &mut self.child { + FusedChild::Done(exit) => Ok(Some(*exit)), + FusedChild::Child(guard) => { + let ret = guard.inner.try_wait(); + + if let Ok(Some(exit)) = ret { + // Avoid the overhead of trying to kill a reaped process + guard.kill_on_drop = false; + self.child = FusedChild::Done(exit); + } + + ret + } + } } /// Returns a future that will resolve to an `Output`, containing the exit @@ -845,7 +898,7 @@ impl Child { let stdout_fut = read_to_end(self.stdout.take()); let stderr_fut = read_to_end(self.stderr.take()); - let (status, stdout, stderr) = try_join3(self, stdout_fut, stderr_fut).await?; + let (status, stdout, stderr) = try_join3(self.wait(), stdout_fut, stderr_fut).await?; Ok(Output { status, @@ -855,14 +908,6 @@ impl Child { } } -impl Future for Child { - type Output = io::Result; - - fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - Pin::new(&mut self.child).poll(cx) - } -} - /// The standard input stream for spawned children. /// /// This type implements the `AsyncWrite` trait to pass data to the stdin handle of diff --git a/tokio/src/process/unix/mod.rs b/tokio/src/process/unix/mod.rs index c25d9897..a8df74a3 100644 --- a/tokio/src/process/unix/mod.rs +++ b/tokio/src/process/unix/mod.rs @@ -117,6 +117,10 @@ impl Child { pub(crate) fn id(&self) -> u32 { self.inner.id() } + + pub(crate) fn try_wait(&mut self) -> io::Result> { + self.inner.inner_mut().try_wait() + } } impl Kill for Child { diff --git a/tokio/src/process/unix/reap.rs b/tokio/src/process/unix/reap.rs index 8963805a..c51a20b9 100644 --- a/tokio/src/process/unix/reap.rs +++ b/tokio/src/process/unix/reap.rs @@ -63,7 +63,7 @@ where self.inner.as_ref().expect("inner has gone away") } - fn inner_mut(&mut self) -> &mut W { + pub(crate) fn inner_mut(&mut self) -> &mut W { self.inner.as_mut().expect("inner has gone away") } } diff --git a/tokio/src/process/windows.rs b/tokio/src/process/windows.rs index cbe2fa75..1fbdee21 100644 --- a/tokio/src/process/windows.rs +++ b/tokio/src/process/windows.rs @@ -24,20 +24,15 @@ use mio_named_pipes::NamedPipe; use std::fmt; use std::future::Future; use std::io; -use std::os::windows::prelude::*; -use std::os::windows::process::ExitStatusExt; +use std::os::windows::prelude::{AsRawHandle, FromRawHandle, IntoRawHandle}; use std::pin::Pin; use std::process::{Child as StdChild, Command as StdCommand, ExitStatus}; use std::ptr; use std::task::Context; use std::task::Poll; -use winapi::shared::minwindef::FALSE; -use winapi::shared::winerror::WAIT_TIMEOUT; use winapi::um::handleapi::INVALID_HANDLE_VALUE; -use winapi::um::processthreadsapi::GetExitCodeProcess; -use winapi::um::synchapi::WaitForSingleObject; use winapi::um::threadpoollegacyapiset::UnregisterWaitEx; -use winapi::um::winbase::{RegisterWaitForSingleObject, INFINITE, WAIT_OBJECT_0}; +use winapi::um::winbase::{RegisterWaitForSingleObject, INFINITE}; use winapi::um::winnt::{BOOLEAN, HANDLE, PVOID, WT_EXECUTEINWAITTHREAD, WT_EXECUTEONLYONCE}; #[must_use = "futures do nothing unless polled"] @@ -86,6 +81,10 @@ impl Child { pub(crate) fn id(&self) -> u32 { self.child.id() } + + pub(crate) fn try_wait(&mut self) -> io::Result> { + self.child.try_wait() + } } impl Kill for Child { @@ -106,11 +105,11 @@ impl Future for Child { Poll::Ready(Err(_)) => panic!("should not be canceled"), Poll::Pending => return Poll::Pending, } - let status = try_wait(&inner.child)?.expect("not ready yet"); + let status = inner.try_wait()?.expect("not ready yet"); return Poll::Ready(Ok(status)); } - if let Some(e) = try_wait(&inner.child)? { + if let Some(e) = inner.try_wait()? { return Poll::Ready(Ok(e)); } let (tx, rx) = oneshot::channel(); @@ -157,23 +156,6 @@ unsafe extern "system" fn callback(ptr: PVOID, _timer_fired: BOOLEAN) { let _ = complete.take().unwrap().send(()); } -pub(crate) fn try_wait(child: &StdChild) -> io::Result> { - unsafe { - match WaitForSingleObject(child.as_raw_handle(), 0) { - WAIT_OBJECT_0 => {} - WAIT_TIMEOUT => return Ok(None), - _ => return Err(io::Error::last_os_error()), - } - let mut status = 0; - let rc = GetExitCodeProcess(child.as_raw_handle(), &mut status); - if rc == FALSE { - Err(io::Error::last_os_error()) - } else { - Ok(Some(ExitStatus::from_raw(status))) - } - } -} - pub(crate) type ChildStdin = PollEvented; pub(crate) type ChildStdout = PollEvented; pub(crate) type ChildStderr = PollEvented; -- cgit v1.2.3