Use Grpc Request Timeout

[tiagolobocastro]

Feature Request

Motivation

I've been doing some timeout testing with tonic and found that when both the channel timeout (https://docs.rs/tonic/0.8.0/tonic/transport/channel/struct.Endpoint.html#method.timeout) and request timeout are set, the smaller value wins: https://github.com/hyperium/tonic/blob/0b03b30cccc67d517b05587614405d63d942b1bb/tonic/src/transport/service/grpc_timeout.rs#L49

In my usecase I'd like the per-request timeout to take precedence over the preconfigured one.

Proposal

Would it be acceptable to be able to configure which timeouts takes precedence via a configuration option?

Alternatives

1. Change the current behaviour: Is the current behaviour of "smaller values wins" right? AFAIK the grpc-timeout value in the request is used to tell the server what the client deadline/timeout is. But if we then timeout "ahead of time" we're kinda then lying to the server by telling it our deadline is x when in fact is y, y<x?
2. I could just write my own timeout layer, but would be nice to use a general solution

[LucioFranco]

In my usecase I'd like the per-request timeout to take precedence over the preconfigured one.

Do you know the behavior of other grpc clients? I think it would be good to match what they do by default.

[washanhanzi]

I think what grpc/grpc-go does is 1. the client request timeout will definitely trigger, 2. the developer can decide to timeout early on the server side. Although grpc/grpc-go doesn't provide a way to set pre-request server timeout, it's the developer's duty to deal with that. Maybe we can provide a way to let the developer decide what to do when client and server timeouts are both sets, but as for now, I think it's fine to let the smaller wins.

[LucioFranco]

Yeah, I think in this case, we should just let the smaller value win. In the future, the transport code will get rewritten and we can make this feature optional and easily customizable to user needs.

[tiagolobocastro]

The thing that "concerns" me with that is that we're not changing the "server-side" timeout (the grpc-timeout in the packet) - so this means the server has wrong expectations on timeouts, which I think is very confusing.

Also it's not a simple logic of 1 overriding the other, it's the "smaller one wins", which depends on the set timeouts, but that should teach me to read code rather than make assumptions based on a single experiment :)

In the future, the transport code will get rewritten and we can make this feature optional and easily customizable to user needs.

:+1:

[DavidAntliff]

I think there are enough use-cases for "shortest wins" and "longest wins" that it's going to be restrictive to pick one over the other in some cases.

In my case, I have many gRPC requests that only need a 10 second timeout, as they are expected to complete fast, and I want the client to time out reasonably quickly so that the user knows there is an issue. But I have one request that initiates significant work on the server, which takes longer than 10 seconds. Here I'd like the timeout to be, say, 60 seconds, but the 10 second timeout on the Endpoint "wins".

With the current API, I only really have two options:

1. Make the default timeout 60 seconds and call set_timeout(10) on every other request. There are hundreds, though, and it's easy to forget to do this on new RPCs,
2. Get the existing channel, convert to a Uri (or cache the original Uri), create a new endpoint, create a new connection with a 60 second timeout, and use this just for this RPC. This can be constructed lazily for efficiency.

Perhaps if there was a way to reconfigure the Channel's timeout, temporarily, I could do this and then restore it to the default value after. But as far as I can tell, there is no way to access the private inner: tonic::client::Grpc<T> within the auto-gen'd client struct.

I'm now looking at modifying my API to send a request to start this work, and then use a server-streaming RPC to send back "progress" messages within the default timeout period. This pushes the problem, and the complexity, to the server handler, unfortunately.

EDIT: I found a solution using an interceptor, that checks to see whether an explicit timeout has been set by an RPC request, and if not applies the default timeout:

#[derive(Clone)]
struct DefaultGrpcTimeout {
    default: Duration,
}

impl DefaultGrpcTimeout {
    fn new(default: Duration) -> Self {
        Self { default }
    }
}

impl Interceptor for DefaultGrpcTimeout {
    fn call(&mut self, mut req: Request<()>) -> Result<Request<()>, Status> {
        if req.metadata().get("grpc-timeout").is_none() {
            req.set_timeout(self.default);
        }
        Ok(req)
    }
}

Then the connection is made with:

    let endpoint = Endpoint::new(dst)?;
    //  DO NOT CALL .connect(timeout) !!!
    //  but .connect_timeout(timeout) is OK

    let timeout_interceptor = DefaultGrpcTimeout::new(timeout);
    let client = MyClient::with_interceptor(channel, timeout_interceptor);

For most requests, no explicit timeout is set so the interceptor sets the default timeout, and a request can set a longer duration timeout with:

    let mut req = request.into_request();
    req.set_timeout(timeout);