On Fri, 26 Jun 2026 11:53:14 GMT, Daniel Fuchs <[email protected]> wrote:
> This issue was noticed because
> `H3MultipleConnectionsToSameHost.java#useNioSelector` once threw an
> `AssertionError` in our CI:
>
> 16 failed: java.util.concurrent.CompletionException: java.io.IOException:
> java.lang.AssertionError: Invalid deadline for
> PacketTransmissionTask(QuicConnection(HttpClientImpl(1),
> QuicClientConnection(37))[HANDSHAKE])
>
>
> This assertion is thrown if a task is found in the `QuicTimerQueue` with a
> deadline equals to `Deadline.MAX`, which should never happen. Code reading
> suggested a potential suspect in `QuicTimerQueue::offer`, but
> `PacketTransmissionTask` does not use that. Further instrumentation of the
> `QuicTimerQueue` (too intrusive, so not part of this fix) and repeated
> testing eventually reproduced the issue once and revealed that the issue
> originated from a race condition between `ClosedConnection::startTimer` and
> `ClosedConnection::processIncoming` when switching to a `DrainingConnection`.
> Since the draining connection is added to the connections map before
> `startTimer` is called, then `processIncoming` can end up running
> concurrently, or before, `startTimer` is called. If that happens, then when
> `startTimer` is called the timer event can already be in the
> `QuicTimerQueue.rescheduled` set. `startTimer` assumes that the event has
> never been added to the list and calls `offer`, at a time when
`QuicTimerQueue::processRescheduled` might be running, which could end up with
`offer` re-adding the event to the `scheduled` skiplist at a time where
`processRescheduled` has taken it out of the skiplist in order to call its
`refreshDeadline`.
>
> This patch fixes the issue in two ways:
>
> 1. it modifies `ClosedConnection::startTimer` to call
> `QuicTimerQueue::reschedule` instead of `QuicTimerQueue::offer`; This fixes
> the issue and has the advantage of being able to pass a prospective deadline.
> Passing a prospective deadline can avoid re-awakening the selector if it is
> not needed.
> 2. it also modifies `QuicTimerQueue::offer` to add the offered event to the
> `rescheduled` set, instead of adding it to the `scheduled` skiplist. This
> also would solve the issue on its own. It makes `QuicTimerQueue` more
> resilient to the use of `offer` which can be called concurrently with
> `reschedule` or `processRescheduled`.
>
> In addition I have revisited the policy with which the selector is awaken
> again. I noticed that multiple additions of events at the same deadline could
> cause repeated calls to notifier.run; This is now fixed.
>
> ---------
> - [x] I confirm that I m...
The fix looks good, but the code could use some simplification.
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicTimerQueue.java
line 124:
> 122: assert QuicTimedEvent.COMPARATOR.compare(event, CEILING) < 0;
> 123: Deadline deadline = event.deadline();
> 124: rescheduled.add(event);
With this change, `offer(event)` implementation can be simplified to
`reschedule(event,event.deadline())`
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicTimerQueue.java
line 359:
> 357: // On the other hand, if deadline < notifiedDeadline, we
> 358: // need to call the notifier to force an additional
> wakeup
> 359: if (deadline.isBefore(notifiedDeadline)) {
`notifiedDeadline` is basically the same as `scheduledDeadline`. Please remove
the `scheduled` method and use `scheduledDeadline` here.
-------------
PR Review: https://git.openjdk.org/jdk/pull/31693#pullrequestreview-4579552112
PR Review Comment: https://git.openjdk.org/jdk/pull/31693#discussion_r3481475180
PR Review Comment: https://git.openjdk.org/jdk/pull/31693#discussion_r3481500642
