mirror of
https://github.com/nushell/nushell.git
synced 2026-10-28 17:05:03 +08:00
berce
f3cc93329c
## Description
`ignore`, `save`, `complete` and `tee` ask for an external's stdout and
stderr on their own pipes (`PipeSeparate`), so an external at the end of
a `try` block that is piped into one of them gets a stderr pipe too. A
`try` with `catch` or `finally` then collects the block's output with
`try-collect`, and `collect` calls `into_value` on the child stream.
`ChildProcess::into_bytes` refuses a child that still has a stderr pipe
("internal error: stderr should not exist"), so the error went to
`catch` even though the external exited 0.
A value can't carry a separate stderr, so `try-collect` now leaves such
a child as a stream instead of turning it into a value:
- `ChildProcess::buffer_output` reads stdout and stderr to the end
(stderr on its own thread, with the reader that `wait_with_output`
already used, now shared as `collect_pipes`), waits for the child,
checks its exit status the way `into_bytes` does, and puts the output
back so it can still be read. `do` already rebuilds a finished child
from buffers in a similar way.
- `try_collect` in `eval_ir.rs` does this for a child with a stderr pipe
and passes it on, so the command after the `try` gets stdout, stderr and
the exit status as if there were no `try`. Failing externals still error
inside the `try`, so `catch` still runs for them. Everything else still
goes through `collect`.
- The pipes are read before the pipefail exit check. Waiting for the
exit first hung on a child that filled its stderr pipe.
- `finally` gets a copy of the result, and only a value can be copied.
So at `begin-finally`, a child passed on like this becomes its stdout,
and its stderr goes to the block's stderr, the same as for `let x =
^external`.
`into_bytes` keeps its check: a caller that wants a value can't keep
stderr.
## User-facing changes (Release notes)
Fixed `try { ^external } catch { ... } | ignore` (also with `save
--stderr`, `complete`, `tee --stderr`, and inside `do` or a custom
command) running the `catch` block with "internal error: stderr should
not exist" when the external succeeded. The external's stderr now
reaches the next command the same way it does without the `try`.
With `finally`, the `try` doesn't fail that way anymore either, but the
result is still turned into a value so `finally` can see it. The next
command gets the external's stdout as a value and its stderr is printed,
like for `let x = ^external`. So `save --stderr` after `try ... finally`
gets an empty stderr, and `complete` still says it only works with
external commands.
## Additional notes
Fixes #19335
Tests in `tests/shell/mod.rs`:
- `try_catch_keeps_a_piped_stderr_of_a_successful_external` runs `try {
X } catch { print caught } | C` and `X | C` and checks that stdout and
stderr match. `C` is each of `ignore`, `ignore --stdout`, `ignore
--stderr`, `complete`, and `save --stderr`. `X` is either a short
stdout+stderr external or one that writes 200KB to stderr.
- `try_catch_piped_into_ignore_catches_only_a_failing_external` covers
the same inside a `def`, `finally` with and without `catch`, and a
failing external that is still caught.
On main, 8 of the 14 cases fail, the 5 with 200KB of stderr hang until
killed, and only the failing-external case passes.
- `cargo test --test tests`: 2421 passed
- `cargo test -p nu-protocol -p nu-engine -p nu-cmd-lang`, `cargo test
-p nu-command -- complete ignore save tee try_`: passed
- `cargo fmt --all -- --check`, `cargo clippy -p nu-engine -p
nu-protocol -p nu-cmd-lang --all-targets --no-deps -- -D warnings`, and
`cargo check -p nu-engine --no-default-features` with `-D warnings`:
clean (toolchain 1.99.1)
More than 64KB of stdout with no stderr pipe still hangs with pipefail
on, as on 0.116.2; that path isn't touched here.