diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index fd150a8af..d6e91dca5 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -36,6 +36,7 @@ quickcheck rand_chacha ringbuffer rlimit +rstest smallvec tempdir tempfile diff --git a/tests/common/util.rs b/tests/common/util.rs index c0d6b7df8..312f4278a 100644 --- a/tests/common/util.rs +++ b/tests/common/util.rs @@ -10,6 +10,7 @@ use pretty_assertions::assert_eq; #[cfg(target_os = "linux")] use rlimit::prlimit; +use rstest::rstest; #[cfg(unix)] use std::borrow::Cow; #[cfg(not(windows))] @@ -26,9 +27,10 @@ use std::path::MAIN_SEPARATOR; use std::path::{Path, PathBuf}; use std::process::{Child, Command, Output, Stdio}; use std::rc::Rc; +use std::sync::mpsc::{self, RecvTimeoutError}; use std::thread::{sleep, JoinHandle}; -use std::time::Duration; -use std::{env, thread}; +use std::time::{Duration, Instant}; +use std::{env, hint, thread}; use tempfile::{Builder, TempDir}; use uucore::Args; @@ -950,6 +952,7 @@ pub struct UCommand { #[cfg(any(target_os = "linux", target_os = "android"))] limits: Vec<(rlimit::Resource, u64, u64)>, stderr_to_stdout: bool, + timeout: Option, tmpd: Option>, // drop last } @@ -1000,6 +1003,7 @@ impl UCommand { #[cfg(any(target_os = "linux", target_os = "android"))] limits: vec![], stderr_to_stdout: false, + timeout: Some(Duration::from_secs(30)), }; if let Some(un) = util_name { @@ -1116,6 +1120,18 @@ impl UCommand { self } + /// Set the timeout for [`UCommand::run`] and similar methods in [`UCommand`]. + /// + /// After the timeout elapsed these `run` methods (besides [`UCommand::run_no_wait`]) will + /// panic. When [`UCommand::run_no_wait`] is used, this timeout is applied to + /// [`UChild::wait_with_output`] including all other waiting methods in [`UChild`] implicitly + /// using `wait_with_output()` and additionally [`UChild::kill`]. The default timeout of `kill` + /// will be overwritten by this `timeout`. + pub fn timeout(&mut self, timeout: Duration) -> &mut Self { + self.timeout = Some(timeout); + self + } + /// Spawns the command, feeds the stdin if any, and returns the /// child process immediately. pub fn run_no_wait(&mut self) -> UChild { @@ -1449,6 +1465,7 @@ pub struct UChild { ignore_stdin_write_error: bool, stderr_to_stdout: bool, join_handle: Option>>, + timeout: Option, tmpd: Option>, // drop last } @@ -1468,6 +1485,7 @@ impl UChild { ignore_stdin_write_error: ucommand.ignore_stdin_write_error, stderr_to_stdout: ucommand.stderr_to_stdout, join_handle: None, + timeout: ucommand.timeout, tmpd: ucommand.tmpd.clone(), } } @@ -1504,30 +1522,52 @@ impl UChild { self.delay(millis).make_assertion() } - /// Try to kill the child process. + /// Try to kill the child process and wait for it's termination. /// - /// # Panics - /// If the child process could not be terminated within 60 seconds. + /// This method blocks until the child process is killed, but returns an error if `self.timeout` + /// or the default of 60s was reached. If no such error happened, the process resources are + /// released, so there is usually no need to call `wait` or alike on unix systems although it's + /// still possible to do so. + /// + /// # Platform specific behavior + /// + /// On unix systems the child process resources will be released like a call to [`Child::wait`] + /// or alike would do. + /// + /// # Error + /// + /// If [`Child::kill`] returned an error or if the child process could not be terminated within + /// `self.timeout` or the default of 60s. pub fn try_kill(&mut self) -> io::Result<()> { + let start = Instant::now(); self.raw.kill()?; - for _ in 0..60 { - if !self.is_alive() { - return Ok(()); + + let timeout = self.timeout.unwrap_or(Duration::from_secs(60)); + // As a side effect, we're cleaning up the killed child process with the implicit call to + // `Child::try_wait` in `self.is_alive`, which reaps the process id on unix systems. We + // always fail with error on timeout if `self.timeout` is set to zero. + while self.is_alive() || timeout == Duration::ZERO { + if start.elapsed() < timeout { + self.delay(10); + } else { + return Err(io::Error::new( + io::ErrorKind::Other, + format!("kill: Timeout of '{}s' reached", timeout.as_secs_f64()), + )); } - sleep(Duration::from_secs(1)); + hint::spin_loop(); } - Err(io::Error::new( - io::ErrorKind::Other, - "Killing the child process within 60 seconds failed.", - )) + + Ok(()) } /// Terminate the child process unconditionally and wait for the termination. /// - /// Ignores any errors happening during [`Child::kill`] (i.e. child process already exited). + /// Ignores any errors happening during [`Child::kill`] (i.e. child process already exited) but + /// still panics on timeout. /// /// # Panics - /// If the child process could not be terminated within 60 seconds. + /// If the child process could not be terminated within `self.timeout` or the default of 60s. pub fn kill(&mut self) -> &mut Self { self.try_kill() .or_else(|error| { @@ -1544,10 +1584,12 @@ impl UChild { /// Wait for the child process to terminate and return a [`CmdResult`]. /// - /// This method can also be run if the child process was killed with [`UChild::kill`]. + /// See [`UChild::wait_with_output`] for details on timeouts etc. This method can also be run if + /// the child process was killed with [`UChild::kill`]. /// /// # Errors - /// Returns the error from the call to [`Child::wait_with_output`] if any + /// + /// Returns the error from the call to [`UChild::wait_with_output`] if any pub fn wait(self) -> io::Result { let (bin_path, util_name, tmpd) = ( self.bin_path.clone(), @@ -1571,10 +1613,43 @@ impl UChild { /// Wait for the child process to terminate and return an instance of [`Output`]. /// - /// Joins with the thread created by [`UChild::pipe_in`] if any. + /// If `self.timeout` is reached while waiting, a [`io::ErrorKind::Other`] representing a + /// timeout error is returned. If no errors happened, we join with the thread created by + /// [`UChild::pipe_in`] if any. + /// + /// # Error + /// + /// If `self.timeout` is reached while waiting or [`Child::wait_with_output`] returned an + /// error. #[deprecated = "Please use wait() -> io::Result instead."] pub fn wait_with_output(mut self) -> io::Result { - let mut output = self.raw.wait_with_output()?; + let output = if let Some(timeout) = self.timeout { + let child = self.raw; + + let (sender, receiver) = mpsc::channel(); + let handle = thread::spawn(move || sender.send(child.wait_with_output())); + + match receiver.recv_timeout(timeout) { + Ok(result) => { + // unwraps are safe here because we got a result from the sender and there was no panic + // causing a disconnect. + handle.join().unwrap().unwrap(); + result + } + Err(RecvTimeoutError::Timeout) => Err(io::Error::new( + io::ErrorKind::Other, + format!("wait: Timeout of '{}s' reached", timeout.as_secs_f64()), + )), + Err(RecvTimeoutError::Disconnected) => { + handle.join().unwrap().unwrap(); + panic!("Error receiving from waiting thread because of unexpected disconnect") + } + } + } else { + self.raw.wait_with_output() + }; + + let mut output = output?; if let Some(join_handle) = self.join_handle.take() { join_handle @@ -2703,4 +2778,90 @@ mod tests { .stdout_matches(&Regex::new("[\0].*").unwrap()) .no_stderr(); } + + #[cfg(feature = "sleep")] + #[test] + fn test_uchild_when_wait_and_timeout_is_reached_then_timeout_error() { + let ts = TestScenario::new("sleep"); + let child = ts + .ucmd() + .timeout(Duration::from_secs(1)) + .arg("10.0") + .run_no_wait(); + + match child.wait() { + Err(error) if error.kind() == io::ErrorKind::Other => { + std::assert_eq!(error.to_string(), "wait: Timeout of '1s' reached"); + } + Err(error) => panic!( + "Assertion failed: Expected error with timeout but was: {}", + error + ), + Ok(_) => panic!("Assertion failed: Expected timeout of `wait`."), + } + } + + #[cfg(feature = "sleep")] + #[rstest] + #[timeout(Duration::from_secs(5))] + fn test_uchild_when_kill_and_timeout_higher_than_kill_time_then_no_panic() { + let ts = TestScenario::new("sleep"); + let mut child = ts + .ucmd() + .timeout(Duration::from_secs(60)) + .arg("20.0") + .run_no_wait(); + + child.kill().make_assertion().is_not_alive(); + } + + #[cfg(feature = "sleep")] + #[test] + fn test_uchild_when_try_kill_and_timeout_is_reached_then_error() { + let ts = TestScenario::new("sleep"); + let mut child = ts.ucmd().timeout(Duration::ZERO).arg("10.0").run_no_wait(); + + match child.try_kill() { + Err(error) if error.kind() == io::ErrorKind::Other => { + std::assert_eq!(error.to_string(), "kill: Timeout of '0s' reached"); + } + Err(error) => panic!( + "Assertion failed: Expected error with timeout but was: {}", + error + ), + Ok(_) => panic!("Assertion failed: Expected timeout of `try_kill`."), + } + } + + #[cfg(feature = "sleep")] + #[test] + #[should_panic = "kill: Timeout of '0s' reached"] + fn test_uchild_when_kill_with_timeout_and_timeout_is_reached_then_panic() { + let ts = TestScenario::new("sleep"); + let mut child = ts.ucmd().timeout(Duration::ZERO).arg("10.0").run_no_wait(); + + child.kill(); + panic!("Assertion failed: Expected timeout of `kill`."); + } + + #[cfg(feature = "sleep")] + #[test] + #[should_panic(expected = "wait: Timeout of '1.1s' reached")] + fn test_ucommand_when_run_with_timeout_and_timeout_is_reached_then_panic() { + let ts = TestScenario::new("sleep"); + ts.ucmd() + .timeout(Duration::from_millis(1100)) + .arg("10.0") + .run(); + + panic!("Assertion failed: Expected timeout of `run`.") + } + + #[cfg(feature = "sleep")] + #[rstest] + #[timeout(Duration::from_secs(10))] + fn test_ucommand_when_run_with_timeout_higher_then_execution_time_then_no_panic() { + let ts = TestScenario::new("sleep"); + ts.ucmd().timeout(Duration::from_secs(60)).arg("1.0").run(); + } }