Skip to content
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

exit on broken pipe #279

Closed
wants to merge 1 commit into from
Closed

exit on broken pipe #279

wants to merge 1 commit into from

Conversation

rirze
Copy link

@rirze rirze commented Feb 29, 2020

Using a pipe to restrict the output of skim -f <somestring> results in an error:

thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', src/libstd/io/stdio.rs:805:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The error is innocuous, yet should probably be addressed in a future commit. Ideally, skim should terminate once it encounters a Broken Pipe error internally. ripgrep has gone through this same feature progression, as documented by these links:

BurntSushi/ripgrep#200
https://github.com/BurntSushi/ripgrep/pull/586/files

In that discussion, there is a hacky fix to terminate the program once it encounters a SIGPIPE error. This fix is what I have included in this PR.

Like @BurntSushi laments, this error should be handled naturally within the program. I'm not too comfortable with Rust so I wasn't able to understand how he handles in ripgrep, but in case anyone wants to understand it, the commit that makes this feature possible is at https://github.com/BurntSushi/ripgrep/pull/1017/files#diff-639fbc4ef05b315af92b4d836c31b023

I've tested this hotfix on my local machine and it seems to terminate as expected. Speed wise, I am not seeing any difference.

@BurntSushi
Copy link

ripgrep today handles this by inspecting I/O errors. If it's a broken pipe, then ripgrep quits gracefully. For example: https://github.com/BurntSushi/ripgrep/blob/50d2047ae2c0ce2edb5189ae3071538e21b15822/crates/core/main.rs#L95-L98

This approach is also a fine one to take, and mirrors how a CLI program written in C would behave by default. (SIGPIPE is suppressed by Rust's runtime, so this is kind of a Rust thing to deal with.)

@lotabout
Copy link
Owner

lotabout commented Mar 1, 2020

@rirze Gonna close this and choose the inspecting approach. Thanks!

@lotabout lotabout closed this Mar 1, 2020
@lotabout lotabout mentioned this pull request Mar 1, 2020
@rirze rirze deleted the reset_sigpipe branch March 1, 2020 15:58
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.

3 participants