From 598bee0a1f1a1b6747f2bc3c62f6fdc87a32c77e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 19:29:28 +0000 Subject: [PATCH 01/12] Command::spawn: example: Actually wait for the Child (!) Signed-off-by: Ian Jackson --- library/std/src/process.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index fb78e62834a07..1034019f358bf 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -839,9 +839,13 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .spawn() - /// .expect("ls command failed to start"); + /// .expect("ls command failed to start") + /// .wait() + /// .expect("failed to wait for child"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn spawn(&mut self) -> io::Result { From 51cbc01a00acd17e2c080cad94c19ba3145db99e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 19:30:00 +0000 Subject: [PATCH 02/12] Command::spawn: docs: Suggest use of `status` instead If you don't need to call methods on the `Child`, then `status` is more convenient. This should be mentioned. Signed-off-by: Ian Jackson --- library/std/src/process.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 1034019f358bf..9e8008a9cd20c 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -832,6 +832,9 @@ impl Command { /// /// By default, stdin, stdout and stderr are inherited from the parent. /// + /// If you just want to run the command and wait for it to complete, consider using + /// [`Command::status`] instead. + /// /// # Examples /// /// Basic usage: From a4ffa2cc55513e5821493b26796ad304017f6371 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 19:51:23 +0000 Subject: [PATCH 03/12] Command docs: Replace many spawn with status and an extra check All these examples called `spawn` and then didn't wait for the child (!) Fix this by having them call .status() rather than .spawn(), and assert that the status was success. Signed-off-by: Ian Jackson --- library/std/src/process.rs | 66 +++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 9e8008a9cd20c..3b7d808a5b96b 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -525,9 +525,11 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("sh") - /// .spawn() + /// let status = Command::new("sh") + /// .status() /// .expect("sh command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn new>(program: S) -> Command { @@ -569,11 +571,13 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .arg("-l") /// .arg("-a") - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn arg>(&mut self, arg: S) -> &mut Command { @@ -599,10 +603,12 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .args(&["-l", "-a"]) - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn args(&mut self, args: I) -> &mut Command @@ -628,10 +634,12 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .env("PATH", "/bin") - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn env(&mut self, key: K, val: V) -> &mut Command @@ -659,13 +667,15 @@ impl Command { /// k == "TERM" || k == "TZ" || k == "LANG" || k == "PATH" /// ).collect(); /// - /// Command::new("printenv") + /// let status = Command::new("printenv") /// .stdin(Stdio::null()) /// .stdout(Stdio::inherit()) /// .env_clear() /// .envs(&filtered_env) - /// .spawn() + /// .status() /// .expect("printenv failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "command_envs", since = "1.19.0")] pub fn envs(&mut self, vars: I) -> &mut Command @@ -689,10 +699,12 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .env_remove("PATH") - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn env_remove>(&mut self, key: K) -> &mut Command { @@ -709,10 +721,12 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .env_clear() - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn env_clear(&mut self) -> &mut Command { @@ -737,10 +751,12 @@ impl Command { /// ```no_run /// use std::process::Command; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .current_dir("/bin") - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` /// /// [`canonicalize`]: crate::fs::canonicalize @@ -765,10 +781,12 @@ impl Command { /// ```no_run /// use std::process::{Command, Stdio}; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .stdin(Stdio::null()) - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn stdin>(&mut self, cfg: T) -> &mut Command { @@ -791,10 +809,12 @@ impl Command { /// ```no_run /// use std::process::{Command, Stdio}; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .stdout(Stdio::null()) - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn stdout>(&mut self, cfg: T) -> &mut Command { @@ -817,10 +837,12 @@ impl Command { /// ```no_run /// use std::process::{Command, Stdio}; /// - /// Command::new("ls") + /// let status = Command::new("ls") /// .stderr(Stdio::null()) - /// .spawn() + /// .status() /// .expect("ls command failed to start"); + /// + /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub fn stderr>(&mut self, cfg: T) -> &mut Command { From 0076a3b037df15824302d2d921b580f99899c86a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 17:32:04 +0000 Subject: [PATCH 04/12] std::process:Child: discuss other problems with just dropping See #70186 which makes the point about io handles. Signed-off-by: Ian Jackson --- library/std/src/process.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 3b7d808a5b96b..c9569abc622f4 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -132,15 +132,25 @@ use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; /// /// # Warning /// +/// A `Child` which is simply dropped may continue running in parallel, +/// possibly even after the program which launched it exits. +/// If it inherited stdin/stdout, it may still read and write to them +/// later, causing confusion and disruption. +/// /// On some systems, calling [`wait`] or similar is necessary for the OS to /// release resources. A process that terminated but has not been waited on is /// still around as a "zombie". Leaving too many zombies around may exhaust /// global resources (for example process IDs). /// +/// Unless [`wait`] is called, any failure of the child will not be +/// visible. +/// /// The standard library does *not* automatically wait on child processes (not /// even if the `Child` is dropped), it is up to the application developer to do -/// so. As a consequence, dropping `Child` handles without waiting on them first -/// is not recommended in long-running applications. +/// so. +/// +/// For these reasons, dropping `Child` handles without waiting on them first +/// is usually incorrect. /// /// # Examples /// From eae70c6bf5dd5b9af142f6813f25df4961bdf205 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 19:52:37 +0000 Subject: [PATCH 05/12] Command docs: Check three unchecked exit statuses These three doctests called `.status()` but ignored the result. Fix this. In two cases, with an assertion, and in the case where we are just printing something, by printing the exit status value. Signed-off-by: Ian Jackson --- library/std/src/process.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index c9569abc622f4..06f7d39284246 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -493,7 +493,8 @@ impl fmt::Debug for ChildStderr { /// let mut list_dir = Command::new("ls"); /// /// // Execute `ls` in the current directory of the program. -/// list_dir.status().expect("process failed to execute"); +/// let status = list_dir.status().expect("process failed to execute"); +/// assert!(status.success()); /// /// println!(); /// @@ -501,7 +502,8 @@ impl fmt::Debug for ChildStderr { /// list_dir.current_dir("/"); /// /// // And then execute `ls` again but in the root directory. -/// list_dir.status().expect("process failed to execute"); +/// let status = list_dir.status().expect("process failed to execute"); +/// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] pub struct Command { @@ -1596,8 +1598,8 @@ impl Child { /// /// let mut command = Command::new("ls"); /// if let Ok(mut child) = command.spawn() { - /// child.wait().expect("command wasn't running"); - /// println!("Child has finished its execution!"); + /// let status = child.wait().expect("command wasn't running"); + /// println!("Child has finished its execution, status = {}!", status); /// } else { /// println!("ls command didn't start"); /// } From 73365b261d33ee46c925e10c275516f0d7c04745 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 28 Jan 2021 12:24:00 +0000 Subject: [PATCH 06/12] bootstrap rustc shim: Report a warning if on_fail command fails Signed-off-by: Ian Jackson --- src/bootstrap/bin/rustc.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/bin/rustc.rs b/src/bootstrap/bin/rustc.rs index 3694bdbf67054..7cdd4d647c5fc 100644 --- a/src/bootstrap/bin/rustc.rs +++ b/src/bootstrap/bin/rustc.rs @@ -179,7 +179,10 @@ fn main() { } if let Some(mut on_fail) = on_fail { - on_fail.status().expect("Could not run the on_fail command"); + let status = on_fail.status().expect("Could not run the on_fail command"); + if !status.success() { + eprintln!("\nwarning: the on-fail command also failed: {}\n", status); + } } // Preserve the exit code. In case of signal, exit with 0xfe since it's From 370ab01ba807633dfaf69ffcf0145f63e8eea592 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 28 Jan 2021 13:54:45 +0000 Subject: [PATCH 07/12] runtest: Check two exit statuses from adb Signed-off-by: Ian Jackson --- src/tools/compiletest/src/runtest.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 5608ff98417cd..f78975b64584e 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -869,17 +869,23 @@ impl<'test> TestCx<'test> { let adb_path = &self.config.adb_path; - Command::new(adb_path) + let status = Command::new(adb_path) .arg("push") .arg(&exe_file) .arg(&self.config.adb_test_dir) .status() .unwrap_or_else(|_| panic!("failed to exec `{:?}`", adb_path)); + if !status.success() { + panic!("Program failed `{:}`", adb_path); + } - Command::new(adb_path) + let status = Command::new(adb_path) .args(&["forward", "tcp:5039", "tcp:5039"]) .status() .unwrap_or_else(|_| panic!("failed to exec `{:?}`", adb_path)); + if !status.success() { + panic!("Program failed `{:}`", adb_path); + } let adb_arg = format!( "export LD_LIBRARY_PATH={}; \ From d49fd08a3bbea52a1836989ae32b549410fe363d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 28 Jan 2021 15:18:09 +0000 Subject: [PATCH 08/12] fds-are-cloexec test: Check an erroneously-ignored exit status Signed-off-by: Ian Jackson --- src/test/ui/fds-are-cloexec.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/ui/fds-are-cloexec.rs b/src/test/ui/fds-are-cloexec.rs index 4482b7032a75a..465eef61a3621 100644 --- a/src/test/ui/fds-are-cloexec.rs +++ b/src/test/ui/fds-are-cloexec.rs @@ -66,7 +66,9 @@ fn parent() { .status() .unwrap(); assert!(status.success()); - child.wait().unwrap(); + + let status = child.wait().unwrap(); + assert!(status.success()); } fn child(args: &[String]) { From be40a3f9c4c3e4bcb4f8003c7de7ae71289e0c8d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 28 Jan 2021 15:18:46 +0000 Subject: [PATCH 09/12] try-wait test: Check exit status from wait for killed child We are making `ExitStatus` `#[must_use]`. So we need to explicitly handle the exit status here. Handle it the same way as the values from the repeated calls to `try_wait`. Signed-off-by: Ian Jackson --- src/test/ui/try-wait.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/ui/try-wait.rs b/src/test/ui/try-wait.rs index 692197210b15f..cdc7fe44a7d2e 100644 --- a/src/test/ui/try-wait.rs +++ b/src/test/ui/try-wait.rs @@ -31,7 +31,8 @@ fn main() { assert!(maybe_status.is_none()); me.kill().unwrap(); - me.wait().unwrap(); + let status = me.wait().unwrap(); + assert!(!status.success()); let status = me.try_wait().unwrap().unwrap(); assert!(!status.success()); From 4921c370cf2ec294fecdf2b2a5473cd7a3766915 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 17:24:09 +0000 Subject: [PATCH 10/12] Mark std::process:Child as #[must_use] Simply dropping this is usually a bad idea, for the reasons extensively discussed in the documentation. Closes: #70186 Signed-off-by: Ian Jackson --- library/std/src/process.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 06f7d39284246..6dec72a949fc2 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -170,6 +170,7 @@ use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; /// /// [`wait`]: Child::wait #[stable(feature = "process", since = "1.0.0")] +#[must_use = "this Child should probably be `wait`ed, or `status` used instead of `spawn`"] pub struct Child { handle: imp::Process, From 31f6acc1e64291dcf653e6ae7d31b616b80361d0 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 17:34:52 +0000 Subject: [PATCH 11/12] Mark std::process::Command as #[must_use] For the same reason as futures. Signed-off-by: Ian Jackson --- library/std/src/process.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 6dec72a949fc2..cdbb3888ab2dd 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -507,6 +507,7 @@ impl fmt::Debug for ChildStderr { /// assert!(status.success()); /// ``` #[stable(feature = "process", since = "1.0.0")] +#[must_use = "this Command does not do anything unless it is run, eg using `status` or `spawn`"] pub struct Command { inner: imp::Command, } From b6a8dd655c7cab7963b6d491d6add43cf0f0bffb Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Jan 2021 17:36:55 +0000 Subject: [PATCH 12/12] Mark std::process::ExitStatus as #[must_use] For the same reason as `Result` is #[must_use] Cloess: #73127 Signed-off-by: Ian Jackson --- library/std/src/process.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index cdbb3888ab2dd..78eb98f405b30 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -1416,6 +1416,7 @@ impl From for Stdio { /// [`wait`]: Child::wait #[derive(PartialEq, Eq, Clone, Copy, Debug)] #[stable(feature = "process", since = "1.0.0")] +#[must_use = "this ExitStatus might represent an error that should be checked and handled"] pub struct ExitStatus(imp::ExitStatus); impl ExitStatus {