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... This pull request has now been integrated. Changeset: 17f2e11f Author: Daniel Fuchs <[email protected]> URL: https://git.openjdk.org/jdk/commit/17f2e11fe400ade68ee3b0ac4706209aae4e14dd Stats: 48 lines in 3 files changed: 13 ins; 25 del; 10 mod 8386989: QuicEndpoint.ClosedConnection should not use QuicTimerQueue::offer Reviewed-by: djelinski ------------- PR: https://git.openjdk.org/jdk/pull/31693
