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

Introduce TransportError type #374

Merged
merged 2 commits into from Feb 11, 2019

Conversation

Projects
None yet
3 participants
@ilammy
Copy link
Member

ilammy commented Feb 11, 2019

It is not very convenient to use () for Result error type. Unit type does not implement Error trait and thus cannot be used with the ? operator. It is not very convenient for users which may need to return arbitrary errors from their transport callbacks. These may be IO errors from the standard library or some custom errors.

Introduce a new type TransportError which will represent errors for transport callbacks. It is an enumeration which can hold three kinds of errors:

  • Arbitrary std::error::Error type

    This kind allows to wrap any other error into a TransportError. We implement a From conversion so that TransportError works with the ? operator. This makes it very convenient to forward existing errors out of SecureSessionTransport callbacks.

  • Custom human-readable string

    This kind is useful when you don't have an Error handy but still want to return something more descriptive. Anything that can be converted into a String will do.

  • Unspecified kind

    This kind is useful to explicity say that we do not have any detailed information on the error. It will be used in some conversions of raw Themis errors.

Unfortunately, it's not possible to implement Error trait for TransportError because of a blanket impl on From/Into traits: it conflicts with out custom From<T: Error> implementation. Well, it's not a big deal for now, but may bite us later.

However, we do implement all other Error requirements like Display so the error should be convenient enough to use.

Note that the error itself is a struct which wraps an enumeration. This allows us to not expose enumeration variants to the user.

Also note the additional Send + Sync trait bounds on the Error impl. It is important to have these traits implemented if we want to be able to transfer errors between threads.

Update the SecureSessionTransport trait to use the new type.

Update usage in tests. Note how ? is used to forward channel errors and TransportError::new() usage for custom error reporting.

ilammy added some commits Feb 9, 2019

Introduce TransportError type
It is not very convenient to use () for Result error type. Unit type
does not implement Error trait and thus cannot be used with the "?"
operator. It is not very convenient for users which may need to return
arbitrary errors from their transport callbacks. These may be IO errors
from the standard library or some custom errors.

Introduce a new type "TransportError" which will represent errors for
transport callbacks. It is an enumeration which can hold three kinds
of errors:

  - Arbitrary std::error::Error type

    This kind allows to wrap any other error into a TransportError.
    We implement a From conversion so that TransportError works with
    the "?" operator. This makes it very convenient to forward existing
    errors out of SecureSessionTransport callbacks.

  - Custom human-readable string

    This kind is useful when you don't have an Error handy but still
    want to return something more descriptive. Anything that can be
    converted into a String will do.

  - Unspecified kind

    This kind is useful to explicity say that we do not have any
    detailed information on the error. It will be used in some
    conversions of raw Themis errors.

Unfortunately, it's not possible to implement Error trait for
TransportError because of a blanket impl on From/Into traits:
it conflicts with out custom From<T: Error> implementation.
Well, it's not a big deal for now, but may bite us later.

However, we do implement all other Error requirements like Display
so the error should be convenient enough to use.

Note that the error itself is a struct which wraps an enumeration.
This allows us to not expose enumeration variants to the user.

Also note the additional "Send + Sync" trait bounds on the Error impl.
It is important to have these traits implemented if we want to be able
to transfer errors between threads.
Use TransportError in Secure Session
Update the SecureSessionTransport trait to use the new type.

Update usage in tests. Note how "?" is used to forward channel errors
and TransportError::new() usage for custom error reporting.

@ilammy ilammy added the rust label Feb 11, 2019

@ilammy ilammy requested review from vixentael and Lagovas Feb 11, 2019

@vixentael
Copy link
Member

vixentael left a comment

I believe that having a separate error type for transport errors is a great idea!

@ilammy

This comment has been minimized.

Copy link
Member Author

ilammy commented Feb 11, 2019

Yeah, Rust has amazing ergonomics at times. While you have to spell out all these Results explicitly in the function signatures, it's very convenient to simply forward the error up the call stack using the ? operator (similar to exceptions in other languages).

However, this keeps all the benefits of strong static typing (you always know the precise type of the error, unless you explicitly erase it with something like Box<dyn Error>). And – what I think is the best feature of this error handling system – Rust does not hide an additional control flow path in the function that can return errors. In most languages with exceptions virtually any statement or expression can throw an exception. It is completely invisible in the code (and during code review). On the other hand, you will always see a ?, try!, or return if there is an early return due to an error in the Rust code.

@ilammy ilammy merged commit 7b19328 into cossacklabs:master Feb 11, 2019

6 checks passed

ci/circleci: android Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: php5 Your tests passed on CircleCI!
Details
ci/circleci: php70 Your tests passed on CircleCI!
Details
ci/circleci: php71 Your tests passed on CircleCI!
Details
ci/circleci: x86_64 Your tests passed on CircleCI!
Details

@ilammy ilammy deleted the ilammy:transport-error branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment