From 5cf1c8a7bdc9cd385d37dd1310b09099a966796d Mon Sep 17 00:00:00 2001 From: Thomas O'Donnell Date: Thu, 21 Jan 2021 22:59:14 +0100 Subject: [PATCH] perf(utils): Add timeout to `utils::exec_cmd` (#2171) * perf(utils): Add timeout to `utils::exec_cmd` This adds a timeout to any command executed using the `utils::exec_cmd`. The initial time limit is hard coded to 500ms but if required we can make this configurable. Have also switched the tests to be a bit more granular on which systems they are ignored. * Terminate the processes if they timeout --- Cargo.lock | 16 ++++++++++++++-- Cargo.toml | 1 + src/utils.rs | 51 ++++++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d3f70ee7..dbc09ff3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -559,9 +559,9 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55" [[package]] name = "libc" -version = "0.2.81" +version = "0.2.82" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1482821306169ec4d07f6aca392a4681f66c75c9918aa49641a2595db64053cb" +checksum = "89203f3fba0a3795506acaad8ebce3c80c0af93f994d5a1d7a0b1eeb23271929" [[package]] name = "libdbus-sys" @@ -928,6 +928,17 @@ dependencies = [ "unicode-xid 0.2.1", ] +[[package]] +name = "process_control" +version = "3.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9cac0d971172c65b01208ef9567f52892fc89b1c4530c50e997c6f29da7ee961" +dependencies = [ + "crossbeam-channel", + "libc", + "winapi", +] + [[package]] name = "quick-xml" version = "0.20.0" @@ -1281,6 +1292,7 @@ dependencies = [ "path-slash", "pest", "pest_derive", + "process_control", "quick-xml", "rand 0.8.2", "rayon", diff --git a/Cargo.toml b/Cargo.toml index a6c26a34..da3af3a9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -64,6 +64,7 @@ indexmap = "1.6.1" notify-rust = { version = "4.2.2", optional = true } semver = "0.11.0" which = "4.0.2" +process_control = { version = "3.0.0", features = ["crossbeam-channel"] } # Optional/http: attohttpc = { version = "0.16.1", optional = true, default-features = false, features = ["tls", "form"] } diff --git a/src/utils.rs b/src/utils.rs index b866a2bd..68435288 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,8 +1,9 @@ +use process_control::{ChildExt, Timeout}; use std::fs::File; use std::io::{Read, Result}; use std::path::Path; -use std::process::Command; -use std::time::Instant; +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; use crate::context::Shell; @@ -258,15 +259,31 @@ fn internal_exec_cmd(cmd: &str, args: &[&str]) -> Option { log::trace!("Using {:?} as {:?}", full_path, cmd); full_path } - Err(e) => { - log::trace!("Unable to find {:?} in PATH, {:?}", cmd, e); + Err(error) => { + log::trace!("Unable to find {:?} in PATH, {:?}", cmd, error); return None; } }; + let time_limit = Duration::from_millis(500); + let start = Instant::now(); - match Command::new(full_path).args(args).output() { - Ok(output) => { + + let process = match Command::new(full_path) + .args(args) + .stderr(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + { + Ok(process) => process, + Err(error) => { + log::info!("Unable to run {:?}, {:?}", cmd, error); + return None; + } + }; + + match process.with_output_timeout(time_limit).terminating().wait() { + Ok(Some(output)) => { let stdout_string = String::from_utf8(output.stdout).unwrap(); let stderr_string = String::from_utf8(output.stderr).unwrap(); @@ -287,6 +304,10 @@ fn internal_exec_cmd(cmd: &str, args: &[&str]) -> Option { stderr: stderr_string, }) } + Ok(None) => { + log::warn!("Executing command {:?} timed out", cmd); + None + } Err(error) => { log::info!("Executing command {:?} failed by: {:?}", cmd, error); None @@ -295,7 +316,6 @@ fn internal_exec_cmd(cmd: &str, args: &[&str]) -> Option { } #[cfg(test)] -#[cfg(not(windows))] // While the exec_cmd should work on Windows these tests assume a Unix-like environment. mod tests { use super::*; @@ -310,7 +330,11 @@ mod tests { assert_eq!(result, expected) } + // While the exec_cmd should work on Windows some of these tests assume a Unix-like + // environment. + #[test] + #[cfg(not(windows))] fn exec_no_output() { let result = internal_exec_cmd("true", &[]); let expected = Some(CommandOutput { @@ -322,6 +346,7 @@ mod tests { } #[test] + #[cfg(not(windows))] fn exec_with_output_stdout() { let result = internal_exec_cmd("/bin/sh", &["-c", "echo hello"]); let expected = Some(CommandOutput { @@ -333,6 +358,7 @@ mod tests { } #[test] + #[cfg(not(windows))] fn exec_with_output_stderr() { let result = internal_exec_cmd("/bin/sh", &["-c", "echo hello >&2"]); let expected = Some(CommandOutput { @@ -344,6 +370,7 @@ mod tests { } #[test] + #[cfg(not(windows))] fn exec_with_output_both() { let result = internal_exec_cmd("/bin/sh", &["-c", "echo hello; echo world >&2"]); let expected = Some(CommandOutput { @@ -355,6 +382,7 @@ mod tests { } #[test] + #[cfg(not(windows))] fn exec_with_non_zero_exit_code() { let result = internal_exec_cmd("false", &[]); let expected = None; @@ -362,6 +390,15 @@ mod tests { assert_eq!(result, expected) } + #[test] + #[cfg(not(windows))] + fn exec_slow_command() { + let result = internal_exec_cmd("sleep", &["500"]); + let expected = None; + + assert_eq!(result, expected) + } + #[test] fn test_color_sequence_wrappers() { let test0 = "\x1b2mhellomynamekeyes\x1b2m"; // BEGIN: \x1b END: m