Skip to content

summarize: Don't panic on broken pipe - #243

Open
sharnoff wants to merge 3 commits into
rust-lang:masterfrom
sharnoff:summarize-pipe-ok
Open

summarize: Don't panic on broken pipe#243
sharnoff wants to merge 3 commits into
rust-lang:masterfrom
sharnoff:summarize-pipe-ok

Conversation

@sharnoff

Copy link
Copy Markdown

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 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 #31.

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.
Comment thread summarize/src/main.rs Outdated
table.printstd();

println!("Total cpu time: {:?}", results.total_time);
_ = writeln!(

Copy link
Copy Markdown
Member

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.

@sharnoff sharnoff Jun 24, 2025

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Author

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

@Kobzol

Kobzol commented Jul 7, 2025

Copy link
Copy Markdown
Member

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.

@ChrisDenton

Copy link
Copy Markdown
Member

Non-unix platforms may not have signals (e.g. windows).

@Kobzol

Kobzol commented Jul 7, 2025

Copy link
Copy Markdown
Member

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?

@ChrisDenton

Copy link
Copy Markdown
Member

Closing a pipe on Windows just causes further writes to fail. The println macro will panic on any failure to write so it will panic on a closed pipe.

@Kobzol

Kobzol commented Jul 7, 2025

Copy link
Copy Markdown
Member

I see. Well, hard to say how many people redirect CLI commands into pipes on Windows with summarize 😆

@ChrisDenton

ChrisDenton commented Jul 7, 2025

Copy link
Copy Markdown
Member

Sure, I don't know how much of an issue it is in practice.

Incidentally rustc has a safe_print macro for handling this across platforms. It still panics but with a payload that tells rustc to treat it as a normal (albeit fatal) error rather than an unexpected panic (this works because rustc uses catch_unwind to catch all panics).

@bjorn3

bjorn3 commented Jul 7, 2025

Copy link
Copy Markdown
Member

Why does println!() even produce a panic message for broken pipes? Can't it just silently panic?

@ChrisDenton

Copy link
Copy Markdown
Member

"silently panic"?

@bjorn3

bjorn3 commented Jul 7, 2025

Copy link
Copy Markdown
Member

std::panic::resume_unwind(Box::new(SomeMarkerType)) will unwind without producing any panic message.

@ChrisDenton

Copy link
Copy Markdown
Member

Ah. Likely because nobody has proposed it to libs-api yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Summarize panics if piped into head

4 participants