diff --git a/crates/nu-command/tests/commands/run_external.rs b/crates/nu-command/tests/commands/run_external.rs index 151834c54..e93b8dc51 100644 --- a/crates/nu-command/tests/commands/run_external.rs +++ b/crates/nu-command/tests/commands/run_external.rs @@ -139,6 +139,17 @@ fn failed_command_with_semicolon_will_not_execute_following_cmds() { }) } +#[test] +fn semicolon_works_within_subcommand() { + Playground::setup("external failed command with semicolon", |dirs, _| { + let actual = nu!( + cwd: dirs.test(), pipeline(r#"(nu --testbin outcome_err "aa"; echo done)"# + )); + + assert!(!actual.out.contains("done")); + }) +} + #[cfg(not(windows))] #[test] fn external_args_with_quoted() { diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 10e0bbb68..f0305f511 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -699,7 +699,14 @@ pub fn eval_expression_with_input( } }; - Ok(might_consume_external_result(input)) + // Note: for `table` command, it mights returns `ExternalStream with stdout` + // whatever `redirect_output` is true or false, so we only want to consume ExternalStream + // if relative stdout is None. + if let PipelineData::ExternalStream { stdout: None, .. } = input { + Ok(might_consume_external_result(input)) + } else { + Ok((input, false)) + } } // Try to catch and detect if external command runs to failed. @@ -1197,14 +1204,72 @@ pub fn eval_subexpression( mut input: PipelineData, ) -> Result { for pipeline in block.pipelines.iter() { - for expr in pipeline.elements.iter() { - input = eval_element_with_input(engine_state, stack, expr, input, true, false)?.0 + for (expr_indx, expr) in pipeline.elements.iter().enumerate() { + if expr_indx != pipeline.elements.len() - 1 { + input = eval_element_with_input(engine_state, stack, expr, input, true, false)?.0; + } else { + // In subexpression, we always need to redirect stdout because the result is substituted to a Value. + // + // But we can check if external result is failed to run when it's the last expression + // in pipeline. e.g: (^false; echo aaa) + // + // If external command is failed to run, it can't be convert into value, in this case + // we throws out `ShellError::ExternalCommand`. And show it's stderr message information. + // In the case, we need to capture stderr first during eval. + input = eval_element_with_input(engine_state, stack, expr, input, true, true)?.0; + if matches!(input, PipelineData::ExternalStream { .. }) { + input = check_subexp_substitution(input)?; + } + } } } Ok(input) } +fn check_subexp_substitution(mut input: PipelineData) -> Result { + let consume_result = might_consume_external_result(input); + input = consume_result.0; + let failed_to_run = consume_result.1; + if let PipelineData::ExternalStream { + stdout, + stderr, + exit_code, + span, + metadata, + trim_end_newline, + } = input + { + let stderr_msg = match stderr { + None => "".to_string(), + Some(stderr_stream) => stderr_stream.into_string().map(|s| s.item)?, + }; + if failed_to_run { + Err(ShellError::ExternalCommand( + "External command failed".to_string(), + stderr_msg, + span, + )) + } else { + // we've captured stderr message, but it's running success. + // So we need to re-print stderr message out. + if !stderr_msg.is_empty() { + eprintln!("{stderr_msg}"); + } + Ok(PipelineData::ExternalStream { + stdout, + stderr: None, + exit_code, + span, + metadata, + trim_end_newline, + }) + } + } else { + Ok(input) + } +} + pub fn eval_variable( engine_state: &EngineState, stack: &Stack, diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs index 2fb506a32..ada9ace2e 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline_data.rs @@ -531,7 +531,7 @@ impl PipelineData { // Only need ExternalStream without redirecting output. // It indicates we have no more commands to execute currently. if let PipelineData::ExternalStream { - stdout: None, + stdout, stderr, mut exit_code, span, @@ -542,22 +542,56 @@ impl PipelineData { 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. + // use a thread to receive stderr message. + // Or we may get a deadlock if child process sends out too much bytes to stdout. // - // 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 = stderr_stream + // For example: in normal linux system, stdout pipe's limit is 65535 bytes. + // if child process sends out 65536 bytes, the process will be hanged because no consumer + // consumes the first 65535 bytes + // So we need a thread to receive stderr message, then the current thread can continue to consume + // stdout messages. + let stderr_handler = stderr.map(|stderr| { + let stderr_span = stderr.span; + let stderr_ctrlc = stderr.ctrlc.clone(); + ( + thread::Builder::new() + .name("stderr consumer".to_string()) + .spawn(move || { + stderr + .into_bytes() + .map(|bytes| bytes.item) + .unwrap_or_default() + }) + .expect("failed to create thread"), + stderr_span, + stderr_ctrlc, + ) + }); + let stdout = stdout.map(|stdout_stream| { + let stdout_ctrlc = stdout_stream.ctrlc.clone(); + let stdout_span = stdout_stream.span; + let stdout_bytes = stdout_stream .into_bytes() .map(|bytes| bytes.item) .unwrap_or_default(); + RawStream::new( + Box::new(vec![Ok(stdout_bytes)].into_iter()), + stdout_ctrlc, + stdout_span, + None, + ) + }); + let stderr = stderr_handler.map(|(handler, stderr_span, stderr_ctrlc)| { + let stderr_bytes = handler + .join() + .map_err(|err| { + ShellError::ExternalCommand( + "Fail to receive external commands stderr message".to_string(), + format!("{err:?}"), + stderr_span, + ) + }) + .unwrap_or_default(); RawStream::new( Box::new(vec![Ok(stderr_bytes)].into_iter()), stderr_ctrlc, @@ -565,7 +599,6 @@ impl PipelineData { None, ) }); - match exit_code { Some(exit_code_stream) => { let ctrlc = exit_code_stream.ctrlc.clone(); @@ -578,7 +611,7 @@ impl PipelineData { } ( PipelineData::ExternalStream { - stdout: None, + stdout, stderr, exit_code: Some(ListStream::from_stream(exit_code.into_iter(), ctrlc)), span, @@ -590,7 +623,7 @@ impl PipelineData { } None => ( PipelineData::ExternalStream { - stdout: None, + stdout, stderr, exit_code: None, span,