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

Prevent CLI to panic when the pipe is broken, fixes #5 #29

Merged
merged 5 commits into from
Oct 12, 2024
Merged

Prevent CLI to panic when the pipe is broken, fixes #5 #29

merged 5 commits into from
Oct 12, 2024

Conversation

jnioche
Copy link
Owner

@jnioche jnioche commented Oct 11, 2024

No description provided.

@jnioche jnioche added the enhancement New feature or request label Oct 11, 2024
@jnioche jnioche added this to the 0.3.0 milestone Oct 11, 2024
@jnioche jnioche requested a review from xoen October 11, 2024 19:31
@xoen
Copy link
Collaborator

xoen commented Oct 12, 2024

I'm trying to wrap my head around this...not quite the diff itself which is relatively "clear" (use standard C library's signal() function to "set the disposition of the signal" to use the default action)

But I'm trying to figure out why this happens, this SO answer is the best explanation I've found so far.

TL;DR: it's to do with how println!() expands to use ::std::io::_print() which can panic (the answers point to the reasons why but it doesn't really matter)

...but it's complicated and it shouldn't be! We write Rust so that we can avoid the horrible pitfalls of C most of the times...using the standard C library directly defeats the purpose of this :)

The problem really is that println!() fails to get hold of stdout and it fails.

But there is an alternative, use writeln!(), which returns a Result<T> instead of panicking!
This makes the "ignore write error" behaviour more explicit, for example if you print using writeln!() like this:

// note that `writeln!()` uses the `std::io::Write` trait which has to be in scope
writeln!(std::io::stdout(), "{}...", whatever);

Rust would warn you that writeln!() may fail, it returns a Result and the potential error should be handled explicitly.
If you just unwrap() you get roughly the same behaviour as before (panic) but in this case you could use unwrap_or_default() to ignore the error and carry on.

In other words replace the println! in main.rs with writeln!(std::io::stderr(), [...] should solve the issue (unless I'm misunderstanding this, which is possible of course) without having to rely on conditional compilation, foreign functions, unstable, the standard C library being present and what not :) (which has the added benefit of possibly solving this problem on any non-unix platform)

Don't know if any of this makes sense :)

@jnioche
Copy link
Owner Author

jnioche commented Oct 12, 2024

Clippy doesn't like it
https://rust-lang.github.io/rust-clippy/master/index.html#explicit_write

We'll tell it to shut up

src/main.rs Outdated Show resolved Hide resolved
@xoen
Copy link
Collaborator

xoen commented Oct 12, 2024

Possibly worthy adding a note to the CHANGELOG for this?

### Fixed

- [...]
- prevent CLI to panic when the pipe is broken ([issue #5](https://github.com/jnioche/carbonintensity-api/issues/5))

@jnioche jnioche merged commit 4ed6cf8 into main Oct 12, 2024
1 check passed
@jnioche jnioche deleted the 5 branch October 12, 2024 15:39
Copy link
Collaborator

@xoen xoen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants