Skip to content

Commit

Permalink
Fix race condition in connection termination
Browse files Browse the repository at this point in the history
See hyperium/hyper#3652.

What I have found is the final reference to a stream being dropped
after the `maybe_close_connection_if_no_streams` but before the
`inner.poll()` completes can lead to the connection dangling forever
without any forward progress. No streams/references are alive, but the
connection is not complete and never wakes up again. This seems like a
classic TOCTOU race condition.

In this fix, I check again at the end of poll and if this state is
detected, wake up the task again.

Wth the test in hyperium/hyper#3655, on my machine, it fails about 5% of the time:
```
1876 runs so far, 100 failures (94.94% pass rate). 95.197349ms avg, 1.097347435s max, 5.398457ms min
```

With that PR, this test is 100% reliable
```
64010 runs so far, 0 failures (100.00% pass rate). 44.484057ms avg, 121.454709ms max, 1.872657ms min
```

Note: we also have reproduced this using `h2` directly outside of `hyper`, which is what gives me
confidence this issue lies in `h2` and not `hyper`.
  • Loading branch information
howardjohn authored and seanmonstar committed May 2, 2024
1 parent 0d66e3c commit be12983
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1428,7 +1428,12 @@ where

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
self.inner.maybe_close_connection_if_no_streams();
self.inner.poll(cx).map_err(Into::into)
let result = self.inner.poll(cx).map_err(Into::into);
if result.is_pending() && !self.inner.has_streams_or_other_references() {
tracing::trace!("last stream closed during poll, wake again");
cx.waker().wake_by_ref();
}
result
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/proto/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,13 @@ where
}
}

/// Checks if there are any streams or references left
pub fn has_streams_or_other_references(&self) -> bool {
// If we poll() and realize that there are no streams or references
// then we can close the connection by transitioning to GOAWAY
self.inner.streams.has_streams_or_other_references()
}

pub(crate) fn take_user_pings(&mut self) -> Option<UserPings> {
self.inner.ping_pong.take_user_pings()
}
Expand Down

0 comments on commit be12983

Please sign in to comment.