search

hyperium / tonic

A native gRPC client & server implementation with async/await support.
https://docs.rs/tonic
MIT License
10.02k stars 1.02k forks source link

Use Grpc Request Timeout #1105

Open tiagolobocastro opened 2 years ago

tiagolobocastro commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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: