Repository navigation
fix: bound WebSocket handshakes and request header reads - #1657
Merged
Merged
Conversation
Client: `WsTransportClientBuilder::connection_timeout` only covered the TCP connect and the TLS handshake. The WebSocket upgrade then waited for the server's response without any timeout, so a peer that accepted the connection but never answered left `build()` hanging forever. A single such attempt was enough to wedge reconnecting clients, e.g. subxt's. The handshake is now bounded by `connection_timeout` as well, for both `build` and `build_with_stream`, and fails with `WsHandshakeError::Timeout`. Server: hyper's HTTP/1 `header_read_timeout` was never active because no timer was configured, and hyper-util's auto builder reads the protocol preface without any timeout. Connections that never sent a (complete) request header therefore stayed open forever. The server now configures a `TokioTimer` for HTTP/1 and bounds the first read, so such connections are closed after `header_read_timeout` (default 30s, configurable via `ServerConfigBuilder::set_header_read_timeout`; `serve` and `serve_with_graceful_shutdown` use the default). Idle HTTP/1 keep-alive connections are closed after the same timeout; upgraded WebSocket connections are not affected.
alvicsam
approved these changes
Sep 29, 2026
skunert
reviewed
Sep 29, 2026
| let read = Pin::new(&mut this.io).poll_read(cx, buf); | ||
|
|
||
| if read.is_ready() { | ||
| this.deadline = None; |
There was a problem hiding this comment.
It is possible to outplay this timeout if the remote sends a partial prefix for HTTP/2.
The loop here calls our function repeatedly as long as the received bytes match the prefix. So if remote sends P and then nothing for example, we wait here forever. So to fix this we would need to check here if the first byte uniquely identifies the protocol.
skunert
reviewed
Sep 29, 2026
| type Future = S::Future; | ||
|
|
||
| fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
| self.service.poll_ready(cx) |
There was a problem hiding this comment.
I think it would be smarter to call notify_one() here.
Otherwise even if a client sends us something and the inner service does not signal readiness immediately, we might abort the connection before reaching call below, even though its not the clients fault.
skunert
approved these changes
Sep 29, 2026
Updated release date for version 0.26.1 and clarified changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Client:
WsTransportClientBuilder::connection_timeoutonly covered the TCP connect and the TLS handshake. The WebSocket upgrade then waited for the server's response without any timeout, so a peer that accepted the connection but never answered leftbuild()hanging forever. A single such attempt was enough to wedge reconnecting clients, e.g. subxt's. The handshake is now bounded byconnection_timeoutas well, for bothbuildandbuild_with_stream, and fails withWsHandshakeError::Timeout.Server: hyper's HTTP/1
header_read_timeoutwas never active because no timer was configured, and hyper-util's auto builder reads the protocol preface without any timeout. Connections that never sent a (complete) request header therefore stayed open forever. The server now configures aTokioTimerfor HTTP/1 and bounds the first read, so such connections are closed afterheader_read_timeout(default 30s, configurable viaServerConfigBuilder::set_header_read_timeout;serveandserve_with_graceful_shutdownuse the default). Idle HTTP/1 keep-alive connections are closed after the same timeout; upgraded WebSocket connections are not affected.