On Fri, 26 Jun 2026 15:18:25 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. >> >> ----... > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Review feedback: offer() can call reschedule() LGTM. ------------- Marked as reviewed by djelinski (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/31693#pullrequestreview-4581079384
