Ignore EPIPE in CLI #746
Merged
Ignore EPIPE in CLI #746
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Piping command output to a process that performs a partial read before
closing the pipe, such as `head`, will cause an `EPIPE` to be raised on
the next write attempt. The standard `(e)print(ln)!` macros will panic
on any errors when writing, including `EPIPE`. One option to handle this
is to switch to `write(ln)!`, but this will inject new requirements to
handle `Result` where used, which can be onerous.
Create wrappers around the print macros to ignore `EPIPE`, and replace
calls to the originals with them. Update `main` to ignore any `EPIPE`s
returned from `writeln!` calls in subcommands.
We deliberately do not make this change to the `auth login/logout`
subcommands as these are mutating the config. Failure to notify the user
of the changes is fatal.
Before:
$ oxide system networking switch-port-settings show | head -1
switch0/qsfp0
thread 'tokio-runtime-worker' panicked at library/std/src/io/stdio.rs:1021:9:
failed printing to stdout: Broken pipe (os error 32)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at cli/src/main.rs:102:10:
called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(15), ...)
After:
$ oxide system networking switch-port-settings show | head -1
switch0/qsfp0
wfchandler
force-pushed
the
wc/epipe-panic
branch
from
83b71eb to
8eb27f3
Compare
|
Sorry to bombard you with reviews @ahl, are there any other maintainers I can spread these out to? |
I think I'm on the only one with state on why things are the way they are and where they're going. I feel like you're on a trajectory to shouldering some of that load. |
ahl
approved these changes
Jul 17, 2024
wfchandler
added a commit
that referenced
this pull request
Jul 18, 2024
With f472bce (Ignore EPIPE in CLI (#746), 2024-07-17) we added the `_nopipe` variants of the `(e)print(ln)!` macros to avoid panicking when piping to head(1). However, we do not systematically enforce their use, which will inevitably lead inconsistent usage within the project. Add `clippy` lints to ban use of the print macros in all non-test code. Either the `_nopipe` variants should be used when we don't care if the input is read, or `write!` when the information is crucial, such as an interactive session.
wfchandler
added a commit
that referenced
this pull request
Aug 7, 2024
With f472bce (Ignore EPIPE in CLI (#746), 2024-07-17) we added the `_nopipe` variants of the `(e)print(ln)!` macros to avoid panicking when piping to head(1). However, we do not systematically enforce their use, which will inevitably lead inconsistent usage within the project. Add `clippy` lints to ban use of the print macros in all non-test code. Either the `_nopipe` variants should be used when we don't care if the input is read, or `write!` when the information is crucial, such as an interactive session.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Piping command output to a process that performs a partial read before closing the pipe, such as
head, will cause anEPIPEto be raised on the next write attempt. The standard(e)print(ln)!macros will panic on any errors when writing, includingEPIPE. One option to handle this is to switch towrite(ln)!, but this will inject new requirements to handleResultwhere used, which can be onerous.Create wrappers around the print macros to ignore
EPIPE, and replace calls to the originals with them. Updatemainto ignore anyEPIPEs returned fromwriteln!calls in subcommands.We deliberately do not make this change to the
auth login/logoutsubcommands as these are mutating the config. Failure to notify the user of the changes is fatal.Before:
After:
$ oxide system networking switch-port-settings show | head -1 switch0/qsfp0