fix(http2): Http2Client evicts dead connections from its pool and keeps healthy ones on a stream reset

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
 

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);
   }
 

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.

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<void>().future;
       final pool = _pool(

pkgs/http2/test/http2_client_test.dart

@@ -309,6 +309,8 @@ void main() {
         first.stream.drain<void>(),
         throwsA(isA<ClientException>()),
       );
+      // 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 = <ServerTransportConnection>[];
+        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<void>();
+            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<ClientException>()));
+        // 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<ClientException>()));
+        // 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();
+      },
+    );
   });
 }