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

Implement error system similar too the approach implemented in io::Error structure #180

Closed
andrii0lomakin opened this issue Nov 19, 2020 · 6 comments

Comments

@andrii0lomakin
Copy link
Collaborator

This issue was discussed in community chat. Main issues with io::Error are:

  1. Mapping between errors that happened inside the glommio library and presented in io::Errror is fuzzy and can not precisely describe the root cause of the issue.
  2. io::Error lacks useful information as for example the path of the file where error happens. The error system should provide means to expose such information without allocation overhead for no-error situations. In general, error types should be very thin and do not request a substantial amount of memory.

Despite the visible simplicity of the task, it requires deep investigation of current implementations of error handling in Rust and a good understanding of a possible set of errors which could happen (will happen) inside of the library.

@andrii0lomakin
Copy link
Collaborator Author

@bryandmc
Copy link
Collaborator

bryandmc commented Dec 8, 2020

I personally think a good pattern for a library like this could involve using thiserror from dtolnay that makes creating ergonomic, library errors much easier. Basically it lets us define one error type for this library (so that users of this library can work with a single type) and multiple enum variants for all the different error cases. It also makes it easier to encapsulate the source error inside of a particular variant (so the user can introspect what the actual cause of the error was). The thing that thiserror gives, is easy From/display implementations based on a proc macro.

This seems to be the most idiomatic way to create library errors at the moment. I can provide an example of what this looks like, if anyone is interested. I actually used to use a hacky declarative macro to do basically the same thing before finding this crate, so this doesn't involve some sort of custom dynamic error type or anything like that. It's just an enum at the end of the day with some generated impl's.

Thoughts? It's pretty easy to implement and I'd be happy to mock up an example using whatever errors we currently have as the starting point.

EDIT: here's an example of what the enum looks like with the macro

#[derive(thiserror::Error, Debug)]
pub enum QuicProtoError {
    #[error("socket bind error")]
    BindError(#[from] io::Error),

    #[error("error with quinn endpoint")]
    EndpointError(#[from] quinn::EndpointError),

    #[error("error setting udp socket options")]
    UdpSetupError(#[from] udp::UdpSocketError),

    #[error("error configuring TLS")]
    TlsConfigurationError(#[from] rustls::TLSError),

    #[error("error parsing certificate data")]
    CertificateParsingError(#[from] quinn::ParseError),

    #[error("error creating quic config")]
    QuicConfigError(#[from] anyhow::Error),

    #[error("QUIC connection error")]
    ConnectionError(#[from] quinn::ConnectionError),

    #[error("error connecting to backend - bad parameters provided.")]
    BadConnectionParameters(#[from] quinn::ConnectError),
}

@glommer
Copy link
Collaborator

glommer commented Dec 8, 2020

hello @bryandmc

That looks great to me. My main concern is that I'd like to make sure that this interops well with io::Error, because that is everywhere : one good example is the Async I/O traits.

That means not only being able to convert, but keeping meaningful messages and causes whenever possible.
In other words, something that resembles a "Bad File" in Glommio should translate to "Bad File" in io::Error, and not just going the lazy route of marshalling everything under Custom

@bryandmc
Copy link
Collaborator

bryandmc commented Dec 8, 2020

@glommer That shouldn't be a problem. It can preserve and entire chain of "source" errors. Meaning it can hold the io::Error that caused the error in the first place, along with other information. I'll take a look at the current code base and see how it'll fit in. That said, I think we can accomplish your goals with this pretty easily.

@glommer
Copy link
Collaborator

glommer commented Dec 8, 2020

our goals!!

I am more concerned about others that aren't originally io::Error. See for example the ones in sync::rwlock

bryandmc added a commit to bryandmc/glommio that referenced this issue Dec 14, 2020
Uses 'thiserror' and some refactoring to implement a cohesive error type that
can encompass all errors produced by glommio. Includes all the natural traits
like from/into so try ('?') should work as expected if you are using it on the
inner type of whichever variant you are using. Also included in this commit is
an example migration of the semaphore errors as an example of how it would
likely work. Assuming this is satisfactory, the rest of the errors can be
refactored to use this as well. This means we shouldn't need to piggyback on the
std::io::Error to express our intent. It should still be used as the inner type
if underlying libraries produce this, but otherwise the correct variant of
GlommioError should just be manually constructed.
glommer pushed a commit that referenced this issue Jan 6, 2021
Addresses #180: Implement error system
@glommer
Copy link
Collaborator

glommer commented Jan 7, 2021

Bryan handled this one - #227

@glommer glommer closed this as completed Jan 7, 2021
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

No branches or pull requests

3 participants