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

Give a more useful error when encryption fails with BrokenPipe #91

Merged
merged 1 commit into from
Mar 21, 2020

Conversation

str4d
Copy link
Owner

@str4d str4d commented Mar 21, 2020

If a user tries to pipe to a program that is not reading from stdin (or
stops doing so early), the default Rust error is something like:

Broken pipe (os error 32)

which is pretty opaque. While there might be other possible causes for
this error, we handle the most likely cause by wrapping this with a
custom error that suggests the user check whether the output is being
read. For example, this command would trigger the error:

rage -p -a file.txt | cat foo

while this would not:

rage -p -a file.txt | cat -

Closes #54.

If a user tries to pipe to a program that is not reading from stdin (or
stops doing so early), the default Rust error is something like:

    Broken pipe (os error 32)

which is pretty opaque. While there might be other possible causes for
this error, we handle the most likely cause by wrapping this with a
custom error that suggests the user check whether the output is being
read. For example, this command would trigger the error:

    rage -p -a file.txt | cat foo

while this would not:

    rage -p -a file.txt | cat -

Closes #54.
@codecov
Copy link

codecov bot commented Mar 21, 2020

Codecov Report

Merging #91 into master will decrease coverage by 0.35%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
- Coverage   53.96%   53.61%   -0.36%     
==========================================
  Files          24       24              
  Lines        1979     1992      +13     
==========================================
  Hits         1068     1068              
- Misses        911      924      +13     
Impacted Files Coverage Δ
rage/src/bin/rage/error.rs 0.00% <0.00%> (ø)
rage/src/bin/rage/main.rs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b2b055...e059421. Read the comment docs.

@str4d str4d merged commit cb2b6ae into master Mar 21, 2020
@str4d str4d deleted the 54-pipe-errors branch March 21, 2020 01:21
@xmo-odoo
Copy link

@str4d is there any way rage could know if the output was likely never read and only error in that case?

My use case was a log file, so to test that I wasn't misusing the tool I round-tripped the file through less to see if it came out alright. Since it did I simply quit (without going to the end of the logfile), which broke the pipe, then displayed the error. I'd have expected it to do nothing, breaking a pipe in that situation is pretty standard behaviour.

Or maybe just don't display an error at all on broken pipe? I feel most tools intended for CLI pipelines assume the user knows what they do. Since the age/rage UI seems designed for use in pipelines that seems like it would be a sensible behaviour.

@str4d
Copy link
Owner Author

str4d commented Feb 16, 2021

Mmm, it's a good point that a broken pipe during a partial write to stdout isn't something that should be considered an error (but should log a warning if logging is enabled).

I still think that a broken pipe after no reading should be an error, and that's really the case I was trying to improve with this PR.

@str4d
Copy link
Owner Author

str4d commented Feb 16, 2021

I've opened #208 for this. Thanks for reporting it!

@xmo-odoo
Copy link

xmo-odoo commented Feb 16, 2021

I still think that a broken pipe after no reading should be an error, and that's really the case I was trying to improve with this PR.

Yep that's what I understood and I don't disagree, but I don't know how easy it would be to diagnose: I assume at best rage could detect whether the pipe broke after it'd written less than PIPE_BUF? But that could be ridiculously high (apparently 64k on modern linuxes, and that's just a default it can be bumped to 1MB), and it's unclear whether libc uses the system's setup or has its own constant.

Definitely a difficult problem, I don't envy that job.

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

Successfully merging this pull request may close these issues.

Piping armor'd output to echo on macOS fails with "Error: Broken pipe (os error 32)"
2 participants