fix(transport): Improve Error type

[faern]

Motivation

Fixes #213

Solution

Makes the tonic::transport::Error type much simpler. It just forwards what's in the source error now. It was previously opaque anyway so a user of the library could not match on the ErrorKind or anything like that anyway.

However, this just solves the problem of the error not having redundant information in it. The library still uses a lot of opaque Box<dyn Error> internally. I personally think it would be nicer if tonic exposed a proper hyper::Error where the error comes from hyper etc. But I did not feel that was in scope for getting this part done.

[LucioFranco]

Would it make sense to add something here related to this being a transport error? Since basically we are wrapping any boxed error it might be nice to ensure that this type is know to be coming from the transport layer?

[faern]

That breaks entire point of the error just being a proxy for the underlying error.

This error is only ever returned back from the transport module. So any user must already know it's a transport error. No?

[LucioFranco]

Well, I guess you can use the type system to check what error this. looks like this is what hyper does for debug https://github.com/hyperium/hyper/blob/master/src/error.rs#L322

and it seems it does what you did here for display too.

[faern]

I'm really not a fan of how hyper designed their errors :)

[LucioFranco]

Can you explain why? I personally have not had an issue with it but not sure what others have experienced either :)

[faern]

No you can't really use the typesystem for anything with any of these opaque errors. All their Kind/ErrorKind enums are private :(

[faern]

The only thing they ever allow you to do currently is to print them. And even then they don't look good because they have a ton of duplicated info in each layer.

[LucioFranco]

You should be able to downcast them https://docs.rs/hyper/0.13.1/hyper/error/struct.Error.html#method.into_cause for std errors and h2 but beyond that yeah its a bit tough, the problem is adding more error types becomes a breaking change. Now that we have the non exhaustive it should be better but I guess rusts error handling is still a bit in flux. Do you have an error type that you like as a good example?

[faern]

I think we have currently, with this PR, made it impossible to downcast the error to an underlying hyper error. Our new tonic error can't be downcasted because it's a newtype wrapping the hyper error. And calling source() on it will not yield the hyper error but rather the source of it.

That being said, I have personally never downcasted an error nor felt the need to do so.

No I don't have an example of a well written error type. I do know of some concrete pain points in, what I consider, bad errors. But I don't have a manual for nice ones. If I had I would contribute it to the official API guidelines, a document I really think is lacking in the error area.

If you make it possible to destructure the error, it's probably correct to break the API when new errors are added? Because if you have such errors you tell the user what can happen and encourage them to handle the different error cases. Which means your API indeed changes if you add a new error. Users should then get compilation errors because there will be new types of errors they are expected to handle.

[LucioFranco]

This looks great! Thank you! Looks like you just need to run cargo fmt and CI will be happy :)

[LucioFranco]

Thanks!