From e91da593ed883d45faaa27f00515db19d3f67d11 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sat, 2 Aug 2025 18:13:36 +0800 Subject: [PATCH 1/6] Experiment with fixing wait_with_pipe() --- src/child.rs | 29 +++++++++++++++++++-- tests/test_macros.rs | 60 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/src/child.rs b/src/child.rs index 973af0c..bbf0a0b 100644 --- a/src/child.rs +++ b/src/child.rs @@ -115,8 +115,17 @@ impl FunChildren { } } - /// Waits for the children processes to exit completely, pipe content will be processed by - /// provided function. + /// Pipes stdout from the last child in the pipeline to the given function, which runs in + /// **the current thread**, then waits for all of the children to exit. + /// + ///
+ /// + /// # Bugs + /// + /// The exit status of the last child is **ignored**. If the function returns early, without + /// reading from stdout until the last child exits, then the last child may be killed instead + /// of being waited for. To avoid these limitations, use [`Self::wait_with_stdout_thread`]. + ///
pub fn wait_with_pipe(&mut self, f: &mut dyn FnMut(Box)) -> CmdResult { let child = self.children.pop().unwrap(); let stderr_thread = @@ -143,6 +152,22 @@ impl FunChildren { CmdChildren::wait_children(&mut self.children) } + /// Pipes stdout from the last child in the pipeline to the given function, which runs in + /// **a new thread**, then waits for all of the children to exit. + pub fn wait_with_pipe_thread( + &mut self, + f: impl FnOnce(Box) + Send + 'static, + ) -> CmdResult { + if let Some(stdout) = self.children.last_mut().unwrap().stdout.take() { + let thread = std::thread::spawn(|| f(Box::new(stdout))); + let wait_res = self.wait_with_output().map(|_| ()); + thread.join().expect("stdout thread panicked"); + return wait_res; + } + + Ok(()) + } + /// Returns the OS-assigned process identifiers associated with these children processes. pub fn pids(&self) -> Vec { self.children.iter().filter_map(|x| x.pid()).collect() diff --git a/tests/test_macros.rs b/tests/test_macros.rs index ff4dbce..9ff7fcf 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -134,7 +134,7 @@ fn test_tls_set() { } #[test] -fn test_pipe() { +fn test_pipe() -> CmdResult { assert!(run_cmd!(echo "xx").is_ok()); assert_eq!(run_fun!(echo "xx").unwrap(), "xx"); assert!(run_cmd!(echo xx | wc).is_ok()); @@ -271,6 +271,64 @@ fn test_pipe() { } assert!(ok); + + // test that illustrates the bugs in wait_with_pipe() + // FIXME: make set_pipefail() thread safe, then move this to a separate test function + assert!(spawn_with_output!(false)?.wait_with_all().0.is_err()); + assert!(spawn_with_output!(false)?.wait_with_output().is_err()); + assert!(spawn_with_output!(false)? + .wait_with_raw_output(&mut vec![]) + .is_err()); + + // wait_with_pipe() can’t check the exit status of the last child + assert!(spawn_with_output!(false)? + .wait_with_pipe(&mut |_stdout| {}) + .is_ok()); + + // wait_with_pipe() kills the last child when the provided function returns + assert!(spawn_with_output!(sh -c "while :; do :; done")? + .wait_with_pipe(&mut |_stdout| {}) + .is_ok()); + + // wait_with_pipe_thread() checks the exit status of the last child, even if pipefail is disabled + set_pipefail(false); + assert!(spawn_with_output!(true | false)? + .wait_with_pipe_thread(|_stdout| {}) + .is_err()); + assert!(spawn_with_output!(true | true)? + .wait_with_pipe_thread(|_stdout| {}) + .is_ok()); + assert!(spawn_with_output!(false)? + .wait_with_pipe_thread(|_stdout| {}) + .is_err()); + assert!(spawn_with_output!(true)? + .wait_with_pipe_thread(|_stdout| {}) + .is_ok()); + set_pipefail(true); + // wait_with_pipe_thread() checks the exit status of the other children, unless pipefail is disabled + set_pipefail(false); + assert!(spawn_with_output!(false | true)? + .wait_with_pipe_thread(|_stdout| {}) + .is_ok()); + set_pipefail(true); + assert!(spawn_with_output!(false | true)? + .wait_with_pipe_thread(|_stdout| {}) + .is_err()); + assert!(spawn_with_output!(true | true)? + .wait_with_pipe_thread(|_stdout| {}) + .is_ok()); + // wait_with_pipe_thread() handles `ignore` + assert!(spawn_with_output!(ignore false | true)? + .wait_with_pipe_thread(|_stdout| {}) + .is_ok()); + assert!(spawn_with_output!(ignore true | false)? + .wait_with_pipe_thread(|_stdout| {}) + .is_ok()); + assert!(spawn_with_output!(ignore false)? + .wait_with_pipe_thread(|_stdout| {}) + .is_ok()); + + Ok(()) } #[test] From d2339127afe7b21582666213bbc69734ab8d1eb4 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sun, 3 Aug 2025 00:08:38 +0800 Subject: [PATCH 2/6] Experiment with fixing wait_with_pipe() another way --- src/child.rs | 57 ++++++++++++++++++++++++++++++++++---------- tests/test_macros.rs | 26 ++++++++++---------- 2 files changed, 57 insertions(+), 26 deletions(-) diff --git a/src/child.rs b/src/child.rs index bbf0a0b..a10c4e4 100644 --- a/src/child.rs +++ b/src/child.rs @@ -116,7 +116,7 @@ impl FunChildren { } /// Pipes stdout from the last child in the pipeline to the given function, which runs in - /// **the current thread**, then waits for all of the children to exit. + /// the current thread, then waits for all of the children to exit. /// ///
/// @@ -153,19 +153,50 @@ impl FunChildren { } /// Pipes stdout from the last child in the pipeline to the given function, which runs in - /// **a new thread**, then waits for all of the children to exit. - pub fn wait_with_pipe_thread( - &mut self, - f: impl FnOnce(Box) + Send + 'static, - ) -> CmdResult { - if let Some(stdout) = self.children.last_mut().unwrap().stdout.take() { - let thread = std::thread::spawn(|| f(Box::new(stdout))); - let wait_res = self.wait_with_output().map(|_| ()); - thread.join().expect("stdout thread panicked"); - return wait_res; - } + /// the current thread, then waits for all of the children to exit. + /// + /// If the function returns early, without reading from stdout until the last child exits, + /// then the rest of stdout is automatically read and discarded to allow the child to finish. + pub fn wait_with_borrowed_pipe(&mut self, f: &mut dyn FnMut(&mut Box)) -> CmdResult { + let mut last_child = self.children.pop().unwrap(); + let mut stderr_thread = StderrThread::new( + &last_child.cmd, + &last_child.file, + last_child.line, + last_child.stderr.take(), + false, + ); + let last_child_res = if let Some(stdout) = last_child.stdout { + let mut stdout: Box = Box::new(stdout); + f(&mut stdout); + let mut buf = vec![0; 65536]; + let status = loop { + if let CmdChildHandle::Proc(child) = &mut last_child.handle { + if let Ok(Some(status)) = child.try_wait() { + break status; + } + } + let _ = stdout.read(&mut buf); + }; + if status.success() { + Ok(()) + } else { + Err(CmdChildHandle::status_to_io_error( + status, + &last_child.cmd, + &last_child.file, + last_child.line, + )) + } + } else { + last_child.wait(true) + }; + let other_children_res = CmdChildren::wait_children(&mut self.children); + let _ = stderr_thread.join(); - Ok(()) + self.ignore_error + .then_some(Ok(())) + .unwrap_or(last_child_res.and(other_children_res)) } /// Returns the OS-assigned process identifiers associated with these children processes. diff --git a/tests/test_macros.rs b/tests/test_macros.rs index 9ff7fcf..814d009 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -290,42 +290,42 @@ fn test_pipe() -> CmdResult { .wait_with_pipe(&mut |_stdout| {}) .is_ok()); - // wait_with_pipe_thread() checks the exit status of the last child, even if pipefail is disabled + // wait_with_borrowed_pipe() checks the exit status of the last child, even if pipefail is disabled set_pipefail(false); assert!(spawn_with_output!(true | false)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_err()); assert!(spawn_with_output!(true | true)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_ok()); assert!(spawn_with_output!(false)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_err()); assert!(spawn_with_output!(true)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_ok()); set_pipefail(true); - // wait_with_pipe_thread() checks the exit status of the other children, unless pipefail is disabled + // wait_with_borrowed_pipe() checks the exit status of the other children, unless pipefail is disabled set_pipefail(false); assert!(spawn_with_output!(false | true)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_ok()); set_pipefail(true); assert!(spawn_with_output!(false | true)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_err()); assert!(spawn_with_output!(true | true)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_ok()); - // wait_with_pipe_thread() handles `ignore` + // wait_with_borrowed_pipe() handles `ignore` assert!(spawn_with_output!(ignore false | true)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_ok()); assert!(spawn_with_output!(ignore true | false)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_ok()); assert!(spawn_with_output!(ignore false)? - .wait_with_pipe_thread(|_stdout| {}) + .wait_with_borrowed_pipe(&mut |_stdout| {}) .is_ok()); Ok(()) From 629c9eea6900c5ec3d28fa233edf9b259103ce03 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sun, 3 Aug 2025 14:14:38 +0800 Subject: [PATCH 3/6] Add wait_with_borrowed_pipe() to ignore/pipefail test suite --- tests/test_macros.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_macros.rs b/tests/test_macros.rs index 814d009..96c65a3 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -233,10 +233,9 @@ fn test_pipe() -> CmdResult { test_cases_for_entry_point!((spawn_with_output!(...)) .unwrap() .wait_with_raw_output(&mut vec![])), - // FIXME: wait_with_pipe() is currently busted - // test_cases_for_entry_point!((spawn_with_output!(...)) - // .unwrap() - // .wait_with_pipe(&mut |_stdout| {})), + test_cases_for_entry_point!((spawn_with_output!(...)) + .unwrap() + .wait_with_borrowed_pipe(&mut |_stdout| {})), ]; macro_rules! check_eq { From ff05adbde57dd496c4b6cc12453e53c7c50cc867 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sun, 3 Aug 2025 14:07:56 +0800 Subject: [PATCH 4/6] Fix hang when wait_with_borrowed_pipe() called with built-ins --- src/child.rs | 124 +++++++++++++++++++++++++++++++++++-------- src/process.rs | 2 +- tests/test_macros.rs | 11 ++++ 3 files changed, 113 insertions(+), 24 deletions(-) diff --git a/src/child.rs b/src/child.rs index a10c4e4..3b97bfc 100644 --- a/src/child.rs +++ b/src/child.rs @@ -1,6 +1,8 @@ use crate::{info, warn}; use crate::{process, CmdResult, FunResult}; use os_pipe::PipeReader; +use std::any::Any; +use std::fmt::Display; use std::io::{BufRead, BufReader, Error, ErrorKind, Read, Result}; use std::process::{Child, ExitStatus}; use std::thread::JoinHandle; @@ -170,10 +172,24 @@ impl FunChildren { let mut stdout: Box = Box::new(stdout); f(&mut stdout); let mut buf = vec![0; 65536]; - let status = loop { - if let CmdChildHandle::Proc(child) = &mut last_child.handle { - if let Ok(Some(status)) = child.try_wait() { - break status; + let status: Box = loop { + match last_child.handle { + CmdChildHandle::Proc(ref mut child) => { + if let Ok(Some(status)) = child.try_wait() { + break Box::new(status); + } + } + CmdChildHandle::Thread(ref mut join_handle) => { + if let Some(handle) = join_handle.take() { + if handle.is_finished() { + break Box::new(ThreadJoinOutcome::from(handle.join())); + } else { + join_handle.replace(handle); + } + } + } + CmdChildHandle::SyncFn => { + break Box::new(SyncFnOutcome); } } let _ = stdout.read(&mut buf); @@ -181,8 +197,8 @@ impl FunChildren { if status.success() { Ok(()) } else { - Err(CmdChildHandle::status_to_io_error( - status, + Err(CmdChildHandle::outcome_to_io_error( + &*status, &last_child.cmd, &last_child.file, last_child.line, @@ -309,10 +325,70 @@ impl CmdChild { pub(crate) enum CmdChildHandle { Proc(Child), - Thread(JoinHandle), + Thread(Option>), SyncFn, } +#[derive(Debug)] +enum ThreadJoinOutcome { + Ok, + Err(std::io::Error), + Panic(Box), +} +impl From> for ThreadJoinOutcome { + fn from(result: std::thread::Result) -> Self { + match result { + Ok(Ok(())) => Self::Ok, + Ok(Err(error)) => Self::Err(error), + Err(panic) => Self::Panic(panic), + } + } +} +impl Display for ThreadJoinOutcome { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Ok => write!(f, "Command thread succeeded"), + Self::Err(error) => write!(f, "Command thread returned error: {error:?}"), + Self::Panic(panic) => write!(f, "Command thread panicked: {panic:?}"), + } + } +} +#[derive(Debug)] +struct SyncFnOutcome; +impl Display for SyncFnOutcome { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Command finished") + } +} +trait ChildOutcome: Display { + fn success(&self) -> bool; + fn code(&self) -> Option; +} +impl ChildOutcome for ExitStatus { + fn success(&self) -> bool { + self.success() + } + fn code(&self) -> Option { + self.code() + } +} +impl ChildOutcome for ThreadJoinOutcome { + fn success(&self) -> bool { + matches!(self, Self::Ok) + } + fn code(&self) -> Option { + None + } +} +impl ChildOutcome for SyncFnOutcome { + fn success(&self) -> bool { + true + } + fn code(&self) -> Option { + None + } +} + impl CmdChildHandle { fn wait(self, cmd: &str, file: &str, line: u32) -> CmdResult { match self { @@ -322,26 +398,28 @@ impl CmdChildHandle { Err(e) => return Err(process::new_cmd_io_error(&e, cmd, file, line)), Ok(status) => { if !status.success() { - return Err(Self::status_to_io_error(status, cmd, file, line)); + return Err(Self::outcome_to_io_error(&status, cmd, file, line)); } } } } - CmdChildHandle::Thread(thread) => { - let status = thread.join(); - match status { - Ok(result) => { - if let Err(e) = result { - return Err(process::new_cmd_io_error(&e, cmd, file, line)); + CmdChildHandle::Thread(mut thread) => { + if let Some(thread) = thread.take() { + let status = thread.join(); + match status { + Ok(result) => { + if let Err(e) = result { + return Err(process::new_cmd_io_error(&e, cmd, file, line)); + } } - } - Err(e) => { - return Err(Error::new( - ErrorKind::Other, - format!( + Err(e) => { + return Err(Error::new( + ErrorKind::Other, + format!( "Running [{cmd}] thread joined with error: {e:?} at {file}:{line}" ), - )) + )) + } } } } @@ -350,8 +428,8 @@ impl CmdChildHandle { Ok(()) } - fn status_to_io_error(status: ExitStatus, cmd: &str, file: &str, line: u32) -> Error { - if let Some(code) = status.code() { + fn outcome_to_io_error(outcome: &dyn ChildOutcome, cmd: &str, file: &str, line: u32) -> Error { + if let Some(code) = outcome.code() { Error::new( ErrorKind::Other, format!("Running [{cmd}] exited with error; status code: {code} at {file}:{line}"), @@ -360,7 +438,7 @@ impl CmdChildHandle { Error::new( ErrorKind::Other, format!( - "Running [{cmd}] exited with error; terminated by {status} at {file}:{line}" + "Running [{cmd}] exited with error; terminated by {outcome} at {file}:{line}" ), ) } diff --git a/src/process.rs b/src/process.rs index e89caaa..a7551e1 100644 --- a/src/process.rs +++ b/src/process.rs @@ -460,7 +460,7 @@ impl Cmd { if pipe_out || with_output { let handle = thread::Builder::new().spawn(move || internal_cmd(&mut env))?; Ok(CmdChild::new( - CmdChildHandle::Thread(handle), + CmdChildHandle::Thread(Some(handle)), full_cmds, self.file, self.line, diff --git a/tests/test_macros.rs b/tests/test_macros.rs index 96c65a3..25796f6 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -217,6 +217,17 @@ fn test_pipe() -> CmdResult { test_case!(true, true, ($macro $bang (ignore true | false)) $($after)*), test_case!(true, true, ($macro $bang (ignore false | true)) $($after)*), test_case!(true, true, ($macro $bang (ignore false | false)) $($after)*), + // Built-ins should work too, without locking up. + test_case!(true, true, ($macro $bang (echo)) $($after)*), + test_case!(true, true, ($macro $bang (echo | true)) $($after)*), + test_case!(false, false, ($macro $bang (echo | false)) $($after)*), + test_case!(true, true, ($macro $bang (true | echo)) $($after)*), + test_case!(false, true, ($macro $bang (false | echo)) $($after)*), + test_case!(true, true, ($macro $bang (cd /)) $($after)*), + test_case!(true, true, ($macro $bang (cd / | true)) $($after)*), + test_case!(false, false, ($macro $bang (cd / | false)) $($after)*), + test_case!(true, true, ($macro $bang (true | cd /)) $($after)*), + test_case!(false, true, ($macro $bang (false | cd /)) $($after)*), ] }; } From 548288f112c365dabb972dec4be2b87fc9df1cb8 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sun, 3 Aug 2025 15:29:54 +0800 Subject: [PATCH 5/6] Clean up new logic and merge with CmdChildHandle::wait() --- src/child.rs | 121 +++++++++++++++++++++------------------------------ 1 file changed, 49 insertions(+), 72 deletions(-) diff --git a/src/child.rs b/src/child.rs index 3b97bfc..c3a8a6c 100644 --- a/src/child.rs +++ b/src/child.rs @@ -171,12 +171,14 @@ impl FunChildren { let last_child_res = if let Some(stdout) = last_child.stdout { let mut stdout: Box = Box::new(stdout); f(&mut stdout); + // The provided function may have left some of stdout unread. + // Continue reading stdout on its behalf, until the child exits. let mut buf = vec![0; 65536]; - let status: Box = loop { + let outcome: Box = loop { match last_child.handle { CmdChildHandle::Proc(ref mut child) => { - if let Ok(Some(status)) = child.try_wait() { - break Box::new(status); + if let Some(result) = child.try_wait().transpose() { + break Box::new(ProcWaitOutcome::from(result)); } } CmdChildHandle::Thread(ref mut join_handle) => { @@ -194,16 +196,7 @@ impl FunChildren { } let _ = stdout.read(&mut buf); }; - if status.success() { - Ok(()) - } else { - Err(CmdChildHandle::outcome_to_io_error( - &*status, - &last_child.cmd, - &last_child.file, - last_child.line, - )) - } + outcome.to_io_result(&last_child.cmd, &last_child.file, last_child.line) } else { last_child.wait(true) }; @@ -329,6 +322,29 @@ pub(crate) enum CmdChildHandle { SyncFn, } +#[derive(Debug)] +struct ProcWaitOutcome(std::io::Result); +impl From> for ProcWaitOutcome { + fn from(result: std::io::Result) -> Self { + Self(result) + } +} +impl Display for ProcWaitOutcome { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self.0 { + Ok(status) => { + if status.success() { + write!(f, "Command process succeeded") + } else if let Some(code) = status.code() { + write!(f, "Command process exited normally with status code {code}") + } else { + write!(f, "Command process exited abnormally: {status}") + } + } + Err(error) => write!(f, "Failed to wait for command process: {error:?}"), + } + } +} #[derive(Debug)] enum ThreadJoinOutcome { Ok, @@ -362,86 +378,47 @@ impl Display for SyncFnOutcome { } trait ChildOutcome: Display { fn success(&self) -> bool; - fn code(&self) -> Option; + fn to_io_result(&self, cmd: &str, file: &str, line: u32) -> std::io::Result<()> { + if self.success() { + Ok(()) + } else { + Err(Error::new( + ErrorKind::Other, + format!("Running [{cmd}] exited with error; {self} at {file}:{line}"), + )) + } + } } -impl ChildOutcome for ExitStatus { +impl ChildOutcome for ProcWaitOutcome { fn success(&self) -> bool { - self.success() - } - fn code(&self) -> Option { - self.code() + self.0.as_ref().is_ok_and(|status| status.success()) } } impl ChildOutcome for ThreadJoinOutcome { fn success(&self) -> bool { matches!(self, Self::Ok) } - fn code(&self) -> Option { - None - } } impl ChildOutcome for SyncFnOutcome { fn success(&self) -> bool { true } - fn code(&self) -> Option { - None - } } impl CmdChildHandle { fn wait(self, cmd: &str, file: &str, line: u32) -> CmdResult { - match self { - CmdChildHandle::Proc(mut proc) => { - let status = proc.wait(); - match status { - Err(e) => return Err(process::new_cmd_io_error(&e, cmd, file, line)), - Ok(status) => { - if !status.success() { - return Err(Self::outcome_to_io_error(&status, cmd, file, line)); - } - } - } - } + let outcome: Box = match self { + CmdChildHandle::Proc(mut proc) => Box::new(ProcWaitOutcome::from(proc.wait())), CmdChildHandle::Thread(mut thread) => { if let Some(thread) = thread.take() { - let status = thread.join(); - match status { - Ok(result) => { - if let Err(e) = result { - return Err(process::new_cmd_io_error(&e, cmd, file, line)); - } - } - Err(e) => { - return Err(Error::new( - ErrorKind::Other, - format!( - "Running [{cmd}] thread joined with error: {e:?} at {file}:{line}" - ), - )) - } - } + Box::new(ThreadJoinOutcome::from(thread.join())) + } else { + unreachable!() } } - CmdChildHandle::SyncFn => {} - } - Ok(()) - } - - fn outcome_to_io_error(outcome: &dyn ChildOutcome, cmd: &str, file: &str, line: u32) -> Error { - if let Some(code) = outcome.code() { - Error::new( - ErrorKind::Other, - format!("Running [{cmd}] exited with error; status code: {code} at {file}:{line}"), - ) - } else { - Error::new( - ErrorKind::Other, - format!( - "Running [{cmd}] exited with error; terminated by {outcome} at {file}:{line}" - ), - ) - } + CmdChildHandle::SyncFn => return Ok(()), + }; + outcome.to_io_result(cmd, file, line) } fn kill(self, cmd: &str, file: &str, line: u32) -> CmdResult { From a0bb1230acdef3bf66f43c157f407aaa54863e4e Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sun, 3 Aug 2025 17:58:42 +0800 Subject: [PATCH 6/6] Replace wait_with_pipe() with wait_with_borrowed_pipe() --- src/child.rs | 39 +--------------------------- tests/test_macros.rs | 62 ++------------------------------------------ 2 files changed, 3 insertions(+), 98 deletions(-) diff --git a/src/child.rs b/src/child.rs index c3a8a6c..24e5687 100644 --- a/src/child.rs +++ b/src/child.rs @@ -117,49 +117,12 @@ impl FunChildren { } } - /// Pipes stdout from the last child in the pipeline to the given function, which runs in - /// the current thread, then waits for all of the children to exit. - /// - ///
- /// - /// # Bugs - /// - /// The exit status of the last child is **ignored**. If the function returns early, without - /// reading from stdout until the last child exits, then the last child may be killed instead - /// of being waited for. To avoid these limitations, use [`Self::wait_with_stdout_thread`]. - ///
- pub fn wait_with_pipe(&mut self, f: &mut dyn FnMut(Box)) -> CmdResult { - let child = self.children.pop().unwrap(); - let stderr_thread = - StderrThread::new(&child.cmd, &child.file, child.line, child.stderr, false); - match child.handle { - CmdChildHandle::Proc(mut proc) => { - if let Some(stdout) = child.stdout { - f(Box::new(stdout)); - let _ = proc.kill(); - } - } - CmdChildHandle::Thread(_) => { - if let Some(stdout) = child.stdout { - f(Box::new(stdout)); - } - } - CmdChildHandle::SyncFn => { - if let Some(stdout) = child.stdout { - f(Box::new(stdout)); - } - } - }; - drop(stderr_thread); - CmdChildren::wait_children(&mut self.children) - } - /// Pipes stdout from the last child in the pipeline to the given function, which runs in /// the current thread, then waits for all of the children to exit. /// /// If the function returns early, without reading from stdout until the last child exits, /// then the rest of stdout is automatically read and discarded to allow the child to finish. - pub fn wait_with_borrowed_pipe(&mut self, f: &mut dyn FnMut(&mut Box)) -> CmdResult { + pub fn wait_with_pipe(&mut self, f: &mut dyn FnMut(&mut Box)) -> CmdResult { let mut last_child = self.children.pop().unwrap(); let mut stderr_thread = StderrThread::new( &last_child.cmd, diff --git a/tests/test_macros.rs b/tests/test_macros.rs index 25796f6..1ce95e6 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -134,7 +134,7 @@ fn test_tls_set() { } #[test] -fn test_pipe() -> CmdResult { +fn test_pipe() { assert!(run_cmd!(echo "xx").is_ok()); assert_eq!(run_fun!(echo "xx").unwrap(), "xx"); assert!(run_cmd!(echo xx | wc).is_ok()); @@ -246,7 +246,7 @@ fn test_pipe() -> CmdResult { .wait_with_raw_output(&mut vec![])), test_cases_for_entry_point!((spawn_with_output!(...)) .unwrap() - .wait_with_borrowed_pipe(&mut |_stdout| {})), + .wait_with_pipe(&mut |_stdout| {})), ]; macro_rules! check_eq { @@ -281,64 +281,6 @@ fn test_pipe() -> CmdResult { } assert!(ok); - - // test that illustrates the bugs in wait_with_pipe() - // FIXME: make set_pipefail() thread safe, then move this to a separate test function - assert!(spawn_with_output!(false)?.wait_with_all().0.is_err()); - assert!(spawn_with_output!(false)?.wait_with_output().is_err()); - assert!(spawn_with_output!(false)? - .wait_with_raw_output(&mut vec![]) - .is_err()); - - // wait_with_pipe() can’t check the exit status of the last child - assert!(spawn_with_output!(false)? - .wait_with_pipe(&mut |_stdout| {}) - .is_ok()); - - // wait_with_pipe() kills the last child when the provided function returns - assert!(spawn_with_output!(sh -c "while :; do :; done")? - .wait_with_pipe(&mut |_stdout| {}) - .is_ok()); - - // wait_with_borrowed_pipe() checks the exit status of the last child, even if pipefail is disabled - set_pipefail(false); - assert!(spawn_with_output!(true | false)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_err()); - assert!(spawn_with_output!(true | true)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_ok()); - assert!(spawn_with_output!(false)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_err()); - assert!(spawn_with_output!(true)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_ok()); - set_pipefail(true); - // wait_with_borrowed_pipe() checks the exit status of the other children, unless pipefail is disabled - set_pipefail(false); - assert!(spawn_with_output!(false | true)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_ok()); - set_pipefail(true); - assert!(spawn_with_output!(false | true)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_err()); - assert!(spawn_with_output!(true | true)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_ok()); - // wait_with_borrowed_pipe() handles `ignore` - assert!(spawn_with_output!(ignore false | true)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_ok()); - assert!(spawn_with_output!(ignore true | false)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_ok()); - assert!(spawn_with_output!(ignore false)? - .wait_with_borrowed_pipe(&mut |_stdout| {}) - .is_ok()); - - Ok(()) } #[test]