Throw out error if external command in subexpression is failed to run (#8204)

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() {

crates/nu-engine/src/eval.rs

@@ -699,7 +699,14 @@ 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))
+    }
 }
 
 // Try to catch and detect if external command runs to failed.
@@ -1197,14 +1204,72 @@ pub fn eval_subexpression(
     mut input: PipelineData,
 ) -> Result<PipelineData, ShellError> {
     for pipeline in block.pipelines.iter() {
-        for expr in pipeline.elements.iter() {
+        for (expr_indx, expr) in pipeline.elements.iter().enumerate() {
-            input = eval_element_with_input(engine_state, stack, expr, input, true, false)?.0
+            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<PipelineData, ShellError> {
+    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,

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,21 +542,55 @@ impl PipelineData {
             let exit_code = exit_code.take();
 
             // Note:
-            // In run-external's implementation detail, the result sender thread
+            // use a thread to receive stderr message.
-            // send out stderr message first, then stdout message, then exit_code.
+            // 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
+            // For example: in normal linux system, stdout pipe's limit is 65535 bytes.
-            // But not the case of `stderr`, so if `stderr` is not None
+            // if child process sends out 65536 bytes, the process will be hanged because no consumer
-            // We need to consume stderr message before reading external commands' exit code.
+            // consumes the first 65535 bytes
-            //
+            // So we need a thread to receive stderr message, then the current thread can continue to consume
-            // Or we'll never have a chance to read exit_code if stderr producer produce too much stderr message.
+            // stdout messages.
-            // So we consume stderr stream and rebuild it.
+            let stderr_handler = stderr.map(|stderr| {
-            let stderr = stderr.map(|stderr_stream| {
+                let stderr_span = stderr.span;
-                let stderr_ctrlc = stderr_stream.ctrlc.clone();
+                let stderr_ctrlc = stderr.ctrlc.clone();
-                let stderr_span = stderr_stream.span;
+                (
-                let stderr_bytes = stderr_stream
+                    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()),
@@ -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,