From 098527b263a54092982f99f19ce2cce257e13fa0 Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Sat, 24 Feb 2024 07:32:39 -0800 Subject: [PATCH] Print stderr streams to stderr in `pipeline_data::print_if_stream()` (#11929) Related to #11928 - `tee --stderr` doesn't really work as expected without it # Description Print stderr streams to stderr in `pipeline_data::print_if_stream()` This corrects unexpected behavior if a stream from an external program is transformed while still preserving its stderr output. Before this change, that output is just drained and discarded. Worse, it's drained to a buffer, which could be really slow and memory hungry if there's a lot of output on stderr. This is needed to make `tee --stderr` function in a non-surprising way. See #11928 # User-Facing Changes A script that was erroneously not producing stderr output before might now, but I can't think of a lot of examples of an external stream being transformed without being converted. # Tests + Formatting - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :green_circle: `toolkit test` - :green_circle: `toolkit test stdlib` # After Submitting --- crates/nu-protocol/src/pipeline_data.rs | 26 ++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs index 72bf48be64..21126e709a 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline_data.rs @@ -4,9 +4,9 @@ use crate::{ format_error, Config, ListStream, RawStream, ShellError, Span, Value, }; use nu_utils::{stderr_write_all_and_flush, stdout_write_all_and_flush}; -use std::path::PathBuf; use std::sync::{atomic::AtomicBool, Arc}; use std::thread; +use std::{io::Write, path::PathBuf}; const LINE_ENDING_PATTERN: &[char] = &['\r', '\n']; @@ -846,13 +846,29 @@ pub fn print_if_stream( to_stderr: bool, exit_code: Option, ) -> Result { - // NOTE: currently we don't need anything from stderr - // so we just consume and throw away `stderr_stream` to make sure the pipe doesn't fill up - if let Some(stderr_stream) = stderr_stream { + // Write stderr to our stderr, if it's present thread::Builder::new() .name("stderr consumer".to_string()) - .spawn(move || stderr_stream.into_bytes()) + .spawn(move || { + let RawStream { + stream, + leftover, + ctrlc, + .. + } = stderr_stream; + let mut stderr = std::io::stderr(); + let _ = stderr.write_all(&leftover); + drop(leftover); + for bytes in stream { + if nu_utils::ctrl_c::was_pressed(&ctrlc) { + break; + } + if let Ok(bytes) = bytes { + let _ = stderr.write_all(&bytes); + } + } + }) .expect("could not create thread"); }