On Fri, 26 Jun 2026 12:50:07 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 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())` Right. But I wanted to preserve the slightly different tracing. This method is supposed to be called when the event is not yet scheduled. Though, as we observed, it can be difficult to assert - hence the used of the `rescheduled` set here instead of `scheduled`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31693#discussion_r3481737086
