add h2::Error as a `source` for tonic::Status when converting from h2::Error #612

tdyas · 2021-04-24T04:32:59Z

Motivation

A gRPC server may send a HTTP/2 GOAWAY frame with NO_ERROR status to gracefully shutdown a connection. This appears to Tonic users as a tonic::Status with Code::Internal and the message set to h2 protocol error: protocol error: not a result of an error.

The only way to currently detect this case and differentiate it from other internal errors (e.g., an application-level internal error) is to match on the message. A client may want to differentiate these cases because it may only want to alert on the application-level internal error and not on the transient transport-level issue. (Indeed, this is the use case for which I'm envisioning using this change.)

Matching on a message is not as robust, however, as matching on an h2::Error and its reason code. (The message could change for example if a future version of Tonic decided to vary the message. This would break any users that matched on the previous version of the message.)

Solution

Store the h2::Error used when creating a tonic::Status from a h2::Error and provide it as the source for purposes of std::error::Error. This will allow users to downcast it and match on the original h2::Reason.

Note: tonic::Status must now manually implement Clone becauseh2::Error cannot implement Clone since one of the Kind variants stores a std::io::Error (which does not implement Clone). Since we know that h2_error will always have a reason since we only store it in that case, this manual implementation of Clone just extracts the reason and creates a new h2::Error from it to "clone" it.

tdyas · 2021-04-24T19:41:22Z

This could also be simplified to just have Status carry a h2_reason field and not support any of the std::error::Error::source functionality. (There would be a h2_reason method returning an Option<h2::Reason>).

davidpdrsn

Nice! I think this makes sense 😊 I had a couple of suggestions. Just minor things.

This could also be simplified to just have Status carry a h2_reason field and not support any of the std::error::Error::source functionality. (There would be a h2_reason method returning an Optionh2::Reason).

I actually prefer the implementation in this PR as that we wont need special methods for each new kind of error source we might add in the future.

tonic/src/status.rs

davidpdrsn

I think this looks good but should hear from @LucioFranco as well.

tonic/src/status.rs

davidpdrsn · 2021-05-18T15:00:36Z

@tdyas do you wanna drive this home? We would like to include the change to Status in the upcoming 0.5 release.

tdyas · 2021-05-19T04:20:43Z

I modified the PR to add a generic source field, although my Rust knowledge is not as advanced so not sure what the right invocation to use to fix this error:

error[E0277]: the trait bound `Box<dyn std::error::Error + Sync + std::marker::Send>: Borrow<dyn std::error::Error>` is not satisfied
   --> tonic/src/status.rs:633:35
    |
633 |         self.source.map(|err| err.borrow())
    |                                   ^^^^^^ the trait `Borrow<dyn std::error::Error>` is not implemented for `Box<dyn std::error::Error + Sync + std::marker::Send>`
    |
    = help: the following implementations were found:
              <Box<T, A> as Borrow<T>>

davidpdrsn · 2021-05-19T07:57:44Z

This seems to do the trick

impl Error for Status {
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        self.source.as_ref().map(|err| (&**err) as _)
    }
}

as_ref is needed because Option::map consumes self and you cannot move source out of a &self
Then map receives as &Box<dyn Error + Send + Sync + 'static>. The *s first derefs the &Box<dyn Error + Send + Sync + 'static> into a Box<dyn Error + Send + Sync + 'static> and the second derefs the box into dyn Error + Send + Sync + 'static which you need to put behind a reference with & since its unsized.
Finally the as _ casts the dyn Error + Send + Sync + 'static into a dyn Error + 'static which rust apparently can't do on its own.

Phew 😨

Removing Clone on Status causes a test in prost.rs to fail but that can be fixed with

let messages = std::iter::repeat_with(move || Ok::<_, Status>(msg.clone())).take(10000);

davidpdrsn

Changing my review "comment" since I wanna give it final look when things are compiling.

Edit: Which apparently github doesn't let you do... How do you go from "approved" to "not approved yet" without "request changes" 🤷

tdyas · 2021-05-19T08:07:34Z

Edit: Which apparently github doesn't let you do... How do you go from "approved" to "not approved yet" without "request changes" 🤷

Not sure but I can hit the re-request review button to reset that state (which I just did).

davidpdrsn

I think it looks good! I had one question and an integration test would be cool as well.

tonic/src/status.rs

davidpdrsn · 2021-05-19T08:26:35Z

tonic/src/status.rs

+            .source()
+            .and_then(|err| err.downcast_ref::<h2::Error>())
+            .unwrap();
+        assert_eq!(orig.reason(), source.reason());


I'm thinking it would also be nice to have a high level integration test kinda thing that uses the APIs users would. Not sure how to write such a test though.

The h2 crate has test helpers for low level testing of HTTP/2 streams. Maybe this integration test would be similar to the tests in https://github.com/hyperium/h2/tree/master/tests/h2-tests?

Any thoughts on whether an h2-level integration test would be the way to go?

I don't think we need those low level helpers, we should be able to trigger most things from hyper/h2 directly, no?

LucioFranco · 2021-06-21T16:42:29Z

@tdyas @davidpdrsn any updates on this?

tdyas · 2021-06-22T13:37:36Z

I haven't had a chance to work on an integration test for this due to my work schedule. I see there is now a merge conflict, in trying to fix that, I am having a compile error in tonic/src/transport/server/recover_error.rs due to err's ownership passing into Status::try_from_error and not being available in the else branch:

                if let Some(status) = Status::try_from_error(&*err) {
                    let mut res = Response::new(MaybeEmptyBody::empty());
                    status.add_header(res.headers_mut()).unwrap();
                    Poll::Ready(Ok(res))
                } else {
                    Poll::Ready(Err(err))
                }

~~I will need to adjust the Status::try_from_error signature to pass the err back out so the caller can recover it if the conversion fails.~~ Never mind, it already does this.

tdyas · 2021-06-22T14:05:18Z

I rebased the PR and now it fails with:

---- picks_client_timeout_if_thats_sorter stdout ----
thread 'picks_client_timeout_if_thats_sorter' panicked at 'assertion failed: err.message().contains(\"Timeout expired\")', tests/integration_tests/tests/timeout.rs:60:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- cancelation_on_timeout stdout ----
thread 'cancelation_on_timeout' panicked at 'assertion failed: err.message().contains(\"Timeout expired\")', tests/integration_tests/tests/timeout.rs:22:5

Are those tests relying on TimeoutExpired being preferred?

LucioFranco · 2021-06-22T14:28:28Z

@davidpdrsn ^

tdyas · 2021-06-22T15:06:48Z

Also, I had to add Send + Sync bounds to the Error passed into Status::try_from_error to get it compiling. Not sure how to have Rust "pass through" Send+Sync if present onto the result type of a method.

davidpdrsn · 2021-06-22T17:47:06Z

Are those tests relying on TimeoutExpired being preferred?

Yes we should still downcast that error correctly. I did some digging and this fixes it:

diff --git a/tonic/src/status.rs b/tonic/src/status.rs
index ef9731a..927d95b 100644
--- a/tonic/src/status.rs
+++ b/tonic/src/status.rs
@@ -320,12 +320,6 @@ impl Status {
             Err(err) => err,
         };
 
-        #[cfg(feature = "transport")]
-        let err = match err.downcast::<crate::transport::TimeoutExpired>() {
-            Ok(timeout) => return Ok(Status::cancelled(timeout.to_string())),
-            Err(err) => err,
-        };
-
         #[cfg(feature = "transport")]
         let err = match err.downcast::<h2::Error>() {
             Ok(h2) => {
@@ -555,6 +549,11 @@ fn find_status_in_source_chain(err: &(dyn Error + 'static)) -> Option<Status> {
             });
         }
 
+        #[cfg(feature = "transport")]
+        if let Some(timeout) = err.downcast_ref::<crate::transport::TimeoutExpired>() {
+            return Some(Status::cancelled(timeout.to_string()));
+        }
+
         source = err.source();
     }

Also, I had to add Send + Sync bounds to the Error passed into Status::try_from_error to get it compiling. Not sure how to have Rust "pass through" Send+Sync if present onto the result type of a method.

Nah I don't think there is another way. Its fine 👌

tdyas · 2021-06-22T17:56:49Z

Pushed @davidpdrsn's fix for the timeout error decoding.

davidpdrsn

@tdyas Thank you for your excellent work on this! I only had one minor comment/question. So close! 😃

davidpdrsn · 2021-06-22T18:06:39Z

tonic/src/status.rs

+impl TryFrom<Box<dyn Error + Send + Sync + 'static>> for Status {
+    type Error = Box<dyn Error + Send + Sync + 'static>;
+
+    fn try_from(err: Box<dyn Error + Send + Sync + 'static>) -> Result<Self, Self::Error> {
+        Status::try_from_error(err)
+    }
+}
+


Is there a specific use-case this impl enables, or is mainly for convenience? I dunno, feels a bit out of place for me.

I don't recall, and I can't find a relevant use of this in the main code bases in which I work. Shall I remove it?

Yeah I think we should just remove it.

davidpdrsn

LGTM!

LucioFranco

LGTM, just need to get CI working then we can merge! Thanks for pushing this across the finish line!

LucioFranco · 2021-06-22T18:35:42Z

tonic/src/status.rs

@@ -356,7 +355,13 @@ impl Status {
            _ => Code::Unknown,
        };

-        Status::new(code, format!("h2 protocol error: {}", err))
+        let mut status = Self::new(code, format!("h2 protocol error: {}", err));
+        let error: Option<Box<dyn Error + Send + Sync + 'static>> = err


why do you need this extra type hint?

I don't recall. I removed it and it still compiles so pushed the removal of it.

LucioFranco · 2021-06-22T18:36:29Z

tonic/src/status.rs

@@ -3,6 +3,7 @@ use crate::metadata::MetadataMap;
 use bytes::Bytes;
 use http::header::{HeaderMap, HeaderValue};
 use percent_encoding::{percent_decode, percent_encode, AsciiSet, CONTROLS};
+use std::convert::TryFrom;


Looks like this is failing CI?

It seems like deny(warnings) is only set in CI? When I run cargo clippy, it only warned for this and some other issues. It would probably be useful to make the crates enable #![deny(warnings)] (but in a separate PR obviously).

riking · 2021-07-07T23:26:32Z

GOAWAY frame with NO_ERROR status to gracefully shutdown a connection. This appears to Tonic users as a tonic::Status with Code::Internal and the message set to h2 protocol error: protocol error: not a result of an error.

I believe the official GRPC-on-h2 spec says that this should be mapped by the library to a UNAVAILABLE code; the user of the library shouldn't have to deal with recognizing it. https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#goaway-frame

davidpdrsn reviewed May 1, 2021

View reviewed changes

tonic/src/status.rs Outdated Show resolved Hide resolved

tonic/src/status.rs Outdated Show resolved Hide resolved

davidpdrsn mentioned this pull request May 1, 2021

Suggestion: Allow users to handle transport errors differently from app errors #587

Closed

davidpdrsn requested a review from LucioFranco May 4, 2021 11:54

davidpdrsn approved these changes May 4, 2021

View reviewed changes

davidpdrsn mentioned this pull request May 7, 2021

impl Display for transport::Error includes the source #632

Closed

LucioFranco requested changes May 7, 2021

View reviewed changes

tonic/src/status.rs Outdated Show resolved Hide resolved

davidpdrsn added the breaking change label May 11, 2021

davidpdrsn added this to the 0.5 milestone May 11, 2021

davidpdrsn mentioned this pull request May 12, 2021

Enable mapping h2 errors to correct Status codes. #394

Closed

davidpdrsn reviewed May 19, 2021

View reviewed changes

tdyas requested a review from davidpdrsn May 19, 2021 08:06

davidpdrsn requested changes May 19, 2021

View reviewed changes

Tom Dyas added 9 commits June 22, 2021 07:21

add h2::Error as a source for tonic::Status

331b455

only define h2_error when transport feature enabled

ab31a94

suggested changes

14ca795

switch to generic source field

0a0a0e9

further work

051650d

compile fixes

a9d0a49

checkpoint

5b1fc1c

switch Status::try_from_error to take an owned (and boxed) error value

a5739a9

rename method s/cause/source/ and make private

8edb9ac

fix merge conflicts

92aaafe

tdyas force-pushed the add_error_source branch from cdc139b to 92aaafe Compare June 22, 2021 13:51

fix decoding of timeout error

cdbc5d7

davidpdrsn reviewed Jun 22, 2021

View reviewed changes

remove TryFrom impl

ac8aad5

davidpdrsn approved these changes Jun 22, 2021

View reviewed changes

LucioFranco approved these changes Jun 22, 2021

View reviewed changes

Tom Dyas added 2 commits June 22, 2021 14:32

fix CI

c10da2c

remove type ascription

368fadd

davidpdrsn merged commit b90bb7b into hyperium:master Jun 23, 2021

riking mentioned this pull request Jul 7, 2021

fix(transport): Correctly map hyper errors #629

Merged

roblabla mentioned this pull request Jan 4, 2022

fix(tonic): Expose h2 error instead of reason #883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add h2::Error as a `source` for tonic::Status when converting from h2::Error #612

add h2::Error as a `source` for tonic::Status when converting from h2::Error #612

tdyas commented Apr 24, 2021 •

edited by davidpdrsn

Loading

tdyas commented Apr 24, 2021

davidpdrsn left a comment

davidpdrsn left a comment

davidpdrsn commented May 18, 2021

tdyas commented May 19, 2021

davidpdrsn commented May 19, 2021 •

edited

Loading

davidpdrsn left a comment •

edited

Loading

tdyas commented May 19, 2021

davidpdrsn left a comment

davidpdrsn May 19, 2021

tdyas May 21, 2021

tdyas Jun 8, 2021

LucioFranco Jun 11, 2021

LucioFranco commented Jun 21, 2021

tdyas commented Jun 22, 2021 •

edited

Loading

tdyas commented Jun 22, 2021

LucioFranco commented Jun 22, 2021

tdyas commented Jun 22, 2021

davidpdrsn commented Jun 22, 2021

tdyas commented Jun 22, 2021

davidpdrsn left a comment

davidpdrsn Jun 22, 2021

tdyas Jun 22, 2021

davidpdrsn Jun 22, 2021

davidpdrsn left a comment

LucioFranco left a comment

LucioFranco Jun 22, 2021

tdyas Jun 22, 2021

LucioFranco Jun 22, 2021

tdyas Jun 22, 2021

tdyas Jun 22, 2021

tdyas Jun 22, 2021

riking commented Jul 7, 2021

add h2::Error as a source for tonic::Status when converting from h2::Error #612

add h2::Error as a source for tonic::Status when converting from h2::Error #612

Conversation

tdyas commented Apr 24, 2021 • edited by davidpdrsn Loading

Motivation

Solution

tdyas commented Apr 24, 2021

davidpdrsn left a comment

Choose a reason for hiding this comment

davidpdrsn left a comment

Choose a reason for hiding this comment

davidpdrsn commented May 18, 2021

tdyas commented May 19, 2021

davidpdrsn commented May 19, 2021 • edited Loading

davidpdrsn left a comment • edited Loading

Choose a reason for hiding this comment

tdyas commented May 19, 2021

davidpdrsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco commented Jun 21, 2021

tdyas commented Jun 22, 2021 • edited Loading

tdyas commented Jun 22, 2021

LucioFranco commented Jun 22, 2021

tdyas commented Jun 22, 2021

davidpdrsn commented Jun 22, 2021

tdyas commented Jun 22, 2021

davidpdrsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidpdrsn left a comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riking commented Jul 7, 2021

add h2::Error as a `source` for tonic::Status when converting from h2::Error #612

add h2::Error as a `source` for tonic::Status when converting from h2::Error #612

tdyas commented Apr 24, 2021 •

edited by davidpdrsn

Loading

davidpdrsn commented May 19, 2021 •

edited

Loading

davidpdrsn left a comment •

edited

Loading

tdyas commented Jun 22, 2021 •

edited

Loading