Skip to content

Commit

Permalink
fix(panic): swallows broken pipe errors
Browse files Browse the repository at this point in the history
We were unwrapping an `Err` from the `write!` calls, of which things
like `BrokenPipe` can happen when the write end of a pipe is closed.
Because we were exit'ing the process anyways at the time the `panic` was
occurring, we are now just swallowing the error.

Changing this API to bubble up the result would be a
breaking change.

Another approach would be to try and write to stdout, if that fails due
to a broken pipe then use stderr. However, that would change the
semantics in what could be argued is a breaking change. Simply dropping
the error, can always be changed to this "use stderr if stdout is
closed" approach later if desired.

For a great explanation of the types of errors see the README in
`calm_io`:

https://github.com/myrrlyn/calm_io/blob/a42845575a04cd8b65e92c19d104627f5fcad3d7/README.md
  • Loading branch information
kbknapp committed Nov 30, 2021
1 parent 0c7da9f commit accf3d2
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 5 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "clap"
version = "2.33.3"
version = "2.33.4"
authors = ["Kevin K. <kbknapp@gmail.com>"]
exclude = ["examples/*", "clap-test/*", "tests/*", "benches/*", "*.png", "clap-perf/*", "*.dot"]
repository = "https://github.com/clap-rs/clap"
Expand Down
23 changes: 19 additions & 4 deletions src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,14 +391,29 @@ impl Error {
}
}

/// Prints the error to `stderr` and exits with a status of `1`
/// Prints the error message and exits. If `Error::use_stderr` evaluates to `true`, the message
/// will be written to `stderr` and exits with a status of `1`. Otherwise, `stdout` is used
/// with a status of `0`.
pub fn exit(&self) -> ! {
if self.use_stderr() {
wlnerr!("{}", self.message);
wlnerr!(@nopanic "{}", self.message);
process::exit(1);
}
let out = io::stdout();
writeln!(&mut out.lock(), "{}", self.message).expect("Error writing Error to stdout");
// We are deliberately dropping errors here. We could match on the error kind, and only
// drop things such as `std::io::ErrorKind::BrokenPipe`, however nothing is being bubbled
// up or reported back to the caller and we will be exit'ing the process anyways.
// Additionally, changing this API to bubble up the result would be a breaking change.
//
// Another approach could be to try and write to stdout, if that fails due to a broken pipe
// then use stderr. However, that would change the semantics in what could be argued is a
// breaking change. Simply dropping the error, can always be changed to this "use stderr if
// stdout is closed" approach later if desired.
//
// A good explanation of the types of errors are SIGPIPE where the read side of the pipe
// closes before the write side. See the README in `calm_io` for a good explanation:
//
// https://github.com/myrrlyn/calm_io/blob/a42845575a04cd8b65e92c19d104627f5fcad3d7/README.md
let _ = writeln!(&mut io::stdout().lock(), "{}", self.message);
process::exit(0);
}

Expand Down
4 changes: 4 additions & 0 deletions src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,10 @@ macro_rules! impl_settings {

// Convenience for writing to stderr thanks to https://github.com/BurntSushi
macro_rules! wlnerr(
(@nopanic $($arg:tt)*) => ({
use std::io::{Write, stderr};
let _ = writeln!(&mut stderr().lock(), $($arg)*);
});
($($arg:tt)*) => ({
use std::io::{Write, stderr};
writeln!(&mut stderr(), $($arg)*).ok();
Expand Down

0 comments on commit accf3d2

Please sign in to comment.