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 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 make this contribution in accordance with the [OpenJDK 
Interim AI Policy](https://openjdk.org/legal/ai).

-------------

Commit messages:
 - 8386989: QuicEndpoint.ClosedConnection should not use QuicTimerQueue::offer

Changes: https://git.openjdk.org/jdk/pull/31693/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=31693&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8386989
  Stats: 23 lines in 3 files changed: 13 ins; 0 del; 10 mod
  Patch: https://git.openjdk.org/jdk/pull/31693.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/31693/head:pull/31693

PR: https://git.openjdk.org/jdk/pull/31693

Reply via email to