Skip to content

[RFC] Switched error handling to anyhow #81

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

Closed
wants to merge 4 commits into from

Conversation

rdelfin
Copy link
Contributor

@rdelfin rdelfin commented Feb 11, 2020

As of rust 1.34+, many crates have switched to using anyhow for error handling. This is because anyhow provides support for returning any error implementing std::error::Error. This change basically replaces out all uses of the failure crate with anyhow. This could cause two potential issues:

  1. Compatibility breakages, this will require clients to handle a completely new return type
  2. Will add a hard dependency on rustc 1.34 for any clients

Overall though, I believe this change could be worth it for the upside of allowing clients to use any error type that implements std::error::Error.

@rdelfin rdelfin requested a review from brayniac February 11, 2020 22:08
@brayniac
Copy link
Collaborator

@rdelfin - thanks for the PR. I haven't used anyhow yet, are we going to miss out on features like backtrace by switching? I'm trying to understand if there's any downside to switching.

So far it looks like all the CI tests are failing.

@jvns - do you have any experience with anyhow vs failure or any opinions on this?

let cname =
CString::new(name).map_err(|_| format_err!("Nul byte in Kprobe name: {}", name))?;
CString::new(name).map_err(|_| anyhow::anyhow!("Nul byte in Kprobe name: {}", name))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like anyhow exports format_err!? If so, that could save a lot of lines in this diff by doing use anyhow::*;

Cargo.toml Outdated
@@ -12,6 +12,7 @@ homepage = "https://github.com/rust-bpf/rust-bcc"
edition = '2018'

[dependencies]
anyhow = "1.0.26"
bcc-sys = "0.12.0"
byteorder = "1.3.1"
failure = "0.1.5"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we'd want to remove failure here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice! Let me switch it up then

@rdelfin
Copy link
Contributor Author

rdelfin commented Feb 13, 2020

Glad to help out. I've mostly figured out the CI failures (seems to be I just missed fixing the examples). I'll send out a fix.

The point on backtraces is... Interesting. anyhow does support returning backtraces. However, where failure has it's own backtrace type, anyhow return an std::backtrace::Backtrace, which seems to still be on the rust nightly builds only. If you feel like this is a vital component of the library, it could make sense to hold off this PR until the feature becomes stable.

@jvns
Copy link
Collaborator

jvns commented Feb 14, 2020

cc @kamalmarhubi who knows more about the current state of rust error handling than me

@kamalmarhubi
Copy link

I haven't been keeping up super closely with the RFC to fix the error trait which came out of issues with failure which are discussed at some length in rust-lang-deprecated/failure#209.

That said, my understanding of the best practices for a library is to just define a custom error type that implements std::error::Error. Using a higher level library forces a dependency on the library's users. If the custom error type implements std::error::Error then it'll interoperate with whatever folks choose to use in their application.

Alternatively, you could replace failure::Error with Box<dyn std::error::Error> since it looks like the only use of failure in the crate is to call format_err, which is not really that different from Err(format!(...)? thanks to impl From<String> for Box<dyn std:error::Error> (playground).

In any case, I'd be wary of changing from failure to a different library without a bit more understanding of where RFC 2504 & backtrace support in std is at. Changing to a custom type or Box<dyn std::error::Error> is fine though since those solutions will work no matter what.

@rdelfin
Copy link
Contributor Author

rdelfin commented Feb 14, 2020

That all makes a lot of sense. @kamalmarhubi would it make more sense then to change the library to use something like [thiserror](https://github.com/dtolnay/thiserror) and return them through a Box<dyn std::error::Error>? I'd be glad to add more structures to the errors here :)

@brayniac brayniac mentioned this pull request Jun 12, 2020
@brayniac
Copy link
Collaborator

Closing this one out since #88 is now merged

@brayniac brayniac closed this Jun 29, 2020
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.

4 participants