Support timeouts with "grpc-timeout" header #516
Conversation
…ing grpc-timeout header, and wrapping request in a timeout
| "te", | ||
| "user-agent", | ||
| "content-type", | ||
| "grpc-timeout", |
There was a problem hiding this comment.
removing this definitely isn't right, but otherwise the header was getting stripped when I tried to set it in the integration test. Would love some guidance on how to set this header from a test
There was a problem hiding this comment.
I think its fine. As the user is now allowed to set it I guess its not reserved anymore.
| /// should never happen in practice because the `grpc-timeout` header should be only | ||
| /// ASCII characters. | ||
| fn try_split_last(val: &str) -> Result<(&str, &str), &str> { | ||
| std::panic::catch_unwind(|| val.split_at(val.len() - 1)).map_err(|_| val) |
There was a problem hiding this comment.
I realized, after writing a unit test, that HeaderValue already prevents non-ASCII characters 🎉 so this extra catch_unwind(|| ...) probably isn't necessary, but will leave it in for now
There was a problem hiding this comment.
I guess str.split_at(...) should be fine since HeaderValue::to_str checks for ASCII as you mention.
|
@LucioFranco Anything I could do to move along at least the client-side support for setting the timeout header? I'm planning on releasing a crate soon that requires it, and it would be great to not need to I'm guessing you want to do something a little more formal than just removing it from reserved headers. If you let me know what that might be I would be happy to make a PR. |
There was a problem hiding this comment.
@ParkMyCar Thanks for working on this! I've left a couple of comments but overall I think the approach looks good 👍
Are you still up for driving this home? Getting this up to date with master might require some work as tonic has updated to tokio 1.x.
I do think having to create a new middleware not being able to use tower::timeout::Timeout is unfortunate but I don't see a nice way around it that doesn't involve creating new middlewares regardless.
| let err = client.unary_call(req).await.unwrap_err(); | ||
| assert!(err.message().contains("Timeout expired")); | ||
| // TODO: Need to return the correct type of code | ||
| // assert_eq!(err.code(), Code::Cancelled); |
There was a problem hiding this comment.
This seems like something that should be fixed before merging 🤔
| "te", | ||
| "user-agent", | ||
| "content-type", | ||
| "grpc-timeout", |
There was a problem hiding this comment.
I think its fine. As the user is now allowed to set it I guess its not reserved anymore.
| fn call(&mut self, req: Request<ReqBody>) -> Self::Future { | ||
| // Try to parse the `grpc-timeout` header, if it is present | ||
| let header_timeout = headers::try_parse_grpc_timeout(req.headers()).unwrap_or_else(|e| { | ||
| warn!("Error parsing grpc-timeout header {:?}", e); |
There was a problem hiding this comment.
nit: not sure warn is the best fit here since clients might be sending garbage data. It isn't really tonics fault, modulo parsing bugs of course 😊 I kinda think we should use trace or drop the log entirely.
| (None, Some(dur)) => Some(dur), | ||
| (Some(header), Some(server)) => { | ||
| let shorter_duration = std::cmp::min(header, server); | ||
| debug!( |
There was a problem hiding this comment.
I'm thinking this should just be trace.
| debug!( | |
| trace!( |
| /// should never happen in practice because the `grpc-timeout` header should be only | ||
| /// ASCII characters. | ||
| fn try_split_last(val: &str) -> Result<(&str, &str), &str> { | ||
| std::panic::catch_unwind(|| val.split_at(val.len() - 1)).map_err(|_| val) |
There was a problem hiding this comment.
I guess str.split_at(...) should be fine since HeaderValue::to_str checks for ASCII as you mention.
|
I'm going to be driving this home in a separate PR. |
|
Superseded by #606 |
Motivation
This PR fixes issue #75, which is to implement support for the "grpc-timeout" header in tonic. Setting a timeout/deadline is an official part of the gRPC spec, and is a recommended practice.
Solution
Server
This PR creates a new
Timeoutstruct that implementstower::Serviceand is added as alayer_fn(...)in aConnection'stower::ServiceBuilder. TheTimeoutservice then parses thegrpc-timeoutheader, and wraps an inner future in a timeout.I'm relying on
tokio::time::Delayfor the actual timeout logic (Delayseems to be 1:1 withSleepin tokio 1.0, which should make migration easy). I realize this prevents theTimeoutservice from being runtime agnostic, but since the service lives within thetransportmodule, which already depends on tokio, I thought this was okay. If we don't want to rely ontokio::time::Delay, we can hand write our own Delay-like future, with a downside being, we won't be able to use mock timers for testing.A
tonic::transport::Serveralready supports a default timeout, I handle the presence of a server timeout, and agrpc-timeoutheader, by using which ever duration is the shortest, essentially the server timeout becomes a ceiling. The reasoning here is I didn't want a client to be able to override a server's settings, but if a client wants to timeout faster, the server shouldn't care about that.Client
This PR doesn't (yet) make any changes to gRPC clients. It could, similar to what the gRPC docs have as examples for other libraries, but I thought the server/client impls would be a good separation for different PRs.
Note
I had previously opened #362 a few months ago which tries to add the same feature, supporting timeouts with the "grpc-timeout" header. I'm opening a new PR now because I had deleted my previous fork of tonic that was used to open that previous PR. I hope this is okay 🙂