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<ExitStatus>` 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 0e190b9 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.
rust-lang · bors · Feb 11, 2016 · Oct 31, 2015 · Feb 4, 2016 · Feb 4, 2016
commit 627515a7ff4fe12084d7e95969bda307849b4d0e
diff --git 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<imp::ExitStatus>,
-
     /// The handle for writing to the child's stdin, if it has been captured
     #[stable(feature = "process", since = "1.0.0")]
     pub stdin: Option<ChildStdin>,
@@ -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<P: AsRef<path::Path>>(&mut self, dir: P) -> &mut Command {
+    pub fn current_dir<P: AsRef<Path>>(&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<ExitStatus> {
         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
@@ -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<ExitStatus>,
 }
 
 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<ExitStatus> {
-        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<ExitStatus> {
-        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<ExitStatus> {
+        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
@@ -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<ExitStatus> {
+    pub fn wait(&mut self) -> io::Result<ExitStatus> {
         unsafe {
             let res = c::WaitForSingleObject(self.handle.raw(), c::INFINITE);
             if res != c::WAIT_OBJECT_0 {