Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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

Error handling #38

Open
wants to merge 1 commit into
base: master
from
Open

Error handling #38

wants to merge 1 commit into from

Conversation

@nrc
Copy link

nrc commented Jan 22, 2020

Modernise, unify, and simplify error handling in TiKV. I propose using a customised or future version of thiserror in all our modules. Converting the errors will make an excellent 'quest issue' for new contributors.

cc @breeswish @brson

@nrc nrc force-pushed the nrc:errors branch 3 times, most recently from 363c577 to 4ecb84c Jan 22, 2020
Signed-off-by: Nick Cameron <nrc@ncameron.org>
@nrc nrc force-pushed the nrc:errors branch from 4ecb84c to 5d0e3ea Jan 23, 2020
@siddontang siddontang requested review from breeswish, brson and BusyJay Jan 23, 2020
#[error(unwrap)]
pub enum Error {
#[error(transparent)]
Engine(#[from] crate::storage::kv::Error),

This comment has been minimized.

Copy link
@siddontang

siddontang Jan 23, 2020

Contributor

seem we don't need to use ErrorInner any more and passing Error directly doesn't affect the performance?

This comment has been minimized.

Copy link
@nrc

nrc Jan 23, 2020

Author

This error is already boxed so it doesn't need to be boxed here too. We still box some errors for performance.

@BusyJay

This comment has been minimized.

Copy link
Contributor

BusyJay commented Jan 23, 2020

Why not keep using failure? Why choosing thiserror over failure?

TiKV has some very large error types which are inefficient to pass by value. So
in some places (e.g.,
[src/storage](https://github.com/tikv/tikv/blob/master/src/storage/errors.rs))
we use a pattern of having `Error` and `ErrorInner` types and boxing the inner

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 23, 2020

Contributor

I think ErrorInner exists because we want to achieve minimal copy cost. It's not caused by quick_error library. Hence this should be able to be solved without switching the error library, right?

This comment has been minimized.

Copy link
@breeswish

breeswish Jan 23, 2020

Member

I think one of the goal of this RFC is to be modern (i.e. utilize the new std::Error). @BusyJay

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 23, 2020

Contributor

quick_error has been using std::error::Error from the beginning.

This comment has been minimized.

Copy link
@nrc

nrc Jan 23, 2020

Author

This RFC has a number of goals that are mostly unrelated, other than that they are easier to implement at once. I have looked at our error handling in a holistic way to find ways to improve. The ErrorInner stuff is mostly orthogonal to the choice of library, but using thiserror makes it easier to implement the extensions than using quick_error. I think thiserror is a better library because it does not reimplement anything from std::Error and its syntax is more succinct and idiomatic.

)))) => ...,
```

Once the error migration is complete, we are guaranteed that the second and

This comment has been minimized.

Copy link
@breeswish

breeswish Jan 23, 2020

Member

What is the "error migration"? How can it remove the second and the third match arm?

This comment has been minimized.

Copy link
@nrc

nrc Jan 23, 2020

Author

"error migration" means converting all our error types to the system proposed in this RFC. Due to the unwrap stuff, there are no deeply nested errors and we only ever have to look one level deep.

@nrc

This comment has been minimized.

Copy link
Author

nrc commented Jan 23, 2020

Why not keep using failure? Why choosing thiserror over failure?

Since much of the functionality of Failure was moved into the std lib, it has been pretty much deprecated as a library. It is now an "experimental library" (see https://github.com/rust-lang-nursery/failure#evolution) and shouldn't really be used.

quick-error is kind of dated and doesn't use newer features. That makes it much harder to implement the extensions described in the RFC.

Copy link

dtolnay left a comment

Weighing in because you asked on Twitter for anyone in the Rust community interested in error handling, but note that I know nothing about TiKV beyond what is in this document and some of the links.

`ErrC::Var4(ErrB::Var2("..."))` (note that the `String` is nested inside an
`ErrB`).

A prototype implementation can be seen in [this branch](https://github.com/dtolnay/thiserror/compare/master...nrc:boxed?expand=1).

This comment has been minimized.

Copy link
@dtolnay

dtolnay Jan 24, 2020

It looks like this implementation requires specialization, which is going to be a hard no from me on upstreaming until that feature is further along. But as you call out below, maintaining this on your fork is fine.

My second proposal is for `#[error(unwrap)]` on error types and
`#[from(unwrap)]` on fields (note that `#[from(boxed, unwrap)]` would be valid).
Where an error marked with `#[error(unwrap)]` is converted to an error type via
a `From` impl generated from `#[from(unwrap)]`, then where possible nested
errors will be unwrapped rather than nested.
Comment on lines +182 to +186

This comment has been minimized.

Copy link
@dtolnay

dtolnay Jan 24, 2020

I am not sold on this, or maybe not the prototype implementation of it, or maybe the RFC needs to call out ways this unwrap-conversion goes wrong and conventions for how we plan to prevent them. For example:

  • I tried adding an Error::DefinitelyNotRocks(String) variant to tikv::storage::txn::Error and it happily gets converted to backup::Error::Rocks because both are strings. Do we stop using String error variants anymore? What is the general guideline for which types to allow or avoid?

  • This forces a model where all errors of the same type go in the same variant, which I see as a step in the wrong direction. In backup::Error I see:

    #[error("IO error {0}")]
    Io(#[from] IoError),

    which would tend to produce unhelpful messages like "IO error Operation not permitted". I would rather see:

    #[error("failed to create external storage: {0}")]
    CreateStorage(IoError),
    #[error("failed to something else: {0}")]
    SomethingElse(IoError),

    But that's no longer supported:

    error[E0119]: conflicting implementations of trait `thiserror::private::__Rewrap<std::io::Error>` for type `errors::Error`:
      --> components/backup/src/errors.rs:95:17
       |
    95 | #[derive(Debug, Error)]
       |                 ^^^^^
       |                 |
       |                 first implementation here
       |                 conflicting implementation for `errors::Error`

Together these two mean that type-based unwrap conversion ascribes a global meaning to every error type. One error enum can't use CreateStorage(IoError) if a different error enum in a different package uses Io(IoError) because an unwrap-conversion could end up going from the general to the specific and thus attaching the wrong specific message to a general error, resulting in wrong and misleading user-facing messages.

Do you combat that by declaring that an error's message may be no more specific than its type, i.e. forbid CreateStorage(IoError)? That leads to pub struct CreateStorage(IoError) and you are back in the world of nesting and repetition: Error::CreateStorage(CreateStorage(e)).

It's worth mentioning that the alternative to unwrap-conversions is to just write the From impl which is not so bad:

impl From<TxnError> for Error {
    fn from(e: TxnError) -> Self {
        match e {
            TxnError::Engine(e) => Error::Engine(e),
            TxnError::Io(e) => Error::Io(e),
            TxnError::Other(e) => Error::Other(e),
            e @ TxnError::Codec(_)
            | e @ TxnError::Mvcc(_)
            | e @ TxnError::ProtoBuf(_)
            | e @ TxnError::InvalidTxnTso { .. }
            | e @ TxnError::InvalidReqRange { .. } => Error::Txn(e),
        }
    }
}
@BusyJay BusyJay mentioned this pull request Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.