From e7fda447e7d05b6ca431fc8fe8f489b1fda810bc Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Tue, 13 Jun 2023 20:32:31 +0100 Subject: [PATCH 1/3] Return `Ok` on kill if process has already exited --- library/std/src/process.rs | 6 +++--- library/std/src/sys/unix/process/process_unix.rs | 7 ++----- library/std/src/sys/unix/process/process_vxworks.rs | 7 ++----- library/std/src/sys/windows/process.rs | 11 ++++++++++- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 9da74a5ddb5..8f3201b0091 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -1904,8 +1904,8 @@ impl FromInner for ExitCode { } impl Child { - /// Forces the child process to exit. If the child has already exited, an [`InvalidInput`] - /// error is returned. + /// Forces the child process to exit. If the child has already exited, `Ok(())` + /// is returned. /// /// The mapping to [`ErrorKind`]s is not part of the compatibility contract of the function. /// @@ -1920,7 +1920,7 @@ impl Child { /// /// let mut command = Command::new("yes"); /// if let Ok(mut child) = command.spawn() { - /// child.kill().expect("command wasn't running"); + /// child.kill().expect("command couldn't be killed"); /// } else { /// println!("yes command didn't start"); /// } diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 3721988b405..d527e1d7ead 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -719,12 +719,9 @@ impl Process { 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. + // random processes, so return Ok because the process has exited already. if self.status.is_some() { - Err(io::const_io_error!( - ErrorKind::InvalidInput, - "invalid argument: can't kill an exited process", - )) + Ok(()) } else { cvt(unsafe { libc::kill(self.pid, libc::SIGKILL) }).map(drop) } diff --git a/library/std/src/sys/unix/process/process_vxworks.rs b/library/std/src/sys/unix/process/process_vxworks.rs index c40e7ada03c..f70d3cb396b 100644 --- a/library/std/src/sys/unix/process/process_vxworks.rs +++ b/library/std/src/sys/unix/process/process_vxworks.rs @@ -144,12 +144,9 @@ impl Process { 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. + // random processes, so return Ok because the process has exited already. if self.status.is_some() { - Err(io::const_io_error!( - ErrorKind::InvalidInput, - "invalid argument: can't kill an exited process", - )) + Ok(()) } else { cvt(unsafe { libc::kill(self.pid, libc::SIGKILL) }).map(drop) } diff --git a/library/std/src/sys/windows/process.rs b/library/std/src/sys/windows/process.rs index a573a05c39c..e3493cbb850 100644 --- a/library/std/src/sys/windows/process.rs +++ b/library/std/src/sys/windows/process.rs @@ -595,7 +595,16 @@ pub struct Process { impl Process { pub fn kill(&mut self) -> io::Result<()> { - cvt(unsafe { c::TerminateProcess(self.handle.as_raw_handle(), 1) })?; + let result = unsafe { c::TerminateProcess(self.handle.as_raw_handle(), 1) }; + if result == c::FALSE { + let error = unsafe { c::GetLastError() }; + // TerminateProcess returns ERROR_ACCESS_DENIED if the process has already been + // terminated (by us, or for any other reason). So check if the process was actually + // terminated, and if so, do not return an error. + if error != c::ERROR_ACCESS_DENIED || self.try_wait().is_err() { + return Err(crate::io::Error::from_raw_os_error(error as i32)); + } + } Ok(()) } From 4309954187657bddc8daba59187a463d612cf3f9 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Sat, 1 Jul 2023 01:34:06 +0100 Subject: [PATCH 2/3] Test Child::kill behaviour on exited process --- library/std/src/process/tests.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/library/std/src/process/tests.rs b/library/std/src/process/tests.rs index d7f4d335de3..e2c56e634d9 100644 --- a/library/std/src/process/tests.rs +++ b/library/std/src/process/tests.rs @@ -582,3 +582,11 @@ fn run_canonical_bat_script() { assert!(output.status.success()); assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), "Hello, fellow Rustaceans!"); } + +#[test] +fn terminate_exited_process() { + let mut p = known_command().arg("hello").spawn().unwrap(); + p.wait().unwrap(); + assert!(p.kill().is_ok()); + assert!(p.kill().is_ok()); +} From 9e5f61fcdd29936938d401c0f0055b9af32caf5c Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 5 Jul 2023 09:54:16 +0100 Subject: [PATCH 3/3] Workaround for old android not having echo --- library/std/src/process/tests.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/library/std/src/process/tests.rs b/library/std/src/process/tests.rs index e2c56e634d9..366b591466c 100644 --- a/library/std/src/process/tests.rs +++ b/library/std/src/process/tests.rs @@ -585,7 +585,14 @@ fn run_canonical_bat_script() { #[test] fn terminate_exited_process() { - let mut p = known_command().arg("hello").spawn().unwrap(); + let mut cmd = if cfg!(target_os = "android") { + let mut p = shell_cmd(); + p.args(&["-c", "true"]); + p + } else { + known_command() + }; + let mut p = cmd.stdout(Stdio::null()).spawn().unwrap(); p.wait().unwrap(); assert!(p.kill().is_ok()); assert!(p.kill().is_ok());