feat(transport): Support timeouts with "grpc-timeout" header #606
Conversation
|
Ah no worries! Thanks for driving this home 🙂 |
|
@ParkMyCar sorry for the long delay on all of this! Thanks for the patience 😄 |
|
@LucioFranco no problem! Just glad to see this get closer to landing |
Co-authored-by: Lucio Franco <luciofranco14@gmail.com>
|
@LucioFranco ready for another round of review! |
davidpdrsn
force-pushed
the
david/grpc-timeout
branch
from
be20e9d
to
19132a2
Compare
| match result { | ||
| Ok(res) => Poll::Ready(Ok(res)), | ||
| Err(err) => { | ||
| if let Some(status) = Status::try_from_error(&*err) { |
There was a problem hiding this comment.
It would be good to document this actually since its quite unclear. What errors would be returned as a status and which ones would fail? I do think this is the right approach though.
There was a problem hiding this comment.
@seanmonstar what do you think here?
There was a problem hiding this comment.
Any error that Status::try_from_error can downcast will be converted to a response. Do you think we should mention that here or refer to the docs for Status::try_from_error (and write some docs for that method as well maybe)?
LucioFranco
changed the title
|
Oh and CI needs a fix |
|
I'll go ahead and merge this and work on Will do follow up PRs if the docs could be made clearer 😊 |
This adds support for the
grpc-timeoutheader as is recommended by the gRPC spec.It replaces use of
tower::timeout::Timeoutin the server with our ownTimeoutmiddleware. That tries to parse the timeout ingrpc-timeoutheader and picks which ever of the server or client timeouts are the shortest.Most of the implementation is based on #516. cc @ParkMyCar
Fixes #75