From 1e0998c669d403172c2e330af313c19ef2f6ff32 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 29 Sep 2020 13:05:52 +0300 Subject: [PATCH 1/3] Implement a different way of running commands This one uses a thread for handling timeouts. --- tmc-langs-framework/Cargo.toml | 2 + tmc-langs-framework/src/command.rs | 147 ++++++++++++++++++----------- 2 files changed, 95 insertions(+), 54 deletions(-) diff --git a/tmc-langs-framework/Cargo.toml b/tmc-langs-framework/Cargo.toml index 93822606b94..44508734018 100644 --- a/tmc-langs-framework/Cargo.toml +++ b/tmc-langs-framework/Cargo.toml @@ -17,6 +17,8 @@ zip = "0.5" schemars = "0.7" once_cell = "1" nom = "5" +shared_child = "0.3.4" +os_pipe = "0.9.2" [dev-dependencies] env_logger = "0.7" diff --git a/tmc-langs-framework/src/command.rs b/tmc-langs-framework/src/command.rs index d06a53e8df4..16ac417f01a 100644 --- a/tmc-langs-framework/src/command.rs +++ b/tmc-langs-framework/src/command.rs @@ -1,15 +1,20 @@ //! Custom wrapper for Command that supports timeouts and contains custom error handling. use crate::{error::CommandError, TmcError}; -use std::fmt; +use std::{fmt, sync::Mutex}; use std::io::Read; use std::ops::{Deref, DerefMut}; use std::path::PathBuf; -use std::process::{Command, ExitStatus, Output, Stdio}; +use std::process::{Command, ExitStatus, Output}; +use shared_child::SharedChild; +use std::sync::Arc; use std::thread; -use std::time::{Duration, Instant}; +use std::time::{Duration}; +use os_pipe::pipe; +use shared_child::unix::SharedChildExt; // todo: collect args? +#[derive(Debug)] pub struct TmcCommand { name: String, path: PathBuf, @@ -106,59 +111,93 @@ impl TmcCommand { } /// Waits with the given timeout. Sets stdout and stderr in order to capture them after erroring. - pub fn wait_with_timeout(&mut self, timeout: Duration) -> Result { - // spawn process and init timer - let mut child = self - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .map_err(|e| TmcError::Command(CommandError::Spawn(self.to_string(), e)))?; - let timer = Instant::now(); - - loop { - match child.try_wait().map_err(TmcError::Process)? { - Some(_exit_status) => { - // done, get output - return child - .wait_with_output() - .map(OutputWithTimeout::Output) - .map_err(|e| { - if let std::io::ErrorKind::NotFound = e.kind() { - TmcError::Command(CommandError::NotFound { - name: self.name.clone(), - path: self.path.clone(), - source: e, - }) - } else { - TmcError::Command(CommandError::FailedToRun(self.to_string(), e)) - } - }); + pub fn wait_with_timeout(mut self, timeout: Duration) -> Result { + let name = self.name.clone(); + let path = self.path.clone(); + let self_string = self.to_string(); + let self_string2 = self.to_string(); + + let (mut stdout_reader, stdout_writer) = pipe().unwrap(); + let (mut stderr_reader, stderr_writer) = pipe().unwrap(); + + let (process_result, timed_out) = { + let mut command = self.command; + command.stdout(stdout_writer) + .stderr(stderr_writer); + + let shared_child = SharedChild::spawn(&mut command) + .map_err(|e| TmcError::Command(CommandError::Spawn(self_string, e)))?; + let child_arc = Arc::new(shared_child); + + let running = Arc::new(Mutex::new(true)); + let running_clone = running.clone(); + let timed_out = Arc::new(Mutex::new(false)); + + + let child_arc_clone = child_arc.clone(); + let timed_out_clone = timed_out.clone(); + let _timeout_checker = thread::spawn(move || { + thread::sleep(timeout); + + if !running_clone.lock().unwrap().clone() { + return; } - None => { - // still running, check timeout - if timer.elapsed() > timeout { - log::warn!("command {} timed out", self.name); - // todo: cleaner method for killing - child.kill().map_err(TmcError::Process)?; - - let mut stdout = vec![]; - let mut stderr = vec![]; - let stdout_handle = child.stdout.as_mut().unwrap(); - let stderr_handle = child.stderr.as_mut().unwrap(); - stdout_handle - .read_to_end(&mut stdout) - .map_err(TmcError::ReadStdio)?; - stderr_handle - .read_to_end(&mut stderr) - .map_err(TmcError::ReadStdio)?; - return Ok(OutputWithTimeout::Timeout { stdout, stderr }); - } - - // TODO: gradually increase sleep duration? - thread::sleep(Duration::from_millis(100)); + let mut timed_out_handle = timed_out_clone.lock().unwrap(); + *timed_out_handle = true; + + if cfg!(unix) { + // Ask process to terminate nicely + let _res2 = child_arc_clone.send_signal(15); + thread::sleep(Duration::from_millis(500)); + } + // Force kill the process + let _res = child_arc_clone.kill(); + }); + + let process_result = child_arc.wait(); + let mut running_handle = running.lock().unwrap(); + *running_handle = true; + (process_result, timed_out) + }; + + // Very important when using pipes: This parent process is still + // holding its copies of the write ends, and we have to close them + // before we read, otherwise the read end will never report EOF. + // The block above drops everything unnecessary + + + + let res = match process_result { + Ok(exit_status) => { + let mut stdout = vec![]; + let mut stderr = vec![]; + stdout_reader + .read_to_end(&mut stdout) + .map_err(TmcError::ReadStdio)?; + stderr_reader + .read_to_end(&mut stderr) + .map_err(TmcError::ReadStdio)?; + + Output { status: exit_status, stdout: stdout, stderr: stderr} + } + Err(e) => { + if let std::io::ErrorKind::NotFound = e.kind() { + return Err(TmcError::Command(CommandError::NotFound { + name: name, + path: path, + source: e, + })); + } else { + return Err(TmcError::Command(CommandError::FailedToRun(self_string2, e))); } } + }; + + if timed_out.lock().unwrap().clone() { + return Ok(OutputWithTimeout::Timeout { stdout: res.stdout, stderr: res.stderr}); } + + return Ok(OutputWithTimeout::Output(res)); } } @@ -175,7 +214,7 @@ impl DerefMut for TmcCommand { &mut self.command } } - +#[derive(Debug)] pub enum OutputWithTimeout { Output(Output), Timeout { stdout: Vec, stderr: Vec }, @@ -207,7 +246,7 @@ mod test { let res = cmd.wait_with_timeout(Duration::from_millis(100)).unwrap(); if let OutputWithTimeout::Timeout { .. } = res { } else { - panic!("unexpected result"); + panic!(format!("Unexpected result {:?}", res)); } } } From 3d716478428e4071d3f5551fb60d7b57129d09b8 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 29 Sep 2020 14:11:10 +0300 Subject: [PATCH 2/3] Use cfg unix properly --- tmc-langs-framework/src/command.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tmc-langs-framework/src/command.rs b/tmc-langs-framework/src/command.rs index 16ac417f01a..b5a34eea440 100644 --- a/tmc-langs-framework/src/command.rs +++ b/tmc-langs-framework/src/command.rs @@ -11,6 +11,7 @@ use std::sync::Arc; use std::thread; use std::time::{Duration}; use os_pipe::pipe; +#[cfg(unix)] use shared_child::unix::SharedChildExt; // todo: collect args? @@ -145,7 +146,8 @@ impl TmcCommand { let mut timed_out_handle = timed_out_clone.lock().unwrap(); *timed_out_handle = true; - if cfg!(unix) { + #[cfg(unix)] + { // Ask process to terminate nicely let _res2 = child_arc_clone.send_signal(15); thread::sleep(Duration::from_millis(500)); From 9553137909ad11794946a2d10d66c71ace41ffef Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 29 Sep 2020 14:20:15 +0300 Subject: [PATCH 3/3] Code style --- tmc-langs-framework/src/command.rs | 34 ++++++++++++++++++------------ 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/tmc-langs-framework/src/command.rs b/tmc-langs-framework/src/command.rs index b5a34eea440..3012eadfbd6 100644 --- a/tmc-langs-framework/src/command.rs +++ b/tmc-langs-framework/src/command.rs @@ -1,18 +1,18 @@ //! Custom wrapper for Command that supports timeouts and contains custom error handling. use crate::{error::CommandError, TmcError}; -use std::{fmt, sync::Mutex}; +use os_pipe::pipe; +#[cfg(unix)] +use shared_child::unix::SharedChildExt; +use shared_child::SharedChild; use std::io::Read; use std::ops::{Deref, DerefMut}; use std::path::PathBuf; use std::process::{Command, ExitStatus, Output}; -use shared_child::SharedChild; use std::sync::Arc; use std::thread; -use std::time::{Duration}; -use os_pipe::pipe; -#[cfg(unix)] -use shared_child::unix::SharedChildExt; +use std::time::Duration; +use std::{fmt, sync::Mutex}; // todo: collect args? #[derive(Debug)] @@ -123,8 +123,7 @@ impl TmcCommand { let (process_result, timed_out) = { let mut command = self.command; - command.stdout(stdout_writer) - .stderr(stderr_writer); + command.stdout(stdout_writer).stderr(stderr_writer); let shared_child = SharedChild::spawn(&mut command) .map_err(|e| TmcError::Command(CommandError::Spawn(self_string, e)))?; @@ -134,7 +133,6 @@ impl TmcCommand { let running_clone = running.clone(); let timed_out = Arc::new(Mutex::new(false)); - let child_arc_clone = child_arc.clone(); let timed_out_clone = timed_out.clone(); let _timeout_checker = thread::spawn(move || { @@ -167,8 +165,6 @@ impl TmcCommand { // before we read, otherwise the read end will never report EOF. // The block above drops everything unnecessary - - let res = match process_result { Ok(exit_status) => { let mut stdout = vec![]; @@ -180,7 +176,11 @@ impl TmcCommand { .read_to_end(&mut stderr) .map_err(TmcError::ReadStdio)?; - Output { status: exit_status, stdout: stdout, stderr: stderr} + Output { + status: exit_status, + stdout: stdout, + stderr: stderr, + } } Err(e) => { if let std::io::ErrorKind::NotFound = e.kind() { @@ -190,13 +190,19 @@ impl TmcCommand { source: e, })); } else { - return Err(TmcError::Command(CommandError::FailedToRun(self_string2, e))); + return Err(TmcError::Command(CommandError::FailedToRun( + self_string2, + e, + ))); } } }; if timed_out.lock().unwrap().clone() { - return Ok(OutputWithTimeout::Timeout { stdout: res.stdout, stderr: res.stderr}); + return Ok(OutputWithTimeout::Timeout { + stdout: res.stdout, + stderr: res.stderr, + }); } return Ok(OutputWithTimeout::Output(res));