Repository navigation
fix(http2): Http2Client evicts dead connections from its pool and keeps healthy ones on a stream reset - #2010
Conversation
f8bbcfd to
3b80204
Compare
3b80204 to
faab4e8
Compare
faab4e8 to
b4bd763
Compare
b4bd763 to
c157584
Compare
PR HealthCoverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Unused Dependencies ✔️
For details on how to fix these, see dependency_validator. This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
…ps healthy ones on a stream reset > [!NOTE] > This PR was generated by an AI coding agent (Jetski) on behalf of @mosuem. ### Summary Two pool-health bugs in `Http2Client`, one hiding the other: 1. **A dead connection stayed in the pool.** When a request failed because its connection died (socket closed, `GOAWAY` + close, protocol error), the stream's `onError` callback called `lease.release()` first, and only afterwards did `send()`'s `attempt()` catch block call `lease.markFailed()`. `release()` is where `ClientPool._closeIfIdle` runs, and at that moment `createFailed` was still `false`, so the connection was kept as the pool's idle connection. The later `markFailed()` only set a flag on an already-released slot, so nothing ever closed it: it stayed in `_connections` (counted by `connectionCount`, skipped by `_select`) until `close()`. The same happened when a connection died under a response body. 2. **A healthy connection was marked failed on any stream error.** `attempt()` marked the lease failed on *every* error, including a server resetting just that one stream (`RST_STREAM`, RFC 9113 Section 5.4.2 — a stream error "does not affect the other streams on the connection"). Together with (1) that meant every reset stream left a zombie connection behind and the next request dialed a new one. ### Changes - **`lib/src/client_pool.dart`**: `PoolLease.markFailed()` now works regardless of ordering: if the slot was already released, it evicts the (idle) connection right away via `_closeIfIdle`, which is made idempotent (`_connections.remove` is checked) so a lease that was released and later marked failed cannot close a connection twice. Fixing this in the pool rather than at each call site means no caller has to get the `markFailed()`/`release()` order right. - **`lib/src/http2_client.dart`**: a single `_isConnectionFailure(connection, error)` predicate — `TransportConnectionException`, `_ConnectionClosedByPeer`, or `!connection.isOpen` — decides whether a failure condemns the connection. It is used in the stream's `onError` (so a connection that dies under a body is evicted at once, not on the next request) and in `attempt()`'s catch block (so a reset of one stream no longer evicts the connection). Not changed, as a candidate follow-up: `ClientConnection.isOpen` is also `false` while the connection is merely at the peer's `SETTINGS_MAX_CONCURRENT_STREAMS` limit, so in that narrow state a healthy connection could still be evicted (gracefully: it is closed once idle and a new one is dialed). A dedicated "finishing or terminated" getter would make the predicate exact; `_sendOverHttp2`'s existing `!transport.isOpen` check has the same property. ### Test Verification (Fails Before $\rightarrow$ Passes After) - `a-lease-failed-after-its-release-evicts-the-connection` in `test/client_pool_test.dart` (pool-level, with the mock connections). - `evicts-a-dead-connection-but-keeps-one-whose-stream-was-reset` in `test/http2_client_test.dart`: request 1 has its connection terminated before any response (pool must be empty afterwards), request 2 gets `RST_STREAM` on a fresh connection (pool must still hold it), request 3 must be served on that same connection. No sleeps; every assertion follows the request's own completion. Also checked that reverting only the `attempt()` guard fails this test at the "connection kept" assertion (`Expected: <1> Actual: <0>`). - `releases-the-slot-when-the-response-body-errors` (existing) now also asserts `connectionCount == 0` after a connection dies under a body. **Before fix** (tests on the previous library code): ```text 00:00 +0 -1: test/client_pool_test.dart: client-pool-test a-lease-failed-after-its-release-evicts-the-connection [E] Expected: <0> Actual: <1> 00:00 +0 -2: test/http2_client_test.dart: http2-client-test releases-the-slot-when-the-response-body-errors [E] Expected: <0> Actual: <1> 00:00 +0 -3: test/http2_client_test.dart: http2-client-test evicts-a-dead-connection-but-keeps-one-whose-stream-was-reset [E] Expected: <0> Actual: <1> ``` **After fix:** ```text 00:00 +3: All tests passed! ``` Full `package:http2` suite at this point of the stack (on `master`): `+267 ~7: All tests passed!`
c157584 to
9174e50
Compare
Note
This PR was generated by an AI coding agent (Jetski) on behalf of @mosuem.
Summary
Two pool-health bugs in
Http2Client, one hiding the other:GOAWAY+ close, protocol error), the stream'sonErrorcallback calledlease.release()first, and only afterwards didsend()'sattempt()catch block calllease.markFailed().release()is whereClientPool._closeIfIdleruns, and at that momentcreateFailedwas stillfalse, so the connection was kept as the pool's idle connection. The latermarkFailed()only set a flag on an already-released slot, so nothing ever closed it: it stayed in_connections(counted byconnectionCount, skipped by_select) untilclose(). The same happened when a connection died under a response body.attempt()marked the lease failed on every error, including a server resetting just that one stream (RST_STREAM, RFC 9113 Section 5.4.2 — a stream error "does not affect the other streams on the connection"). Together with (1) that meant every reset stream left a zombie connection behind and the next request dialed a new one.Changes
lib/src/client_pool.dart:PoolLease.markFailed()now works regardless of ordering: if the slot was already released, it evicts the (idle) connection right away via_closeIfIdle, which is made idempotent (_connections.removeis checked) so a lease that was released and later marked failed cannot close a connection twice. Fixing this in the pool rather than at each call site means no caller has to get themarkFailed()/release()order right.lib/src/http2_client.dart: a single_isConnectionFailure(connection, error)predicate —TransportConnectionException,_ConnectionClosedByPeer, or!connection.isOpen— decides whether a failure condemns the connection. It is used in the stream'sonError(so a connection that dies under a body is evicted at once, not on the next request) and inattempt()'s catch block (so a reset of one stream no longer evicts the connection).Not changed, as a candidate follow-up:
ClientConnection.isOpenis alsofalsewhile the connection is merely at the peer'sSETTINGS_MAX_CONCURRENT_STREAMSlimit, so in that narrow state a healthy connection could still be evicted (gracefully: it is closed once idle and a new one is dialed). A dedicated "finishing or terminated" getter would make the predicate exact;_sendOverHttp2's existing!transport.isOpencheck has the same property.Test Verification (Fails Before$\rightarrow$ Passes After)
a-lease-failed-after-its-release-evicts-the-connectionintest/client_pool_test.dart(pool-level, with the mock connections).evicts-a-dead-connection-but-keeps-one-whose-stream-was-resetintest/http2_client_test.dart: request 1 has its connection terminated before any response (pool must be empty afterwards), request 2 getsRST_STREAMon a fresh connection (pool must still hold it), request 3 must be served on that same connection. No sleeps; every assertion follows the request's own completion. Also checked that reverting only theattempt()guard fails this test at the "connection kept" assertion (Expected: <1> Actual: <0>).releases-the-slot-when-the-response-body-errors(existing) now also assertsconnectionCount == 0after a connection dies under a body.Before fix (tests on the previous library code):
After fix:
Full
package:http2suite at this point of the stack (onmaster):+267 ~7: All tests passed!