summarize: Don't panic on broken pipe#243
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
| table.printstd(); | ||
|
|
||
| println!("Total cpu time: {:?}", results.total_time); | ||
| _ = writeln!( |
There was a problem hiding this comment.
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.
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.
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.
|
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
prettytablepanicking. 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.