Align Server and Endpoint HTTP/2 keepalive setters (Option vs Duration)
- Dominant language
- Rust
- Stars
- 12.5k
- Forks
- 1.3k
- Avg merge
- 4d 7h
- Merged PRs (30d)
- 24
Description
## Feature Request
### Crates
- `tonic` (`transport`: `Server` + `Endpoint`)
### Motivation
`Server` and `Endpoint` store the same kind of values internally (`Option`), but the public setters disagree. Applying one config model to both sides currently requires two different call styles:
```rust
// Server: None disables HTTP/2 PING
Server::builder()
.tcp_keepalive(tcp) // Option
.http2_keepalive_interval(interval) // Option
.http2_keepalive_timeout(timeout) // Option
.timeout(rpc_timeout); // Duration — omit the call to leave unset
// Endpoint: HTTP/2 PING has no None; skip the method to leave hyper's default
Channel::from_static("http://127.0.0.1:50051")
.tcp_keepalive(tcp) // Option (same as Server)
.http2_keep_alive_interval(interval) // Duration — cannot pass None
.keep_alive_timeout(timeout) // Duration
.timeout(rpc_timeout); // Duration
```
This shows up whenever config uses `0` / missing = “do not set”. Server can take `Option` directly; the client must `if let Some(d) { endpoint = endpoint.http2_keep_alive_interval(d); }`.
Related inconsistency on the server setter itself: `Server::http2_keepalive_timeout` takes `Option`, but `None` is a no-op and does not disable the default 20s timeout:
```rust
pub fn http2_keepalive_timeout(mut self, http2_keepalive_timeout: Option) -> Self {
if let Some(timeout) = http2_keepalive_timeout {
self.http2_keepalive_timeout = timeout;
}
self
}
```
So even the `Option` API is not “None means off” for that method.
### Proposal
Make the enable/disable story the same on both builders, for example:
1. `Endpoint::http2_keep_alive_interval` / `keep_alive_timeout` take `Option` (`None` = leave hyper default / disable), matching `Server::http2_keepalive_interval` and `tcp_keepalive`.
2. Document `timeout` / `connect_timeout` as “call to set, omit to leave unset” (already `Duration` wrapping `Some`), or also accept `Option` if you want a single pattern everywhere.
3. Fix `Server::http2_keepalive_timeout(None)` so `None` either disables keepalive timeout or is documented as a no-op (today it looks like disable and is not).
A breaking change could wait for 0.15 with `#[deprecated]` aliases. A non-breaking path is adding `*_opt` methods or accepting `impl Into>` if that is acceptable.
### Alternatives
- Keep the current APIs and document the Server vs Endpoint difference in the transport module docs.
- Only fix `http2_keepalive_timeout(None)` no-op, leave Endpoint as `Duration`.
I can send a PR if the preferred shape is clear.
### Version
tonic 0.14.6
Contributor guide
Research direction
Start with the tonic transport Server and Endpoint keepalive setters described in the issue, comparing their current Option and Duration behavior. Resolve the intended None, default, disabling, and compatibility semantics before changing the public API. Done means one consistent, documented behavior is implemented for both builders and verified with focused tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- grpc, rust
- Domain
- api, backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 38/100