fix core: close idle HTTP/2 connections on keepalive_timeout#1304
Open
SSE4 wants to merge 1 commit into
Open
Conversation
SSE4
force-pushed
the
h2-idle-timeout-fix
branch
from
July 23, 2026 09:53
e0f11c7 to
57410cd
Compare
| if (!ready_id) { | ||
| UASSERT(ready_id == utils::unexpected(engine::WaitAnyError::kCancelled)); | ||
| // Keep waiting while a request is still in flight; otherwise (idle | ||
| // keepalive timeout or cancellation) close the connection. |
Contributor
There was a problem hiding this comment.
A good comment should explains WHY we do that (not WHAT we do). Lets remove that
An idle or slowly-dripping HTTP/2 connection was never closed by the server: ListenForRequests() blocked in wait_any.Wait() with no deadline. keepalive_timeout is still applied inside SocketBufferedReader::TryRead, but that runs only after Wait() reports the socket readable, so a connection that never becomes readable never enters TryRead and the deadline never applies. A client can therefore hold connections open at near-zero cost and exhaust the server's max_connections slots, after which new connections are refused for every client -- a Slowloris. HTTP/1.1 closes such a connection after keepalive_timeout; HTTP/2 used to as well, until the HttpReader refactor replaced the deadline-carrying WaitOnSocket with a deadline-less WaitAnyContext loop. Wait with the keepalive deadline instead: wait_any.WaitUntil( Deadline::FromDuration(keepalive_timeout)) replaces wait_any.Wait(). A timeout while a handler task is still in flight keeps waiting (the connection is serving a response and must not be torn down for lack of incoming bytes); an idle timeout or a cancellation closes the connection. Reusing keepalive_timeout keeps parity with HTTP/1.1. Like HTTP/1.1, the timeout is a per-idle-period deadline rather than a whole-connection cap, so a slow drip of bytes resets it; a separate longer cap can be added later if wanted. Covered by a new IdleKeepAliveTimeout test in connection_test.cpp (TYPED_UTEST over both connection types): after a keep-alive request the connection must terminate on the keepalive timeout on its own, which previously failed for HTTP/2.
SSE4
force-pushed
the
h2-idle-timeout-fix
branch
from
July 24, 2026 03:09
57410cd to
eef9563
Compare
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
2 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.
An idle or slowly-dripping HTTP/2 connection is never closed by the server, so
a client can hold connections open at near-zero cost and exhaust the server's
max_connectionsslots (default 32768), after which new connections arerefused for every client — a classic Slowloris. HTTP/1.1 closes such a
connection after
keepalive_timeout; HTTP/2 used to as well, but theenforcement was dropped in the HttpReader refactor (f753790) that replaced
the deadline-carrying
WaitOnSocketwith a deadline-lessWaitAnyContextloop.
Root cause:
Http2Connection::ListenForRequests()blocked inwait_any.Wait()with no deadline.keepalive_timeoutis still applied insideSocketBufferedReader::TryRead, but that runs only afterWait()reportsthe socket readable — a connection that never becomes readable never enters
TryRead, so the deadline never applies.This restores the timeout on the current concurrent connection loop:
WaitUntil(Deadline::FromDuration(config_.keepalive_timeout))instead of
Wait(). On expiry the connection is closed only when it is idle(
IsIdle()— no in-flight handler task); otherwise the loop re-arms andkeeps waiting. This makes the deadline a purely inbound idle timeout: a
connection actively serving a response is never torn down for lack of
incoming bytes. The close reuses HTTP/1.1's
"Closing idle connection on timeout"log line.the new
kTimeoutbranch is added.Reusing
keepalive_timeoutkeeps parity with HTTP/1.1. Like HTTP/1.1, thetimeout is a per-idle-period deadline, not a whole-connection cap — a slow
drip of bytes resets it — so a separate longer whole-connection cap could be
added later if desired; it is out of scope here.
Verified end-to-end against
ng200okwithkeepalive_timeout: 5over h2c: anidle connection (preface only, no request) is closed at exactly 5.0s and logs
the idle-close line. The in-flight-request path (a
GET /sql/?delay=8handlerslower than the keepalive survives the deadline and returns 200 at 8.3s) was
verified on the body-streaming-restore build, since
ng200okusesresponse-body-stream: true, which only works there; theIsIdle()gate thatprotects it is identical.
closes: #1303