-
Notifications
You must be signed in to change notification settings - Fork 52
summarize: Don't panic on broken pipe #243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
I hit rust-lang#31 while piping summarize to head. The original issue was in part due to `prettytable` panicking. That's since been upgraded to a version with the fix, but some of the `println!`s that happen after `table.printstd()` will also panic from the broken pipe: $ summarize summarize ... | head ... thread 'main' panicked at library/std/src/io/stdio.rs:1165:9: failed printing to stdout: Broken pipe (os error 32) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace To fix this, change the `println!`s to `writeln!`s, and ignore any errors. Fixes rust-lang#31.
summarize/src/main.rs
Outdated
@@ -142,7 +142,11 @@ fn diff(opt: DiffOpt) -> Result<(), Box<dyn Error + Send + Sync>> { | |||
|
|||
table.printstd(); | |||
|
|||
println!("Total cpu time: {:?}", results.total_time); | |||
_ = writeln!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should just exit the process on a broken pipe rather than keep writing data to nowhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense -- implemented a catch-all "exit on failure" in 59876d9.
Do you think it should special-case broken pipe to return a visible error otherwise? or better to just give up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it should special-case broken pipe to return a visible error otherwise?
Yes, I think it should still show an error and exit when an error other than a broken pipe occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, implemented in 75fde69
What do you think about just configuring SIGPIPE to abort the process? For example like this. I think that's a more universal solution than patching a few print lines. |
Non-unix platforms may not have signals (e.g. windows). |
Right, but I thought that this issue only happens on Unix? Does Rust also somehow ignore sigpipe (or something similar) on Windows? Is that even a thing? |
Closing a pipe on Windows just causes further writes to fail. The |
I see. Well, hard to say how many people redirect CLI commands into pipes on Windows with |
Sure, I don't know how much of an issue it is in practice. Incidentally |
Why does |
"silently panic"? |
|
Ah. Likely because nobody has proposed it to libs-api yet. |
I hit #31 while piping summarize to head. The original issue was in part due to
prettytable
panicking. That's since been upgraded to a version with the fix, but some of theprintln!
s that happen aftertable.printstd()
will also panic from the broken pipe:To fix this, change the
println!
s towriteln!
s, and ignore any errors.Fixes #31.