diff --git a/crates/nu-command/src/system/complete.rs b/crates/nu-command/src/system/complete.rs index 8e6d142d5..ffbfe0414 100644 --- a/crates/nu-command/src/system/complete.rs +++ b/crates/nu-command/src/system/complete.rs @@ -37,22 +37,9 @@ impl Command for Complete { let mut cols = vec![]; let mut vals = vec![]; - if let Some(stdout) = stdout { - cols.push("stdout".to_string()); - let stdout = stdout.into_bytes()?; - if let Ok(st) = String::from_utf8(stdout.item.clone()) { - vals.push(Value::String { - val: st, - span: stdout.span, - }) - } else { - vals.push(Value::Binary { - val: stdout.item, - span: stdout.span, - }) - } - } - + // the order is important, we need to read + // stderr, then stdout, then exit_code + // because run_external generate them in this order. if let Some(stderr) = stderr { cols.push("stderr".to_string()); let stderr = stderr.into_bytes()?; @@ -69,6 +56,22 @@ impl Command for Complete { }; } + if let Some(stdout) = stdout { + cols.push("stdout".to_string()); + let stdout = stdout.into_bytes()?; + if let Ok(st) = String::from_utf8(stdout.item.clone()) { + vals.push(Value::String { + val: st, + span: stdout.span, + }) + } else { + vals.push(Value::Binary { + val: stdout.item, + span: stdout.span, + }) + } + } + if let Some(exit_code) = exit_code { let mut v: Vec<_> = exit_code.collect(); diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 6e97823f8..8c4f3df3b 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -337,6 +337,13 @@ impl ExternalCommand { let mut buf_read = BufReader::with_capacity(OUTPUT_BUFFER_SIZE, stderr); while let Ok(bytes) = buf_read.fill_buf() { if bytes.is_empty() { + // drop stderr sender manually, so stderr message consumer + // can make sure that there is no more stderr messages. + // + // and message consumer can continue to wait stdout message. + // If we don't make manually drop, and external command produces many + // stdout messages, relative message consumer will hang on so we'll get a deadlock. + drop(stderr_tx); break; } @@ -438,11 +445,15 @@ impl ExternalCommand { } else { None }, - stderr: Some(RawStream::new( - Box::new(stderr_receiver), - output_ctrlc.clone(), - head, - )), + stderr: if redirect_stderr { + Some(RawStream::new( + Box::new(stderr_receiver), + output_ctrlc.clone(), + head, + )) + } else { + None + }, exit_code: Some(ListStream::from_stream( Box::new(exit_code_receiver), output_ctrlc, diff --git a/crates/nu-command/tests/commands/do_.rs b/crates/nu-command/tests/commands/do_.rs index eb7f7ad15..86be3ab4a 100644 --- a/crates/nu-command/tests/commands/do_.rs +++ b/crates/nu-command/tests/commands/do_.rs @@ -59,3 +59,68 @@ fn do_with_semicolon_break_on_failed_external() { assert_eq!(actual.out, ""); } + +#[test] +#[cfg(not(windows))] +fn ignore_error_not_hang_nushell() { + use nu_test_support::fs::Stub::FileWithContent; + use nu_test_support::pipeline; + use nu_test_support::playground::Playground; + Playground::setup("external with many stderr message", |dirs, sandbox| { + let bytes: usize = 81920; + let mut large_file_body = String::with_capacity(bytes); + for _ in 0..bytes { + large_file_body.push_str("a"); + } + sandbox.with_files(vec![FileWithContent("a_large_file.txt", &large_file_body)]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + do -i {sh -c "cat a_large_file.txt 1>&2"} | complete | get stderr + "# + )); + + assert_eq!(actual.out, large_file_body); + }) +} + +#[test] +#[cfg(not(windows))] +fn ignore_error_with_both_stdout_stderr_messages_not_hang_nushell() { + use nu_test_support::fs::Stub::FileWithContent; + use nu_test_support::playground::Playground; + Playground::setup( + "external with many stdout and stderr messages", + |dirs, sandbox| { + let script_body = r#" + x=$(printf '=%.0s' {1..40960}) + echo $x + echo $x 1>&2 + "#; + let mut expect_body = String::new(); + for _ in 0..40960 { + expect_body.push_str("="); + } + + sandbox.with_files(vec![FileWithContent("test.sh", &script_body)]); + + // check for stdout + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + do -i {bash test.sh} | complete | get stdout | str trim + "# + )); + assert_eq!(actual.out, expect_body); + // check for stderr + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + do -i {bash test.sh} | complete | get stderr | str trim + "# + )); + assert_eq!(actual.out, expect_body); + }, + ) +} diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 2c9350fab..7ff14655b 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -4,7 +4,7 @@ use nu_protocol::{ ast::{Block, Call, Expr, Expression, Operator}, engine::{EngineState, Stack, Visibility}, Config, HistoryFileFormat, IntoInterruptiblePipelineData, IntoPipelineData, ListStream, - PipelineData, Range, ShellError, Span, Spanned, SyntaxShape, Unit, Value, VarId, + PipelineData, Range, RawStream, ShellError, Span, Spanned, SyntaxShape, Unit, Value, VarId, ENV_VARIABLE_ID, }; use nu_utils::stdout_write_all_and_flush; @@ -699,6 +699,31 @@ fn might_consume_external_result(input: PipelineData) -> (PipelineData, bool) { } = input { let exit_code = exit_code.take(); + + // Note: + // In run-external's implementation detail, the result sender thread + // send out stderr message first, then stdout message, then exit_code. + // + // In this clause, we already make sure that `stdout` is None + // But not the case of `stderr`, so if `stderr` is not None + // We need to consume stderr message before reading external commands' exit code. + // + // Or we'll never have a chance to read exit_code if stderr producer produce too much stderr message. + // So we consume stderr stream and rebuild it. + let stderr = stderr.map(|stderr_stream| { + let stderr_ctrlc = stderr_stream.ctrlc.clone(); + let stderr_span = stderr_stream.span; + let stderr_bytes = match stderr_stream.into_bytes() { + Err(_) => vec![], + Ok(bytes) => bytes.item, + }; + RawStream::new( + Box::new(vec![Ok(stderr_bytes)].into_iter()), + stderr_ctrlc, + stderr_span, + ) + }); + match exit_code { Some(exit_code_stream) => { let ctrlc = exit_code_stream.ctrlc.clone(); diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs index e1b91196b..b9ea88433 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline_data.rs @@ -433,10 +433,14 @@ impl PipelineData { if let PipelineData::ExternalStream { stdout: stream, + stderr: stderr_stream, exit_code, .. } = self { + // NOTE: currently we don't need anything from stderr + // so directly consumes `stderr_stream` to make sure that everything is done. + let _ = stderr_stream.map(|x| x.into_bytes()); if let Some(stream) = stream { for s in stream { let s_live = s?;