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

Replace anyhow with proper error types #2741

Open
matthiasbeyer opened this issue Sep 22, 2024 · 6 comments
Open

Replace anyhow with proper error types #2741

matthiasbeyer opened this issue Sep 22, 2024 · 6 comments
Labels
feat New feature or request

Comments

@matthiasbeyer
Copy link
Contributor

anyhow is for application error handling, please replace it with dedicated error types in your library crates, so users of these crates can do sane error handling.

@flub
Copy link
Contributor

flub commented Sep 23, 2024

Could you be more specific to which error return values you would like to match on for different failure cases?

In general you are right, and as iroh moves towards 1.0 we probably also should migrate to more specific error types. On the other hand we really need this to be guided by application needs rather than enumerate all the current failure types, as that would lead to breaking the API on every release. If we were to use a custom error type right now things would probably be a single opaque error type, which is just like anyhow, but worse.

So far in our own uses we have not needed to distinguish error types, but this doesn't mean there isn't a need! This is why to move this forward we really need specific APIs with specific usecases called out for needing a custom error type. We'd really appreciate your input on this.

@matthiasbeyer
Copy link
Contributor Author

So I am just starting out using iroh, and only because IPFS is not usable in Rust, because there is no machine-readable API specification that I can program against.

So all of what the following refers to is very much hacked together in a short time, so take everything with a grain of salt please.

My error type in my application (or rather, in the library that becomes my backend implementation), looks like this:

#[derive(Debug, thiserror::Error)]
pub enum Error {
    #[error("Creating Node")]
    CreatingNode(#[source] anyhow::Error),

    #[error("Spawning Node")]
    SpawningNode(#[source] anyhow::Error),

    #[error("Failed to get reader")]
    GettingReader(#[source] anyhow::Error),

    #[error("Failed to put blob")]
    PutBlobError(#[source] anyhow::Error),

    #[error("Shutdown failed")]
    Shutdown(#[from] anyhow::Error),

    #[error("Error while serializing metadata to JSON")]
    SerdeJson(#[from] serde_json::Error),

    #[error("Error while adding bytes")]
    AddingBytes(#[source] anyhow::Error),

    #[error("Error while reading bytes")]
    ReadingBytes(#[source] anyhow::Error),

    #[error("Error while putting content object")]
    PuttingContent(#[source] Box<Error>),

    #[error("Error while putting metadata object")]
    PuttingMetadata(#[source] Box<Error>),
}

Maybe that helps a bit. As you can probably see from the variant names, half of the cases above are just wrapping errors your API returns. The rest is my application logic errors, that again wrap anyhow::Error because the underlying APIs of yours.

Remember, I am pretty much in the "sketch things up" phase.

@b5
Copy link
Member

b5 commented Sep 24, 2024

Hey @matthiasbeyer, great to see you back after our last encounter,

Last we spoke, the team came to the working conclusion that we'd stick with anyhow pre-1.0, and transition to thiserror as part of a 1.0 release (reference), thankfully, we've made a bunch of progress on iroh in the last year, and are now starting to think seriously about a 1.0 roadmap. From our latest blog post:

We're aiming to publish a 1.0 roadmap in 2025.

Which means (unless others, especially @dignifiedquire disagree): we need to figure out when to transition over to thiserror, and at least have a plan in place in 2025, so maybe we use this issue & your baseline set of errors as a starting point for planning?

@b5 b5 added the feat New feature or request label Sep 24, 2024
@iacore
Copy link

iacore commented Oct 6, 2024

If the rewrite can happen in a day, would it happen earlier? What's blocking it from happening now?

Just looked at the source code. There are 146 invocations of anyhow!. I think this is a translation (i18n) problem, where we need to translate the 146 strings to types, and then group them into bigger error sets like described above. I think I can do this.

Are there any other ways where one can craft a unique anyhow::Error? It's like i18n, where I need to track all strings.

Then it goes on Crowdin, or some other place for translation.

@matheus23
Copy link
Contributor

matheus23 commented Oct 6, 2024

Are there any other ways where one can craft a unique anyhow::Error?

Yes. There is this implementation: impl<E> From<E> for Error where E: StdError + Send + Sync + 'static

So everywhere ? in any function that returns anyhow::Result, there might be a conversion from something that impls StdError (that includes anyhow::Error itself) where we may need to add either

  • an additional impl From<...> for IrohError
  • a new error enum variant with a #[from] annotation or
  • a call to .map_err before the ?.

There might be even more issues where code has to be re-achitected when something was assuming just anyhow::Error before, but now needs to handle multiple different error types, thus might need to be abstracted.

Unfortunately it's a little more involved than replacing anyhow! expressions with enum values.

@iacore
Copy link

iacore commented Oct 6, 2024

  • a new error enum variant with a #[from] annotation or

This is the way to go.

So the immediate plan looks like this:

#[derive(Debug, thiserror::Error)]
pub enum Error {
... 146 variants with existing error strings
... #[from] errors from other libraries
}

Then, we split the 146 variants into different sub errors, or have err.coarse_cause() return a enum that corresponds to the variants in #2741 (comment).

There might be even more issues where code has to be re-achitected when something was assuming just anyhow::Error before, but now needs to handle multiple different error types, thus might need to be abstracted.

The above design is one error. Breakage is unavoidable.

Any other concerns?

One thing I can think of is that different libraries, e.g. iroh-net might use its own crate error type. So essentially we are doing this for each procedure for each of the crate. We will need to make a flow diagram of this to visualize how different errors and 3rdparty error types coalesce into anyhow::Error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
Status: No status
Development

No branches or pull requests

5 participants