On Fri, 26 Jun 2026 12:55:04 GMT, Daniel Jeliński <[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.
>>
>> ----...
>
> 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.
It's unfortunately not. `notifiedDeadline` is about when to call the
`notifier.run()` again. It is the minimum deadline that caused notifier.run()
to be called. Any deadline later or equal to this should not cause the notifier
to be run again. This does not exactly match with the `scheduledDeadline` which
is a prospective deadline that was submitted to `rescheduled` while the
selector may not yet have folded it into `scheduled`.
You will notice that `scheduledDeadline` is set again to `Deadline.MAX` as soon
as the `next` deadline has been computed. `notifiedDeadline` is not. In other
words `notifiedDeadline` is there to figure out whether the selector should be
woken up early again right after `processEventsAndReturnNextDeadline` finishes
in order to process events again and recompute a new deadline.
`scheduledDeadline` is the prospective deadline that will be returned by
`processEventsAndReturnNextDeadline`. I have tried very hard to not add a third
deadline, but ended up doing it anyway given that they have a different life
cycle.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31693#discussion_r3482104309
