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

Use thiserror Errors instead of relying on anyhow #13

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

miguelfrde
Copy link

@miguelfrde miguelfrde commented Apr 22, 2024

This improves matching on particular errors when we need to handle them differently downstream. At the moment we would need to be matching error strings which is brittle and not ideal.

For now I left the possibility of converting a anyhow::Error to a DecodeError in this change, but every other error this crate exposes is now a variant of DecodeError. I've left EncodeError untouched.

I'm preparing a change on top of this one in the netlink-packet-route crate as well that makes that create expose more granular errors that can be matched against.

Bug #10

@miguelfrde miguelfrde force-pushed the main branch 3 times, most recently from f18501c to e2a36da Compare April 22, 2024 21:57
@miguelfrde miguelfrde marked this pull request as draft April 23, 2024 01:57
@miguelfrde miguelfrde marked this pull request as ready for review April 23, 2024 01:58
@cathay4t
Copy link
Member

@miguelfrde Changing dependency always require justification why new crate is better and risk is less.

I dislike anyhow as I spend 2+ hours to debug a issue which was hidden by anyhow crates. But this does not mean I am a fan of any new crate wrapping errors.

My expectation on error handling: not hiding root cause or origin of error. For example, when a missformed netlink message has invalid MTU data length, I am expecting error message indicate so instead of saying failed to parse netlink message.

If you want to introduce new crate thiserror, please provide evidence it could make our code more elegant than self-cooked error From trait or using current anyhow context.

@miguelfrde
Copy link
Author

Changing dependency always require justification why new crate is better and risk is less.

I totally understand the concern of not wanting to depend on new crates.

Fwiw this crate is mostly a convenience on top of std::error::Error.

My expectation on error handling: not hiding root cause or origin of error.

I believe with this approach I'm taking, this can be accomplished. A pattern I've grown to like is to leverage std::error::Errors in crates, for which thiserror is very convenient to easily generate the impls carrying context, structured and typed information as well as human readable messages. Then use anyhow::Error in applications and binaries that don't need to deal with the granularity of the error itself, but do want to get useful human readable messages.

In my opinion, anyhow in libraries is an anti-pattern because it doesn't let clients handle the errors appropriately. The motivation for this change is coming from https://fuchsia-review.googlesource.com/c/fuchsia/+/1023407 in which we need to check a specific error that the crate netlink-packet-route returns and we can't do so at the moment without doing a brittle string comparison on a string meant for human consumption.

I wrote this toy example to demonstrate that we can get the origin of the error and the entire trace of errors if we correctly leverage the #[source],#[from] features of thiserror: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a664c3ea76dcc8535e4bc33284e7c761

In this case we get a nice message showing the top level error plus all of the underlying errors that got us there:

Error: top level error failed

Caused by:
    0: bar failed
    1: baz failed: i'm baz

I have a follow-up for this change in rust-netlink/netlink-packet-route#118 I think this approach gets us what we both desire!

Let me know your thoughts. I'm more than happy to accommodate this PR to accomplish both human readable messages and machine usable errors.

This improves matching on particular errors when we need to handle
different conditions downstream.

It's still possible to convert a anyhow::Error to a DecodeError in this
change, but every other error this crate expsoes is now in a variant.
@little-dude
Copy link
Contributor

Huge +1 from me on the idea. The reason I went for untyped errors initially is that it allowed me to move fast in the first stages of the project. I've thought of moving to thiserror in the past, but was discouraged by the amount of work.

I like thiserror because it doesn't introduce anything new. It's just a macro to reduce boilerplate when implementing the standard library Error trait.

I'll try to find some time to review this in more details. Note that if we go with types errors, we should try to make it right from the start, because making changes to an error type will be a breaking change in the future.

Thanks for starting this @miguelfrde !

@gbbosak
Copy link

gbbosak commented Jul 18, 2024

@little-dude Anything that we need to do to move this forward? Thanks!

@little-dude
Copy link
Contributor

Hey @gbbosak, I apologize for not moving on this. I have very little time/energy/motivation to dedicate to open-source these days and I've been postponing this for far too long. I have a project to push past the finish line by end of this month at work, but I'll review first week of August.

@MarkusPettersson98
Copy link
Contributor

Hello @little-dude, I hope you are doing well!

Would you like some help with moving this forward? I currently have quite a lot of time that I'd like to dedicate to open source, so I wouldn't mind helping out if you'd like 😊

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.

5 participants