grpc / grpc/grpc-rust

Deadline propagation

Open
#2,316 5 comments 0 reactions 1 assignee Claimed by @LucioFranco View on GitHub
I-needs-decision P-high
Dominant language
Rust
Stars
12.5k
Forks
1.3k
Avg merge
4d 7h
Merged PRs (30d)
24

Description

One important feature of gRPC is automatic (or hard-to-forget) propagation of deadlines between incoming calls and outgoing calls. All core gRPC languages have something in place to satisfy this requirement. As an example, in Go, a [stdlib context](https://pkg.go.dev/context) with a timeout is provided to the server's method handler, which is then passed as a required parameter when making any outgoing calls.

Tonic's API, while ergonomic for basic use cases, unfortunately makes it easy to forget to set a deadline. This happens for two reasons:

1. The `Request::new` constructor doesn't require or even accept a deadline. It must be set as a separate operation.
1. In the proto generated code, RPC operations accept an [`IntoRequest`](https://docs.rs/tonic/latest/tonic/trait.IntoRequest.html) (or `IntoStreamingRequest`) which are blanked implemented for any message type. This allows calls to accept messages directly, so even if the `Request::new` constructor accepted a deadline, it could be missed because of this shortcut.

I would recommend something like the following:

1. `Request::new` is replaced by two new constructors:
- One constructor accepts a `tonic::Request<_>` from which it will retrieve the deadline and propagate it to the new request.
- One constructor is named in a way to indicate that the outgoing request is not done in service of an incoming request. This should potentially include a `deadline: Option` or `timeout: Option` parameter. (Aside: we should make sure naming is consistent everywhere: a "deadline" is a point in time, and a "timeout" is a duration in the future.)
1. `IntoRequest` is changed back to `Request` in the proto generated code.

This would be a breaking change that is likely to be unpopular with some (many?) users. The latter change might justify maintaining a new major version of codegen unless we are willing to break users for this change. Looking at the tonic repo, however, I don't see any examples that take advantage of the `IntoRequest` functionality, even though most could do so -- they all manually call `Request::new()`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.