8377181: HttpClient may leak closed QUIC connection objects

Reviewed-by: dfuchs

src/java.net.http/share/classes/jdk/internal/net/http/quic/ConnectionTerminatorImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024, 2026, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -203,6 +203,9 @@ final class ConnectionTerminatorImpl implements ConnectionTerminator {
             // an endpoint has been established (which is OK)
             return;
         }
+        // close the connection ID managers; any in-flight connection ID changes should be ignored.
+        connection.localConnectionIdManager().close();
+        connection.peerConnectionIdManager().close();
         endpoint.removeConnection(this.connection);
     }
 
@@ -434,6 +437,9 @@ final class ConnectionTerminatorImpl implements ConnectionTerminator {
         final QuicPacket packet = connection.newQuicPacket(keySpace, List.of(toSend));
         final ProtectionRecord protectionRecord = ProtectionRecord.single(packet,
                 connection::allocateDatagramForEncryption);
+        // close the connection ID managers; any in-flight connection ID changes should be ignored.
+        connection.localConnectionIdManager().close();
+        connection.peerConnectionIdManager().close();
         // while sending the packet containing the CONNECTION_CLOSE frame, the pushDatagram will
         // remap the QuicConnectionImpl in QuicEndpoint.
         connection.pushDatagram(protectionRecord);

src/java.net.http/share/classes/jdk/internal/net/http/quic/PeerConnIdManager.java

@@ -65,6 +65,7 @@ final class PeerConnIdManager {
     private final QuicConnectionImpl connection;
     private final String logTag;
     private final boolean isClient;
+    private boolean closed; // when true, no more reset tokens are registered
 
     private enum State {
         INITIAL_PKT_NOT_RECEIVED_FROM_PEER,
@@ -267,6 +268,7 @@ final class PeerConnIdManager {
             if (handshakeConnId == null) {
                 throw new IllegalStateException("No handshake peer connection available");
             }
+            if (closed) return;
             // recreate the conn id with the stateless token
             this.peerConnectionIds.put(0L, new PeerConnectionId(handshakeConnId.asReadOnlyBuffer(),
                     statelessResetToken));
@@ -283,6 +285,10 @@ final class PeerConnIdManager {
     public List<byte[]> activeResetTokens() {
         lock.lock();
         try {
+            // this method is currently only used to remove a connection from the endpoint
+            // after the connection is closed.
+            // The below assert can be removed if the method is needed elsewhere.
+            assert closed;
             // we only support one active connection ID at the time
             PeerConnectionId cid = peerConnectionIds.get(activeConnIdSeq);
             byte[] statelessResetToken = null;
@@ -305,7 +311,7 @@ final class PeerConnIdManager {
     QuicConnectionId getPeerConnId() {
         lock.lock();
         try {
-            if (activeConnIdSeq < largestReceivedRetirePriorTo) {
+            if (activeConnIdSeq < largestReceivedRetirePriorTo && !closed) {
                 // stop using the old connection ID
                 switchConnectionId();
             }
@@ -496,9 +502,11 @@ final class PeerConnIdManager {
             // connection ids. It does however store the peer-issued stateless reset token of a
             // peer connection id, so we let the endpoint know that the stateless reset token needs
             // to be forgotten since the corresponding peer connection id is being retired
-            final byte[] resetTokenToForget = entry.getValue().getStatelessResetToken();
-            if (resetTokenToForget != null) {
-                this.connection.endpoint().forgetStatelessResetToken(resetTokenToForget);
+            if (seqNumToRetire == activeConnIdSeq) {
+                final byte[] resetTokenToForget = entry.getValue().getStatelessResetToken();
+                if (resetTokenToForget != null) {
+                    this.connection.endpoint().forgetStatelessResetToken(resetTokenToForget);
+                }
             }
         }
         for (Iterator<Long> iterator = gaps.iterator(); iterator.hasNext(); ) {
@@ -540,4 +548,13 @@ final class PeerConnIdManager {
             lock.unlock();
         }
     }
+
+    public void close() {
+        lock.lock();
+        try {
+            closed = true;
+        } finally {
+            lock.unlock();
+        }
+    }
 }

src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java

@@ -1758,6 +1758,10 @@ public class QuicConnectionImpl extends QuicConnection implements QuicPacketRece
         return localConnIdManager;
     }
 
+    PeerConnIdManager peerConnectionIdManager() {
+        return peerConnIdManager;
+    }
+
     /**
      * {@return the local connection id}
      */

src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java

@@ -1532,12 +1532,16 @@ public abstract sealed class QuicEndpoint implements AutoCloseable
      */
     void removeConnection(final QuicPacketReceiver connection) {
         if (debug.on()) debug.log("removing connection " + connection);
-        // remove the connection completely
-        connection.connectionIds().forEach(connections::remove);
-        assert !connections.containsValue(connection) : connection;
         // remove references to this connection from the map which holds the peer issued
         // reset tokens
         dropPeerIssuedResetTokensFor(connection);
+        // remove the connection completely
+        connection.connectionIds().forEach(connections::remove);
+        assert !connections.containsValue(connection) : connection;
+        // Check that if there are no connections, there are no reset tokens either.
+        // This is safe because connections are added before reset tokens and removed after,
+        // except when we're closing the endpoint and don't bother with removing tokens.
+        assert peerIssuedResetTokens.isEmpty() || !connections.isEmpty() || closed : peerIssuedResetTokens;
     }
 
     /**
@@ -1587,7 +1591,6 @@ public abstract sealed class QuicEndpoint implements AutoCloseable
         if (closed) return;
 
         final long idleTimeout = connection.peerPtoMs() * 3; // 3 PTO
-        connection.localConnectionIdManager().close();
         DrainingConnection draining = new DrainingConnection(connection.connectionIds(),
                 connection.activeResetTokens(), idleTimeout);
         // we can ignore stateless reset in the draining state.
@@ -1626,7 +1629,6 @@ public abstract sealed class QuicEndpoint implements AutoCloseable
         closingDatagram.flip();
 
         final long idleTimeout = connection.peerPtoMs() * 3; // 3 PTO
-        connection.localConnectionIdManager().close();
         var closingConnection = new ClosingConnection(connection.connectionIds(),
                 connection.activeResetTokens(), idleTimeout, datagram);
         remapPeerIssuedResetToken(closingConnection);

test/jdk/java/net/httpclient/CancelRequestTest.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 8245462 8229822 8254786 8297075 8297149 8298340 8302635
+ * @bug 8245462 8229822 8254786 8297075 8297149 8298340 8302635 8377181
  * @summary Tests cancelling the request.
  * @library /test/lib /test/jdk/java/net/httpclient/lib
  * @key randomness

test/jdk/java/net/httpclient/http3/H3ErrorHandlingTest.java

@@ -71,7 +71,7 @@ import static org.junit.jupiter.api.Assertions.*;
 
 /*
  * @test
- * @bug 8373409
+ * @bug 8373409 8377181
  * @key intermittent
  * @comment testResetControlStream may fail if the client doesn't read the stream type
  *              before the stream is reset,