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

fix(transport): Improve Error type #217

Merged
merged 1 commit into from
Jan 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions tonic/src/transport/channel/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::Channel;
use super::ClientTlsConfig;
#[cfg(feature = "tls")]
use crate::transport::service::TlsConnector;
use crate::transport::{Error, ErrorKind};
use crate::transport::Error;
use bytes::Bytes;
use http::uri::{InvalidUri, Uri};
use std::{
Expand Down Expand Up @@ -44,9 +44,7 @@ impl Endpoint {
D: TryInto<Self>,
D::Error: Into<crate::Error>,
{
let me = dst
.try_into()
.map_err(|e| Error::from_source(ErrorKind::Client, e.into()))?;
let me = dst.try_into().map_err(|e| Error::from_source(e.into()))?;
Ok(me)
}

Expand Down
7 changes: 3 additions & 4 deletions tonic/src/transport/channel/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ impl Channel {

let svc = Connection::new(connector, endpoint)
.await
.map_err(|e| super::Error::from_source(super::ErrorKind::Client, e))?;
.map_err(|e| super::Error::from_source(e))?;

let svc = Buffer::new(Either::A(svc), buffer_size);

Expand Down Expand Up @@ -174,8 +174,7 @@ impl GrpcService<BoxBody> for Channel {
type Future = ResponseFuture;

fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
GrpcService::poll_ready(&mut self.svc, cx)
.map_err(|e| super::Error::from_source(super::ErrorKind::Client, e))
GrpcService::poll_ready(&mut self.svc, cx).map_err(|e| super::Error::from_source(e))
}

fn call(&mut self, mut request: Request<BoxBody>) -> Self::Future {
Expand All @@ -193,7 +192,7 @@ impl Future for ResponseFuture {

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let val = futures_util::ready!(Pin::new(&mut self.inner).poll(cx))
.map_err(|e| super::Error::from_source(super::ErrorKind::Client, e))?;
.map_err(|e| super::Error::from_source(e))?;
Ok(val).into()
}
}
Expand Down
46 changes: 6 additions & 40 deletions tonic/src/transport/error.rs
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)
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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 :)

Copy link
Member

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 :)

Copy link
Contributor Author

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 :(

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

}
}

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()
}
}
2 changes: 0 additions & 2 deletions tonic/src/transport/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,5 +112,3 @@ pub use self::channel::ClientTlsConfig;
#[cfg(feature = "tls")]
#[cfg_attr(docsrs, doc(cfg(feature = "tls")))]
pub use self::server::ServerTlsConfig;

pub(crate) use self::error::ErrorKind;
12 changes: 4 additions & 8 deletions tonic/src/transport/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,9 @@ impl Server {
.serve(svc)
.with_graceful_shutdown(signal)
.await
.map_err(map_err)?
.map_err(super::Error::from_source)?
} else {
server.serve(svc).await.map_err(map_err)?;
server.serve(svc).await.map_err(super::Error::from_source)?;
}

Ok(())
Expand Down Expand Up @@ -374,7 +374,7 @@ where
/// [`Server`]: struct.Server.html
pub async fn serve(self, addr: SocketAddr) -> Result<(), super::Error> {
let incoming = TcpIncoming::new(addr, self.server.tcp_nodelay, self.server.tcp_keepalive)
.map_err(map_err)?;
.map_err(super::Error::from_source)?;
self.server
.serve_with_shutdown::<_, _, future::Ready<()>, _, _>(self.routes, incoming, None)
.await
Expand All @@ -391,7 +391,7 @@ where
f: F,
) -> Result<(), super::Error> {
let incoming = TcpIncoming::new(addr, self.server.tcp_nodelay, self.server.tcp_keepalive)
.map_err(map_err)?;
.map_err(super::Error::from_source)?;
self.server
.serve_with_shutdown(self.routes, incoming, Some(f))
.await
Expand All @@ -413,10 +413,6 @@ where
}
}

fn map_err(e: impl Into<crate::Error>) -> super::Error {
super::Error::from_source(super::ErrorKind::Server, e.into())
}

impl fmt::Debug for Server {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Builder").finish()
Expand Down