improper error handling with from h2 reason::cancel -> tonic::code::unkown

[lyang24]

Bug Report

Version: latest

Platform: all (linux)

Crates

http_body, hyper, h2

Description

maybe Streaming::message need to handle h2 error?

tonic/tonic/src/codec/decode.rs

Line 151 in c62f382

pub async fn message(&mut self) -> Result<Option<T>, Status> {

called from here

tonic/tonic/src/client/grpc.rs

Line 193 in c62f382

if let Some(trailers) = body.trailers().await? {

I expected to see this happen: Code::Cancelled, message: stream no longer needed

Instead, this happened: code: Unknown, message: "error reading a body from connection: stream error received: stream no longer needed

Todo add minimal reproduce example

[davidpdrsn]

Are you able to share a minimal reproducible example?

[lyang24]

Are you able to share a minimal reproducible example?

not yet sorry i am not that skilled in rust. but if h2 will throws an error with reason::cancel and that is propagated by hyper's poll_inner fn at hyper/src/body/body.rs the downcast here will not catch it right? then it will be code::unknown mixed with hyper's message instead of code::cancelled which is confusing

[davidpdrsn]

Hyper errors have h2 errors as the "source" which should get picked up by tonic. At least I remember doing a bunch of testing around that last time we worked on it.

[lyang24]

Hyper errors have h2 errors as the "source" which should get picked up by tonic. At least I remember doing a bunch of testing around that last time we worked on it.

maybe its a version issue let me double check, close the report for now

[lyang24]

@davidpdrsn yep you are right hyper have h2 error as source. i have a quick question should we handle h2 error here since we are polling from the stream? i guess this is where the code known come from? will google how to repo bug with custom dependencies tmr. its way to late today.

tonic/tonic/src/codec/decode.rs

Line 151 in c62f382

pub async fn message(&mut self) -> Result<Option<T>, Status> {

[lyang24]

updated desc reopen and will work towards an example tmr

[inevity]

Though we have get fn from_h2_error(err: &h2::Error), but this is not exhaust.
When set timeout 1us by tower service-builder.new().settimeout() layer then server build the tonic. The grpc server will timeout some req.
h2 log this:
http2 service errored: error from user's Service: request timed out,
and tonic rpc will get response : code: Unknow, message: transport error.

[LucioFranco]

In this case you will need to downcast to the timeout error since you added that layer it looks like and tonic doesn't directly map those timeout errors on the server side iirc

[LucioFranco]

@lyang24 did you ever get a chance to get a repro?

[behos]

Hi @LucioFranco , here's a rough implementation reproducing the error handling

https://github.com/behos/tonic-bidi-error

[LucioFranco]

Ok I have a fix here for it #1315 feel free to review it.