diff --git a/crates/nu-command/tests/commands/run_external.rs b/crates/nu-command/tests/commands/run_external.rs index e93b8dc515..151834c54b 100644 --- a/crates/nu-command/tests/commands/run_external.rs +++ b/crates/nu-command/tests/commands/run_external.rs @@ -139,17 +139,6 @@ 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 f87b19e4e2..ac61081aac 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -704,14 +704,7 @@ pub fn eval_expression_with_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)) - } + Ok(might_consume_external_result(input)) } // Try to catch and detect if external command runs to failed. @@ -1216,72 +1209,14 @@ pub fn eval_subexpression( mut input: PipelineData, ) -> Result { for pipeline in block.pipelines.iter() { - 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)?; - } - } + for expr in pipeline.elements.iter() { + input = eval_element_with_input(engine_state, stack, expr, input, true, false)?.0 } } 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 { - label: "External command failed".to_string(), - help: 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 40026e25be..2ca731d7dd 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline_data.rs @@ -537,7 +537,7 @@ impl PipelineData { // Only need ExternalStream without redirecting output. // It indicates we have no more commands to execute currently. if let PipelineData::ExternalStream { - stdout, + stdout: None, stderr, mut exit_code, span, @@ -548,54 +548,22 @@ impl PipelineData { let exit_code = exit_code.take(); // Note: - // use a thread to receive stderr message. - // Or we may get a deadlock if child process sends out too much bytes to stdout. + // In run-external's implementation detail, the result sender thread + // send out stderr message first, then stdout message, then exit_code. // - // 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 + // 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 .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 { - label: "Fail to receive external commands stderr message".to_string(), - help: format!("{err:?}"), - span: stderr_span, - }) - .unwrap_or_default(); RawStream::new( Box::new(vec![Ok(stderr_bytes)].into_iter()), stderr_ctrlc, @@ -603,6 +571,7 @@ impl PipelineData { None, ) }); + match exit_code { Some(exit_code_stream) => { let ctrlc = exit_code_stream.ctrlc.clone(); @@ -615,7 +584,7 @@ impl PipelineData { } ( PipelineData::ExternalStream { - stdout, + stdout: None, stderr, exit_code: Some(ListStream::from_stream(exit_code.into_iter(), ctrlc)), span, @@ -627,7 +596,7 @@ impl PipelineData { } None => ( PipelineData::ExternalStream { - stdout, + stdout: None, stderr, exit_code: None, span,