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/Cargo.lock b/Cargo.lock index 49fd7d4fe..e81300286 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -347,6 +347,7 @@ dependencies = [ "rand_pcg", "regex", "rlimit", + "rstest", "selinux", "sha1", "tempfile", @@ -884,6 +885,101 @@ dependencies = [ "libc", ] +[[package]] +name = "futures" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "38390104763dc37a5145a53c29c63c1290b5d316d6086ec32c293f6736051bb0" +dependencies = [ + "futures-channel", + "futures-core", + "futures-executor", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + +[[package]] +name = "futures-channel" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52ba265a92256105f45b719605a571ffe2d1f0fea3807304b522c1d778f79eed" +dependencies = [ + "futures-core", + "futures-sink", +] + +[[package]] +name = "futures-core" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "04909a7a7e4633ae6c4a9ab280aeb86da1236243a77b694a49eacd659a4bd3ac" + +[[package]] +name = "futures-executor" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7acc85df6714c176ab5edf386123fafe217be88c0840ec11f199441134a074e2" +dependencies = [ + "futures-core", + "futures-task", + "futures-util", +] + +[[package]] +name = "futures-io" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "00f5fb52a06bdcadeb54e8d3671f8888a39697dcb0b81b23b55174030427f4eb" + +[[package]] +name = "futures-macro" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bdfb8ce053d86b91919aad980c220b1fb8401a9394410e1c289ed7e66b61835d" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "futures-sink" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "39c15cf1a4aa79df40f1bb462fb39676d0ad9e366c2a33b590d7c66f4f81fcf9" + +[[package]] +name = "futures-task" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2ffb393ac5d9a6eaa9d3fdf37ae2776656b706e200c8e16b1bdb227f5198e6ea" + +[[package]] +name = "futures-timer" +version = "3.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e64b03909df88034c26dc1547e8970b91f98bdb65165d6a4e9110d94263dbb2c" + +[[package]] +name = "futures-util" +version = "0.3.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "197676987abd2f9cadff84926f410af1c183608d36641465df73ae8211dc65d6" +dependencies = [ + "futures-channel", + "futures-core", + "futures-io", + "futures-macro", + "futures-sink", + "futures-task", + "memchr", + "pin-project-lite", + "pin-utils", + "slab", +] + [[package]] name = "gcd" version = "2.1.0" @@ -1517,6 +1613,12 @@ dependencies = [ "siphasher", ] +[[package]] +name = "pin-project-lite" +version = "0.2.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e0a7ae3ac2f1173085d398531c705756c94a4c56843785df85a60c1a0afac116" + [[package]] name = "pin-utils" version = "0.1.0" @@ -1772,6 +1874,32 @@ dependencies = [ "libc", ] +[[package]] +name = "rstest" +version = "0.16.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b07f2d176c472198ec1e6551dc7da28f1c089652f66a7b722676c2238ebc0edf" +dependencies = [ + "futures", + "futures-timer", + "rstest_macros", + "rustc_version", +] + +[[package]] +name = "rstest_macros" +version = "0.16.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7229b505ae0706e64f37ffc54a9c163e11022a6636d58fe1f3f52018257ff9f7" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "rustc_version", + "syn", + "unicode-ident", +] + [[package]] name = "rust-ini" version = "0.18.0" @@ -1788,6 +1916,15 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" +[[package]] +name = "rustc_version" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +dependencies = [ + "semver", +] + [[package]] name = "rustix" version = "0.35.13" @@ -1855,6 +1992,12 @@ dependencies = [ "walkdir", ] +[[package]] +name = "semver" +version = "1.0.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e25dfac463d778e353db5be2449d1cce89bd6fd23c9f1ea21310ce6e5a1b29c4" + [[package]] name = "serde" version = "1.0.147" @@ -1935,6 +2078,15 @@ version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7bd3e3206899af3f8b12af284fafc038cc1dc2b41d1b89dd17297221c5d225de" +[[package]] +name = "slab" +version = "0.4.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4614a76b2a8be0058caa9dbbaf66d988527d86d003c11a94fbd335d7661edcef" +dependencies = [ + "autocfg", +] + [[package]] name = "smallvec" version = "1.10.0" diff --git a/Cargo.toml b/Cargo.toml index 4ad55d4de..dfd853997 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -400,6 +400,7 @@ uucore = { version=">=0.0.16", package="uucore", path="src/uucore", features=["e walkdir = "2.2" atty = "0.2" hex-literal = "0.3.1" +rstest = "0.16.0" [target.'cfg(any(target_os = "linux", target_os = "android"))'.dev-dependencies] procfs = { version = "0.14", default-features = false } diff --git a/tests/by-util/test_factor.rs b/tests/by-util/test_factor.rs index f84a86aa9..6a17c00ff 100644 --- a/tests/by-util/test_factor.rs +++ b/tests/by-util/test_factor.rs @@ -10,7 +10,7 @@ use crate::common::util::*; -use std::time::SystemTime; +use std::time::{Duration, SystemTime}; #[path = "../../src/uu/factor/sieve.rs"] mod sieve; @@ -35,7 +35,7 @@ fn test_invalid_arg() { fn test_parallel() { use hex_literal::hex; use sha1::{Digest, Sha1}; - use std::fs::OpenOptions; + use std::{fs::OpenOptions, time::Duration}; use tempfile::TempDir; // factor should only flush the buffer at line breaks let n_integers = 100_000; @@ -55,6 +55,7 @@ fn test_parallel() { for child in (0..10) .map(|_| { new_ucmd!() + .timeout(Duration::from_secs(240)) .set_stdout(output.try_clone().unwrap()) .pipe_in(input_string.clone()) .run_no_wait() @@ -275,6 +276,7 @@ fn run(input_string: &[u8], output_string: &[u8]) { ); // now run factor new_ucmd!() + .timeout(Duration::from_secs(240)) .pipe_in(input_string) .run() .stdout_is(String::from_utf8(output_string.to_owned()).unwrap()); diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 6e71f2664..a03217cb9 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -5,6 +5,8 @@ // spell-checker:ignore (words) ints +use std::time::Duration; + use crate::common::util::*; fn test_helper(file_name: &str, possible_args: &[&str]) { @@ -948,6 +950,7 @@ fn test_compress_fail() { fn test_merge_batches() { TestScenario::new(util_name!()) .ucmd_keepenv() + .timeout(Duration::from_secs(120)) .args(&["ext_sort.txt", "-n", "-S", "150b"]) .succeeds() .stdout_only_fixture("ext_sort.expected"); diff --git a/tests/common/util.rs b/tests/common/util.rs index c0d6b7df8..fb3c8fb8d 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().expect("Panic caused disconnect").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(); + } }