From 9174e50f9bc099f7da794d631d4a0f752fc50186 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Oct 2026 10:27:11 +0000 Subject: [PATCH] fix(http2): Http2Client evicts dead connections from its pool and keeps healthy ones on a stream reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > [!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!` --- pkgs/http2/CHANGELOG.md | 3 ++ pkgs/http2/lib/src/client_pool.dart | 14 ++++++- pkgs/http2/lib/src/http2_client.dart | 16 +++++++- pkgs/http2/test/client_pool_test.dart | 16 ++++++++ pkgs/http2/test/http2_client_test.dart | 54 ++++++++++++++++++++++++++ 5 files changed, 99 insertions(+), 4 deletions(-) diff --git a/pkgs/http2/CHANGELOG.md b/pkgs/http2/CHANGELOG.md index 3cec911a82..e4a9dcade5 100644 --- a/pkgs/http2/CHANGELOG.md +++ b/pkgs/http2/CHANGELOG.md @@ -30,6 +30,9 @@ - `finish()` and `terminate()` no longer hang when a write to the socket fails (for example because the peer closed the connection); previously they waited for a `Socket.done` that never completes after a failed write. +- `Http2Client` now drops a connection from its pool as soon as it is dead, + instead of keeping it as an idle connection no request may use, and keeps a + healthy connection pooled when the server resets just one of its streams. ## 3.1.0 diff --git a/pkgs/http2/lib/src/client_pool.dart b/pkgs/http2/lib/src/client_pool.dart index 359ee75425..6f9783c61b 100644 --- a/pkgs/http2/lib/src/client_pool.dart +++ b/pkgs/http2/lib/src/client_pool.dart @@ -89,7 +89,15 @@ class PoolLease { var _released = false; /// Stops the pool routing new work to this connection. - void markFailed() => _pooled.createFailed = true; + /// + /// May be called after [release]: a stream's failure is often only + /// understood once its terminal callback has already given the slot back. + /// The connection is then closed right away if it is idle, instead of + /// lingering in the pool as a connection no one may use. + void markFailed() { + _pooled.createFailed = true; + if (_released && !_pool._terminated) _pool._closeIfIdle(_pooled); + } /// Gives the slot back. Idempotent, so it is safe to call from several /// terminal paths that may race. @@ -231,7 +239,9 @@ class ClientPool { if (pooled.inFlightCount > 0) return; if (!pooled.createFailed && !_hasExcessIdleCapacity(pooled)) return; - _connections.remove(pooled); + // Already evicted - e.g. by the release of a lease that is only now also + // being marked failed. + if (!_connections.remove(pooled)) return; _startClose(pooled); } diff --git a/pkgs/http2/lib/src/http2_client.dart b/pkgs/http2/lib/src/http2_client.dart index 1f234bbfab..1db3dd609c 100644 --- a/pkgs/http2/lib/src/http2_client.dart +++ b/pkgs/http2/lib/src/http2_client.dart @@ -248,6 +248,7 @@ class Http2Client extends BaseClient { lease.release(); }, onError: (Object error, StackTrace stackTrace) { + if (_isConnectionFailure(transport, error)) lease.markFailed(); final failure = error is ClientException ? error @@ -305,8 +306,10 @@ class Http2Client extends BaseClient { } try { return await _sendOverHttp2(lease, request, bodyBytes!); - } catch (_) { - lease.markFailed(); + } catch (error) { + // A reset of just this stream says nothing about the connection, which + // keeps serving its other streams - only drop it when it is gone. + if (_isConnectionFailure(lease.connection, error)) lease.markFailed(); lease.release(); rethrow; } @@ -364,6 +367,15 @@ class _ConnectionClosedByPeer implements Exception { 'request could be sent.'; } +/// Whether [error], raised by a request on [connection], means the connection +/// itself is gone or going away - as opposed to the peer having reset just +/// that one stream (RFC 9113 5.4.2), which leaves the connection and its other +/// streams intact. +bool _isConnectionFailure(ClientConnection connection, Object error) => + error is TransportConnectionException || + error is _ConnectionClosedByPeer || + !connection.isOpen; + /// HTTP/2 carries no reason phrase (RFC 9113 8.3.2 dropped it as redundant /// with the status code), so one is derived from the status instead - the same /// approach `package:cupertino_http` takes for NSURLSession. diff --git a/pkgs/http2/test/client_pool_test.dart b/pkgs/http2/test/client_pool_test.dart index 3e5a84a484..0fae423437 100644 --- a/pkgs/http2/test/client_pool_test.dart +++ b/pkgs/http2/test/client_pool_test.dart @@ -403,6 +403,22 @@ void main() { expect(used, [0, 1]); }); + test('a-lease-failed-after-its-release-evicts-the-connection', () async { + final connections = _Connections(); + final pool = _pool(connections, maxConcurrentStreams: 10); + + final lease = await pool.acquire(); + lease.release(); + expect(pool.size, 1, reason: 'an idle connection is kept'); + + // A stream's failure is often only understood after its terminal + // callback has already given the slot back. + lease.markFailed(); + + expect(pool.size, 0); + expect(connections.closed, [0]); + }); + test('does-not-couple-a-stream-to-a-slow-close', () async { final connections = _Connections()..closeGate = Completer().future; final pool = _pool( diff --git a/pkgs/http2/test/http2_client_test.dart b/pkgs/http2/test/http2_client_test.dart index 37d95190af..bbe179f40b 100644 --- a/pkgs/http2/test/http2_client_test.dart +++ b/pkgs/http2/test/http2_client_test.dart @@ -309,6 +309,8 @@ void main() { first.stream.drain(), throwsA(isA()), ); + // The connection died under the body, so it must have left the pool. + expect(client.connectionCount, 0); gate.complete(); @@ -441,5 +443,57 @@ void main() { await server.close(); }); + + test( + 'evicts-a-dead-connection-but-keeps-one-whose-stream-was-reset', + () async { + final context = _serverContext()..setAlpnProtocols(['h2'], true); + final socket = await SecureServerSocket.bind('localhost', 0, context); + final serverConnections = []; + var requestCount = 0; + + socket.listen((raw) { + final connection = ServerTransportConnection.viaSocket(raw); + serverConnections.add(connection); + connection.incomingStreams.listen((stream) async { + final index = requestCount++; + await stream.incomingMessages.drain(); + switch (index) { + case 0: + // Tear down the whole connection before any response headers. + await connection.terminate(); + case 1: + // On a fresh connection: reset only this stream (RST_STREAM), + // leaving the connection itself healthy. + stream.terminate(); + default: + stream.sendHeaders([Header.ascii(':status', '200')]); + stream.sendData(ascii.encode('reused'), endStream: true); + } + }); + }); + + final client = _testClient(maxIdleConnections: 1); + final url = Uri.parse('https://localhost:${socket.port}/'); + + await expectLater(client.get(url), throwsA(isA())); + // The dead connection must have left the pool, not stayed as its one + // idle connection. + expect(client.connectionCount, 0); + + await expectLater(client.get(url), throwsA(isA())); + // A reset of one stream says nothing about the connection. + expect(client.connectionCount, 1); + + final response = await client.get(url); + expect(response.statusCode, 200); + expect(response.body, 'reused'); + expect(serverConnections, hasLength(2)); + + client.close(); + await client.closed; + await socket.close(); + }, + ); }); }