From 3c0a3835ff2ce5cfbe7a3f156b929b1996f7ea4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Hahn?= Date: Mon, 9 Aug 2021 21:15:43 -0400 Subject: [PATCH 1/6] [BROKEN] Add new test for stderr --- Cargo.toml | 2 +- memory-test/src/bin/cradle_user.rs | 8 ------- memory-test/src/bin/produce_bytes.rs | 17 -------------- {memory-test => memory-tests}/Cargo.lock | 0 {memory-test => memory-tests}/Cargo.toml | 2 +- memory-tests/src/bin/cradle_user.rs | 13 +++++++++++ memory-tests/src/bin/produce_bytes.rs | 22 +++++++++++++++++++ .../src/bin/run.rs | 18 ++++++++++----- tests/integration.rs | 4 ++-- 9 files changed, 51 insertions(+), 35 deletions(-) delete mode 100644 memory-test/src/bin/cradle_user.rs delete mode 100644 memory-test/src/bin/produce_bytes.rs rename {memory-test => memory-tests}/Cargo.lock (100%) rename {memory-test => memory-tests}/Cargo.toml (85%) create mode 100644 memory-tests/src/bin/cradle_user.rs create mode 100644 memory-tests/src/bin/produce_bytes.rs rename memory-test/src/bin/run_test.rs => memory-tests/src/bin/run.rs (76%) diff --git a/Cargo.toml b/Cargo.toml index 9062c34b..74862b42 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ keywords = ["child", "child-process", "command", "process", "shell"] categories = ["filesystem", "os"] [workspace] -members = [".", "memory-test"] +members = [".", "memory-tests"] [dependencies] rustversion = "1.0.4" diff --git a/memory-test/src/bin/cradle_user.rs b/memory-test/src/bin/cradle_user.rs deleted file mode 100644 index 33259980..00000000 --- a/memory-test/src/bin/cradle_user.rs +++ /dev/null @@ -1,8 +0,0 @@ -use cradle::prelude::*; - -fn main() { - let mut args = std::env::args(); - let bytes: usize = args.nth(1).unwrap().parse().unwrap(); - eprintln!("consuming {} KiB", bytes / 2_usize.pow(10)); - cmd_unit!("./target/release/produce_bytes", bytes.to_string()); -} diff --git a/memory-test/src/bin/produce_bytes.rs b/memory-test/src/bin/produce_bytes.rs deleted file mode 100644 index 60c9ad6f..00000000 --- a/memory-test/src/bin/produce_bytes.rs +++ /dev/null @@ -1,17 +0,0 @@ -use anyhow::Result; -use std::io::{stdout, Write}; - -fn main() -> Result<()> { - let mut args = std::env::args(); - let mut bytes: usize = args.nth(1).unwrap().parse()?; - eprintln!("producing {} KiB", bytes / 2_usize.pow(10)); - let buffer = &[b'x'; 1024]; - let mut stdout = stdout(); - while bytes > 0 { - let chunk_size = bytes.min(1024); - stdout.write_all(&buffer[..chunk_size])?; - bytes -= chunk_size; - } - stdout.flush()?; - Ok(()) -} diff --git a/memory-test/Cargo.lock b/memory-tests/Cargo.lock similarity index 100% rename from memory-test/Cargo.lock rename to memory-tests/Cargo.lock diff --git a/memory-test/Cargo.toml b/memory-tests/Cargo.toml similarity index 85% rename from memory-test/Cargo.toml rename to memory-tests/Cargo.toml index 7b10f4ac..c2d60e6e 100644 --- a/memory-test/Cargo.toml +++ b/memory-tests/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "memory-test" +name = "memory-tests" version = "0.0.0" edition = "2018" diff --git a/memory-tests/src/bin/cradle_user.rs b/memory-tests/src/bin/cradle_user.rs new file mode 100644 index 00000000..28a567e0 --- /dev/null +++ b/memory-tests/src/bin/cradle_user.rs @@ -0,0 +1,13 @@ +use cradle::prelude::*; + +fn main() { + let mut args = std::env::args(); + let stream_type: String = args.nth(1).unwrap(); + let bytes: usize = args.next().unwrap().parse().unwrap(); + eprintln!("consuming {} KiB", bytes / 2_usize.pow(10)); + cmd_unit!( + "./target/release/produce_bytes", + stream_type, + bytes.to_string() + ); +} diff --git a/memory-tests/src/bin/produce_bytes.rs b/memory-tests/src/bin/produce_bytes.rs new file mode 100644 index 00000000..36c62c7d --- /dev/null +++ b/memory-tests/src/bin/produce_bytes.rs @@ -0,0 +1,22 @@ +use anyhow::Result; +use std::io::{self, Write}; + +fn main() -> Result<()> { + let mut args = std::env::args(); + let stream_type: String = args.nth(1).unwrap(); + let mut bytes: usize = args.next().unwrap().parse()?; + eprintln!("writing {} KiB to {}", bytes / 2_usize.pow(10), stream_type); + let buffer = &[b'x'; 1024]; + let mut stream: Box = match stream_type.as_str() { + "stdout" => Box::new(io::stdout()), + "stderr" => Box::new(io::stderr()), + _ => panic!("unknown stream type: {}", stream_type), + }; + while bytes > 0 { + let chunk_size = bytes.min(1024); + stream.write_all(&buffer[..chunk_size])?; + bytes -= chunk_size; + } + stream.flush()?; + Ok(()) +} diff --git a/memory-test/src/bin/run_test.rs b/memory-tests/src/bin/run.rs similarity index 76% rename from memory-test/src/bin/run_test.rs rename to memory-tests/src/bin/run.rs index cc7a51a2..34a9be3b 100644 --- a/memory-test/src/bin/run_test.rs +++ b/memory-tests/src/bin/run.rs @@ -1,5 +1,4 @@ use anyhow::Result; -use cradle::prelude::*; use std::process::{Command, Stdio}; fn from_mib(mebibytes: usize) -> usize { @@ -7,29 +6,36 @@ fn from_mib(mebibytes: usize) -> usize { } fn main() -> Result<()> { - Split("cargo build --release").run_unit(); + test("stdout")?; + test("stderr")?; + Ok(()) +} + +fn test(stream_type: &str) -> Result<()> { let bytes = from_mib(64); - let memory_consumption = measure_memory_consumption(bytes)?; + let memory_consumption = measure_memory_consumption(stream_type, bytes)?; let allowed_memory_consumption = from_mib(16); assert!( memory_consumption < allowed_memory_consumption, - "Maximum resident set size: {}, allowed upper limit: {}", + "stream type: {}, Maximum resident set size: {}, allowed upper limit: {}", + stream_type, memory_consumption, allowed_memory_consumption ); Ok(()) } -fn measure_memory_consumption(bytes: usize) -> Result { +fn measure_memory_consumption(stream_type: &str, bytes: usize) -> Result { let output = Command::new("/usr/bin/time") .arg("-v") .arg("./target/release/cradle_user") + .arg(stream_type) .arg(bytes.to_string()) .stdout(Stdio::null()) .output()?; let stderr = String::from_utf8(output.stderr)?; - eprintln!("{}", stderr); if !output.status.success() { + eprintln!("{}", stderr); panic!("running 'cradle_user' failed"); } let memory_size_prefix = "Maximum resident set size (kbytes): "; diff --git a/tests/integration.rs b/tests/integration.rs index 6ac20f33..3ae8f424 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -318,6 +318,6 @@ mod run_interface { #[test] fn memory_test() { use cradle::prelude::*; - cmd_unit!(%"cargo build -p memory-test --release"); - cmd_unit!(%"cargo run -p memory-test --bin run_test"); + cmd_unit!(%"cargo build -p memory-tests --release"); + cmd_unit!(%"cargo run -p memory-tests --bin run"); } From c2c2eb12d5fe73701f8c5adec5cede99963ec223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Hahn?= Date: Mon, 9 Aug 2021 21:38:34 -0400 Subject: [PATCH 2/6] Don't read stderr into memory when not captured --- src/collected_output.rs | 16 +++++++++++----- src/lib.rs | 2 +- src/output.rs | 12 ++++++------ 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/collected_output.rs b/src/collected_output.rs index 0a8cf876..f52e04d8 100644 --- a/src/collected_output.rs +++ b/src/collected_output.rs @@ -9,7 +9,7 @@ use std::{ pub(crate) struct Waiter { stdin: JoinHandle>, stdout: JoinHandle>>>, - stderr: JoinHandle>>, + stderr: JoinHandle>>>, } impl Waiter { @@ -54,15 +54,21 @@ impl Waiter { }); let mut context_clone = context.clone(); let capture_stderr = config.capture_stderr; - let stderr_join_handle = thread::spawn(move || -> io::Result> { - let mut collected_stderr = Vec::new(); + let stderr_join_handle = thread::spawn(move || -> io::Result>> { + let mut collected_stderr = if capture_stderr { + Some(Vec::new()) + } else { + None + }; let buffer = &mut [0; 256]; loop { let length = child_stderr.read(buffer)?; if (length) == 0 { break; } - collected_stderr.extend(&buffer[..length]); + if let Some(collected_stderr) = &mut collected_stderr { + collected_stderr.extend(&buffer[..length]); + } if !capture_stderr { context_clone.stderr.write_all(&buffer[..length])?; } @@ -96,5 +102,5 @@ impl Waiter { #[derive(Debug)] pub(crate) struct CollectedOutput { pub(crate) stdout: Option>, - pub(crate) stderr: Vec, + pub(crate) stderr: Option>, } diff --git a/src/lib.rs b/src/lib.rs index d2728d80..036d1693 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -323,7 +323,7 @@ where #[derive(Clone, Debug)] pub struct RunResult { stdout: Option>, - stderr: Vec, + stderr: Option>, exit_status: ExitStatus, } diff --git a/src/output.rs b/src/output.rs index 3ceb0e18..36e55827 100644 --- a/src/output.rs +++ b/src/output.rs @@ -170,8 +170,7 @@ impl Output for StdoutUntrimmed { #[doc(hidden)] fn from_run_result(config: &Config, result: Result) -> Result { - let result = result?; - let stdout = result.stdout.ok_or_else(|| Error::internal(config))?; + let stdout = result?.stdout.ok_or_else(|| Error::internal(config))?; Ok(StdoutUntrimmed(String::from_utf8(stdout).map_err( |source| Error::InvalidUtf8ToStdout { full_command: config.full_command(), @@ -209,12 +208,13 @@ impl Output for Stderr { #[doc(hidden)] fn from_run_result(config: &Config, result: Result) -> Result { - Ok(Stderr(String::from_utf8(result?.stderr).map_err( - |source| Error::InvalidUtf8ToStderr { + let stderr = result?.stderr.ok_or_else(|| Error::internal(config))?; + Ok(Stderr(String::from_utf8(stderr).map_err(|source| { + Error::InvalidUtf8ToStderr { full_command: config.full_command(), source: Arc::new(source), - }, - )?)) + } + })?)) } } From 65559c05a768fbf392a86febb942ec41ba2b39d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Hahn?= Date: Mon, 9 Aug 2021 21:44:33 -0400 Subject: [PATCH 3/6] Add error message to Error::Internal --- src/error.rs | 4 +++- src/output.rs | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/error.rs b/src/error.rs index a3edb442..daac9e67 100644 --- a/src/error.rs +++ b/src/error.rs @@ -27,6 +27,7 @@ pub enum Error { source: Arc, }, Internal { + message: String, full_command: String, config: Config, }, @@ -40,8 +41,9 @@ impl Error { } } - pub(crate) fn internal(config: &Config) -> Error { + pub(crate) fn internal(message: &str, config: &Config) -> Error { Error::Internal { + message: message.to_string(), full_command: config.full_command(), config: config.clone(), } diff --git a/src/output.rs b/src/output.rs index 36e55827..ac977c65 100644 --- a/src/output.rs +++ b/src/output.rs @@ -170,7 +170,9 @@ impl Output for StdoutUntrimmed { #[doc(hidden)] fn from_run_result(config: &Config, result: Result) -> Result { - let stdout = result?.stdout.ok_or_else(|| Error::internal(config))?; + let stdout = result? + .stdout + .ok_or_else(|| Error::internal("stdout not captured", config))?; Ok(StdoutUntrimmed(String::from_utf8(stdout).map_err( |source| Error::InvalidUtf8ToStdout { full_command: config.full_command(), @@ -208,7 +210,9 @@ impl Output for Stderr { #[doc(hidden)] fn from_run_result(config: &Config, result: Result) -> Result { - let stderr = result?.stderr.ok_or_else(|| Error::internal(config))?; + let stderr = result? + .stderr + .ok_or_else(|| Error::internal("stderr not captured", config))?; Ok(Stderr(String::from_utf8(stderr).map_err(|source| { Error::InvalidUtf8ToStderr { full_command: config.full_command(), From 4015e1cf32d8d09fad82facf9e82ed97a3328a76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Hahn?= Date: Mon, 9 Aug 2021 22:40:42 -0400 Subject: [PATCH 4/6] Dry up output stream handling --- src/collected_output.rs | 88 +++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 48 deletions(-) diff --git a/src/collected_output.rs b/src/collected_output.rs index f52e04d8..b83d8dad 100644 --- a/src/collected_output.rs +++ b/src/collected_output.rs @@ -13,68 +13,60 @@ pub(crate) struct Waiter { } impl Waiter { - pub(crate) fn spawn_standard_stream_relaying( - context: &Context, - config: &Config, - mut child_stdin: ChildStdin, - mut child_stdout: ChildStdout, - mut child_stderr: ChildStderr, - ) -> Self - where - Stdout: Write + Send + Clone + 'static, - Stderr: Write + Send + Clone + 'static, - { - let config_stdin = config.stdin.clone(); - let stdin_join_handle = thread::spawn(move || -> io::Result<()> { - child_stdin.write_all(&config_stdin)?; - Ok(()) - }); - let mut context_clone = context.clone(); - let capture_stdout = config.capture_stdout; - let stdout_join_handle = thread::spawn(move || -> io::Result>> { - let mut collected_stdout = if capture_stdout { + fn spawn_standard_stream_handler( + capture_stream: bool, + mut source: impl Read + Send + 'static, + mut relay_sink: impl Write + Send + 'static, + ) -> JoinHandle>>> { + thread::spawn(move || -> io::Result>> { + let mut collected = if capture_stream { Some(Vec::new()) } else { None }; let buffer = &mut [0; 256]; loop { - let length = child_stdout.read(buffer)?; + let length = source.read(buffer)?; if (length) == 0 { break; } - if let Some(collected_stdout) = &mut collected_stdout { + if let Some(collected_stdout) = &mut collected { collected_stdout.extend(&buffer[..length]); } - if !capture_stdout { - context_clone.stdout.write_all(&buffer[..length])?; + if !capture_stream { + relay_sink.write_all(&buffer[..length])?; } } - Ok(collected_stdout) - }); - let mut context_clone = context.clone(); - let capture_stderr = config.capture_stderr; - let stderr_join_handle = thread::spawn(move || -> io::Result>> { - let mut collected_stderr = if capture_stderr { - Some(Vec::new()) - } else { - None - }; - let buffer = &mut [0; 256]; - loop { - let length = child_stderr.read(buffer)?; - if (length) == 0 { - break; - } - if let Some(collected_stderr) = &mut collected_stderr { - collected_stderr.extend(&buffer[..length]); - } - if !capture_stderr { - context_clone.stderr.write_all(&buffer[..length])?; - } - } - Ok(collected_stderr) + Ok(collected) + }) + } + + pub(crate) fn spawn_standard_stream_relaying( + context: &Context, + config: &Config, + mut child_stdin: ChildStdin, + child_stdout: ChildStdout, + child_stderr: ChildStderr, + ) -> Self + where + Stdout: Write + Send + Clone + 'static, + Stderr: Write + Send + Clone + 'static, + { + let config_stdin = config.stdin.clone(); + let stdin_join_handle = thread::spawn(move || -> io::Result<()> { + child_stdin.write_all(&config_stdin)?; + Ok(()) }); + let stdout_join_handle = Self::spawn_standard_stream_handler( + config.capture_stdout, + child_stdout, + context.stdout.clone(), + ); + let stderr_join_handle = Self::spawn_standard_stream_handler( + config.capture_stderr, + child_stderr, + context.stderr.clone(), + ); Waiter { stdin: stdin_join_handle, stdout: stdout_join_handle, From eb8cfa913fdb525993fd7aab64cb2fc73ce86c4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Hahn?= Date: Mon, 9 Aug 2021 22:45:15 -0400 Subject: [PATCH 5/6] Fix typo --- memory-tests/src/bin/run.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/memory-tests/src/bin/run.rs b/memory-tests/src/bin/run.rs index 34a9be3b..3cb60057 100644 --- a/memory-tests/src/bin/run.rs +++ b/memory-tests/src/bin/run.rs @@ -17,7 +17,7 @@ fn test(stream_type: &str) -> Result<()> { let allowed_memory_consumption = from_mib(16); assert!( memory_consumption < allowed_memory_consumption, - "stream type: {}, Maximum resident set size: {}, allowed upper limit: {}", + "stream type: {}, maximum resident set size: {}, allowed upper limit: {}", stream_type, memory_consumption, allowed_memory_consumption From 35209166d17dd4af0f85cb0fa47845d2294f53e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Hahn?= Date: Mon, 9 Aug 2021 22:47:09 -0400 Subject: [PATCH 6/6] Another typo --- src/collected_output.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/collected_output.rs b/src/collected_output.rs index b83d8dad..ec795df4 100644 --- a/src/collected_output.rs +++ b/src/collected_output.rs @@ -30,8 +30,8 @@ impl Waiter { if (length) == 0 { break; } - if let Some(collected_stdout) = &mut collected { - collected_stdout.extend(&buffer[..length]); + if let Some(collected) = &mut collected { + collected.extend(&buffer[..length]); } if !capture_stream { relay_sink.write_all(&buffer[..length])?;