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(); + }, + ); }); }