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

This reverts commit dec0a2517f.

It breaks programs like `fzf`

# Description

Fixes: #8472 
Fixes:  #8313
Reopen: #7690 

# User-Facing Changes

_(List of all changes that impact the user experience here. This helps
us keep track of breaking changes.)_

# Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```

# After Submitting

If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.

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

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`
+    Ok(might_consume_external_result(input))
-    // 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.
@@ -1216,72 +1209,14 @@ pub fn eval_subexpression(
     mut input: PipelineData,
 ) -> Result<PipelineData, ShellError> {
     for pipeline in block.pipelines.iter() {
-        for (expr_indx, expr) in pipeline.elements.iter().enumerate() {
+        for expr in pipeline.elements.iter() {
-            if expr_indx != pipeline.elements.len() - 1 {
+            input = eval_element_with_input(engine_state, stack, expr, input, true, false)?.0
-                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 {
-                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,

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