-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix(transport): Improve Error
type
#217
Merged
+15
−58
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,57 +1,23 @@ | ||
use std::{error, fmt}; | ||
|
||
/// Error's that originate from the client or server; | ||
pub struct Error { | ||
kind: ErrorKind, | ||
source: Option<crate::Error>, | ||
} | ||
|
||
impl Error { | ||
pub(crate) fn from_source(kind: ErrorKind, source: crate::Error) -> Self { | ||
Self { | ||
kind, | ||
source: Some(source), | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug)] | ||
pub(crate) enum ErrorKind { | ||
Client, | ||
Server, | ||
} | ||
pub struct Error(crate::Error); | ||
|
||
impl fmt::Debug for Error { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
let mut f = f.debug_tuple("Error"); | ||
f.field(&self.kind); | ||
if let Some(source) = &self.source { | ||
f.field(source); | ||
} | ||
f.finish() | ||
impl Error { | ||
pub(crate) fn from_source(source: impl Into<crate::Error>) -> Self { | ||
Self(source.into()) | ||
} | ||
} | ||
|
||
impl fmt::Display for Error { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
if let Some(source) = &self.source { | ||
write!(f, "{}: {}", self.kind, source) | ||
} else { | ||
write!(f, "{}", self.kind) | ||
} | ||
self.0.fmt(f) | ||
} | ||
} | ||
|
||
impl error::Error for Error { | ||
fn source(&self) -> Option<&(dyn error::Error + 'static)> { | ||
self.source | ||
.as_ref() | ||
.map(|e| &**e as &(dyn error::Error + 'static)) | ||
} | ||
} | ||
|
||
impl fmt::Display for ErrorKind { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
write!(f, "{:?}", self) | ||
self.0.source() | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not a fan of how
hyper
designed their errors :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why? I personally have not had an issue with it but not sure what others have experienced either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No you can't really use the typesystem for anything with any of these opaque errors. All their
Kind
/ErrorKind
enums are private :(There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.