8386985: PacketSpaceManagerTest failed with AssertionError; A race condition may cause packetSent to mistakenly skip rescheduling of the transmitter task

Author: dfuch

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, 2025, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, 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
@@ -645,22 +645,15 @@ private synchronized boolean shouldLogWhenNewDeadline() {
             return false;
         }
 
-        boolean hasNoDeadline() {
-            return Deadline.MAX.equals(nextDeadline);
-        }
-
         // reschedule this task
         void reschedule() {
             Deadline deadline = computeNextDeadline();
-            Deadline nextDeadline = this.nextDeadline;
             if (Deadline.MAX.equals(deadline)) {
-                debug.log("no deadline, don't reschedule");
-            } else if (deadline.equals(nextDeadline)) {
-                debug.log("deadline unchanged, don't reschedule");
-            } else {
-                packetEmitter.reschedule(this, deadline);
-                debug.log("retransmission task: rescheduled");
+                if (debug.on()) debug.log("no deadline, don't reschedule");
+                return;
             }
+            if (debug.on()) debug.log("retransmission task: rescheduled");
+            packetEmitter.reschedule(this, deadline);
         }
 
         @Override
@@ -1304,7 +1297,7 @@ public void packetSent(QuicPacket packet, long previousPacketNumber, long packet
             } finally {
                 transferLock.unlock();
             }
-            if (found && packetTransmissionTask.hasNoDeadline()) {
+            if (found) {
                 packetTransmissionTask.reschedule();
             }
             if (!found) {
@@ -1340,9 +1333,7 @@ public void packetSent(QuicPacket packet, long previousPacketNumber, long packet
                         return;
                     }
                     addAcknowledgement(pending);
-                    if (packetTransmissionTask.hasNoDeadline()) {
-                        packetTransmissionTask.reschedule();
-                    }
+                    packetTransmissionTask.reschedule();
                 } finally {
                     transferLock.unlock();
                 }

test/jdk/java/net/httpclient/quic/PacketSpaceManagerTest.java

@@ -80,16 +80,19 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
 
 /*
  * @test
+ * @bug 8349910 8386985
  * @summary tests the logic to build an AckFrame
  * @library /test/lib
  * @library ../debug
@@ -102,6 +105,7 @@
  * @run junit/othervm -Dseed=-4159871071396382784 ${test.main.class}
  * @run junit/othervm -Dseed=2252276218459363615 ${test.main.class}
  * @run junit/othervm -Dseed=-5130588140709404919 ${test.main.class}
+ * @run junit/othervm -Dseed=4257295716830862528 ${test.main.class}
  */
 // -Djdk.internal.httpclient.debug=true
 public class PacketSpaceManagerTest {
@@ -766,6 +770,22 @@ boolean isDue(Deadline now) {
             }
         }
 
+        public List<QuicFrame> sendPacket(long offset, AckFrame ackFrameToSend, Packet packet, long largestReceivedAckedPN) {
+            // add a crypto frame and build the packet
+            CryptoFrame crypto = new CryptoFrame(offset, 1,
+                    ByteBuffer.wrap(new byte[] {nextByte(offset)}));
+            List<QuicFrame> frames = ackFrameToSend == null ?
+                    List.of(crypto) : List.of(crypto, ackFrameToSend);
+            QuicPacket newPacket = codingContext.encoder
+                    .newInitialPacket(localId, peerId,
+                            null,
+                            packet.packetNumber,
+                            largestReceivedAckedPN,
+                            frames, codingContext);
+            // pretend that we sent a packet
+            manager.packetSent(newPacket, -1, packet.packetNumber);
+            return frames;
+        }
         /**
          * Drives the test by pretending to emit each packet in order,
          * then pretending to receive ack frames (as soon as possible
@@ -830,19 +850,8 @@ public void run() throws Exception {
                     debug.log("largestAckSent is: " + largestAckAcked);
                 }
 
-                // add a crypto frame and build the packet
-                CryptoFrame crypto = new CryptoFrame(offset, 1,
-                        ByteBuffer.wrap(new byte[] {nextByte(offset)}));
-                List<QuicFrame> frames = ackFrameToSend == null ?
-                        List.of(crypto) : List.of(crypto, ackFrameToSend);
-                QuicPacket newPacket = codingContext.encoder
-                        .newInitialPacket(localId, peerId,
-                                null,
-                                packet.packetNumber,
-                                largestReceivedAckedPN,
-                                frames, codingContext);
-                // pretend that we sent a packet
-                manager.packetSent(newPacket, -1, packet.packetNumber);
+                // send a packet
+                List<QuicFrame> frames = sendPacket(offset, ackFrameToSend, packet, largestReceivedAckedPN);
 
                 // compute next deadline
                 var nextDeadline = timerQueue.nextDeadline();
@@ -1125,4 +1134,52 @@ public void testPacketSpaceManager(TestCase testCase) throws Exception {
         driver.check();
     }
 
+    @Test
+    public void testPacketSent() throws Exception {
+        // this test case is specifically for JDK-8386985
+        System.out.printf("%n -------  testPacketSent ------- %n");
+
+        // create a minimal SynchronousTestDriver
+        TestCase testCase = new TestCase(List.of(new Acknowledged(1, 3), new Acknowledged(4,4)),
+                List.of(new Packet(3, 0), new Packet(4, 0)));
+        SynchronousTestDriver driver = new SynchronousTestDriver(testCase);
+
+        // send a first ack-eliciting packet, and move the timeline past PTO
+        driver.sendPacket(0, null, new Packet(1, 0), -1);
+        Deadline pto = driver.manager.nextScheduledDeadline(); // should be PTO
+        driver.timeSource.advance(driver.timeSource.instant().until(pto, ChronoUnit.MILLIS) + 250, ChronoUnit.MILLIS);
+
+        // start processing events, but delay the task that will run the transmitter
+        ArrayList<Runnable> tasks = new ArrayList<>();
+        Executor executor = new Executor() {
+            @Override
+            public void execute(Runnable command) {
+                tasks.add(command);
+            }
+        };
+        driver.timerQueue.processEventsAndReturnNextDeadline(driver.timeSource.instant(), executor);
+
+        // acknowledge the first packet so that it's no longer pending retransmission
+        driver.manager.processAckFrame(new AckFrameBuilder().addAck(1).build());
+
+        // send a second packet, and examine the timerQueue next deadline
+        // if sending the second packet didn't cause the task to be rescheduled, we
+        // will observe Deadline.MAX, or a deadline before now: that's the bug.
+        driver.sendPacket(1, null, new Packet(2, 0), -1);
+
+        Deadline next = driver.timerQueue.nextDeadline();
+        assertNotEquals(next, Deadline.MAX);
+        assertTrue(next.isAfter(driver.timeSource.instant()));
+
+        // now finish running the task and ack the second packet,
+        // so that we leave the packet space manager in a clean state
+        // for running the driver with the next two packets.
+        for (Runnable task : tasks) {
+            task.run();
+        }
+        driver.manager.processAckFrame(new AckFrameBuilder().addAck(1).addAck(2).build());
+        driver.run();
+        driver.check();
+    }
+
  }