From 627515a7ff4fe12084d7e95969bda307849b4d0e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 3 Feb 2016 18:09:35 -0800 Subject: [PATCH] std: Push Child's exit status to sys::process On Unix we have to be careful to not call `waitpid` twice, but we don't have to be careful on Windows due to the way process handles work there. As a result the cached `Option` is only necessary on Unix, and it's also just an implementation detail of the Unix module. At the same time. also update some code in `kill` on Unix to avoid a wonky waitpid with WNOHANG. This was added in 0e190b9a to solve #13124, but the `signal(0)` method is not supported any more so there's no need to for this workaround. I believe that this is no longer necessary as it's not really doing anything. --- src/libstd/process.rs | 49 ++++--------------------------- src/libstd/sys/unix/process.rs | 42 +++++++++++++------------- src/libstd/sys/windows/process.rs | 8 +++-- 3 files changed, 30 insertions(+), 69 deletions(-) diff --git a/src/libstd/process.rs b/src/libstd/process.rs index e11bb72a35a58..819643f20fe81 100644 --- a/src/libstd/process.rs +++ b/src/libstd/process.rs @@ -11,15 +11,14 @@ //! Working with processes. #![stable(feature = "process", since = "1.0.0")] -#![allow(non_upper_case_globals)] use prelude::v1::*; use io::prelude::*; use ffi::OsStr; use fmt; -use io::{self, Error, ErrorKind}; -use path; +use io; +use path::Path; use str; use sys::pipe::{self, AnonPipe}; use sys::process as imp; @@ -61,9 +60,6 @@ use thread::{self, JoinHandle}; pub struct Child { handle: imp::Process, - /// None until wait() or wait_with_output() is called. - status: Option, - /// The handle for writing to the child's stdin, if it has been captured #[stable(feature = "process", since = "1.0.0")] pub stdin: Option, @@ -243,7 +239,7 @@ impl Command { /// Sets the working directory for the child process. #[stable(feature = "process", since = "1.0.0")] - pub fn current_dir>(&mut self, dir: P) -> &mut Command { + pub fn current_dir>(&mut self, dir: P) -> &mut Command { self.inner.cwd(dir.as_ref().as_ref()); self } @@ -288,7 +284,6 @@ impl Command { Err(e) => Err(e), Ok(handle) => Ok(Child { handle: handle, - status: None, stdin: our_stdin.map(|fd| ChildStdin { inner: fd }), stdout: our_stdout.map(|fd| ChildStdout { inner: fd }), stderr: our_stderr.map(|fd| ChildStderr { inner: fd }), @@ -508,34 +503,7 @@ impl Child { /// SIGKILL on unix platforms. #[stable(feature = "process", since = "1.0.0")] pub fn kill(&mut self) -> io::Result<()> { - #[cfg(unix)] fn collect_status(p: &mut Child) { - // On Linux (and possibly other unices), a process that has exited will - // continue to accept signals because it is "defunct". The delivery of - // signals will only fail once the child has been reaped. For this - // reason, if the process hasn't exited yet, then we attempt to collect - // their status with WNOHANG. - if p.status.is_none() { - match p.handle.try_wait() { - Some(status) => { p.status = Some(status); } - None => {} - } - } - } - #[cfg(windows)] fn collect_status(_p: &mut Child) {} - - collect_status(self); - - // if the process has finished, and therefore had waitpid called, - // and we kill it, then on unix we might ending up killing a - // newer process that happens to have the same (re-used) id - if self.status.is_some() { - return Err(Error::new( - ErrorKind::InvalidInput, - "invalid argument: can't kill an exited process", - )) - } - - unsafe { self.handle.kill() } + self.handle.kill() } /// Returns the OS-assigned process identifier associated with this child. @@ -555,14 +523,7 @@ impl Child { #[stable(feature = "process", since = "1.0.0")] pub fn wait(&mut self) -> io::Result { drop(self.stdin.take()); - match self.status { - Some(code) => Ok(ExitStatus(code)), - None => { - let status = try!(self.handle.wait()); - self.status = Some(status); - Ok(ExitStatus(status)) - } - } + self.handle.wait().map(ExitStatus) } /// Simultaneously waits for the child to exit and collect all remaining diff --git a/src/libstd/sys/unix/process.rs b/src/libstd/sys/unix/process.rs index 7387e9def9f04..41b9b3ef12632 100644 --- a/src/libstd/sys/unix/process.rs +++ b/src/libstd/sys/unix/process.rs @@ -8,8 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -#![allow(non_snake_case)] - use prelude::v1::*; use os::unix::prelude::*; @@ -271,7 +269,8 @@ impl fmt::Display for ExitStatus { /// The unique id of the process (this should never be negative). pub struct Process { - pid: pid_t + pid: pid_t, + status: Option, } pub enum Stdio { @@ -285,11 +284,6 @@ pub type RawStdio = FileDesc; const CLOEXEC_MSG_FOOTER: &'static [u8] = b"NOEX"; impl Process { - pub unsafe fn kill(&self) -> io::Result<()> { - try!(cvt(libc::kill(self.pid, libc::SIGKILL))); - Ok(()) - } - pub fn spawn(cfg: &mut Command, in_fd: Stdio, out_fd: Stdio, @@ -324,7 +318,7 @@ impl Process { } }; - let p = Process{ pid: pid }; + let mut p = Process { pid: pid, status: None }; drop(output); let mut bytes = [0; 8]; @@ -516,22 +510,26 @@ impl Process { self.pid as u32 } - pub fn wait(&self) -> io::Result { - let mut status = 0 as c_int; - try!(cvt_r(|| unsafe { libc::waitpid(self.pid, &mut status, 0) })); - Ok(ExitStatus(status)) + pub fn kill(&mut self) -> io::Result<()> { + // If we've already waited on this process then the pid can be recycled + // and used for another process, and we probably shouldn't be killing + // random processes, so just return an error. + if self.status.is_some() { + Err(Error::new(ErrorKind::InvalidInput, + "invalid argument: can't kill an exited process")) + } else { + cvt(unsafe { libc::kill(self.pid, libc::SIGKILL) }).map(|_| ()) + } } - pub fn try_wait(&self) -> Option { - let mut status = 0 as c_int; - match cvt_r(|| unsafe { - libc::waitpid(self.pid, &mut status, libc::WNOHANG) - }) { - Ok(0) => None, - Ok(n) if n == self.pid => Some(ExitStatus(status)), - Ok(n) => panic!("unknown pid: {}", n), - Err(e) => panic!("unknown waitpid error: {}", e), + pub fn wait(&mut self) -> io::Result { + if let Some(status) = self.status { + return Ok(status) } + let mut status = 0 as c_int; + try!(cvt_r(|| unsafe { libc::waitpid(self.pid, &mut status, 0) })); + self.status = Some(ExitStatus(status)); + Ok(ExitStatus(status)) } } diff --git a/src/libstd/sys/windows/process.rs b/src/libstd/sys/windows/process.rs index 6a04aa2f2c4f3..e5e4187d228e9 100644 --- a/src/libstd/sys/windows/process.rs +++ b/src/libstd/sys/windows/process.rs @@ -202,8 +202,10 @@ impl Process { Ok(Process { handle: Handle::new(pi.hProcess) }) } - pub unsafe fn kill(&self) -> io::Result<()> { - try!(cvt(c::TerminateProcess(self.handle.raw(), 1))); + pub fn kill(&mut self) -> io::Result<()> { + try!(cvt(unsafe { + c::TerminateProcess(self.handle.raw(), 1) + })); Ok(()) } @@ -213,7 +215,7 @@ impl Process { } } - pub fn wait(&self) -> io::Result { + pub fn wait(&mut self) -> io::Result { unsafe { let res = c::WaitForSingleObject(self.handle.raw(), c::INFINITE); if res != c::WAIT_OBJECT_0 {