tonic: `Reconnect::poll_ready` returns `Ok(())` on connection error, breaking tower's p2c `Balance` failover
- Dominant language
- Rust
- Stars
- 12.5k
- Forks
- 1.3k
- Avg merge
- 4d 7h
- Merged PRs (30d)
- 24
Description
[PR #458 — "fix(transport): reconnect lazy connections after first failure"](https://github.com/grpc/grpc-rust/pull/458) changed `tonic`'s internal `Reconnect` service so, if lazy, when it finds a connection error it returns `Poll::Ready(Ok(()))` in `poll_ready`, deferring returning the error to `call`. Tower's p2c `Balance`, which sits directly on top of these services in `Channel::balance`, relies on `poll_ready` returning `Err` to detect a broken endpoint and route around it. Because `Reconnect::poll_ready` never does this for lazy/reconnecting endpoints, `Balance` can select a broken endpoint as "ready" and dispatch a request to it via `Service::call`, which then fails and forces the caller to retry the RPC instead of tower transparently failing over to a healthy endpoint.
This is affecting services that have a retry budget, since such budget is consumed by connection errors form request errors and are forced to consume such budget in connection errors.
## Root cause / affected code
### Tonic's `src/transport/channel/service/reconnect.rs`
`Reconnect` tracks a one-shot error field and lazy/has-been-connected flags:
```rust
// lines 36-47
pub(crate) struct Reconnect
where
M: Service,
M::Error: Into,
{
mk_service: M,
state: State,
target: Target,
error: Option,
has_been_connected: bool,
is_lazy: bool,
}
```
`poll_ready` has two places where it returns `Ok(())` despite a connection error:
```rust
// lines 86-91
fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> {
let mut state;
if self.error.is_some() {
return Poll::Ready(Ok(()));
}
```
If a previous poll already recorded an error, any *subsequent* `poll_ready` call short-circuits to `Ok(())` — the caller (Buffer worker / balancer) is told the service is ready, even though it is permanently broken until the next `call` happens to consume the stored error.
```rust
// lines 109-133
State::Connecting(ref mut f) => {
trace!("poll_ready; connecting");
match Pin::new(f).poll(cx) {
Poll::Ready(Ok(service)) => {
state = State::Connected(service);
}
Poll::Pending => {
trace!("poll_ready; not ready");
return Poll::Pending;
}
Poll::Ready(Err(e)) => {
trace!("poll_ready; error");
state = State::Idle;
if !(self.has_been_connected || self.is_lazy) {
return Poll::Ready(Err(e.into()));
} else {
let error = e.into();
tracing::debug!("reconnect::poll_ready: {:?}", error);
self.error = Some(error);
break;
}
}
}
}
```
```rust
// lines 156-161
self.state = state;
}
self.state = state;
Poll::Ready(Ok(()))
}
```
When a connect attempt fails and `has_been_connected || is_lazy` is true, the error is stashed in `self.error`, the loop `break`s, and execution falls through to line 160-161, which still returns `Poll::Ready(Ok(()))`. Only the non-lazy and never-connected case (line 124-125) returns `Err` immediately from `poll_ready`.
### Towers p2c balancer
Tonic uses [tower's p2c balancer](https://github.com/tower-rs/tower/blob/master/tower/src/balance/p2c/service.rs) which considers a service to be ready by calling `poll_ready` on it (see [here](https://github.com/tower-rs/tower/blob/df06d70dbea345facbffb5881fe8647f53bf424d/tower/src/balance/p2c/service.rs#L215)). An endpoint being ready if its service returns `Poll::Ready(Ok())` (see [here](https://github.com/tower-rs/tower/blob/df06d70dbea345facbffb5881fe8647f53bf424d/tower/src/ready_cache/cache.rs#L338)).
In tonic, that inner service is `Connection` which as said before, if lazy, on connection error will return `Poll::Ready(Ok())` and tower's balancer won't try to pick a different one.
Contributor guide
Research direction
Start with tonic/src/transport/channel/service/reconnect.rs, especially Reconnect::poll_ready and its stored error, then compare the readiness expectations in tower's p2c balance and ready-cache references linked in the issue. Done means a failed lazy or previously connected endpoint is not treated as ready by Balance, allowing a healthy endpoint to handle the request without an unnecessary caller retry.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design, networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 74/100