Run HTTP/2 stream openers outside lock

[pavel-ptashyts]

Summary

• reserve HTTP/2 stream slots and dequeue pending openers while holding pendingLock
• invoke immediate and queued stream-opening callbacks after releasing the lock
• keep the empty pending-queue release path allocation-free
• add deterministic concurrency tests for both opener paths

Motivation

Http2ConnectionState invoked stream openers while holding pendingLock. Opening a stream can reach AsyncHandler.onRequestSend, so one slow callback serialized concurrent submissions to the same HTTP/2 connection.

The change preserves the Issue #2160 close/enqueue happens-before relationship and atomic stream-slot accounting. Only callback execution moves outside the monitor.

Validation

• ./mvnw -pl client -Dtest=org.asynchttpclient.netty.channel.Http2ConnectionStateTest test
  • 49 tests passed
• ./mvnw -pl client -Dtest='org.asynchttpclient.netty.channel.Http2ConnectionStateTest,org.asynchttpclient.Http2StreamOrphanRegressionTest,org.asynchttpclient.Http2MultiplexBugRegressionTest,org.asynchttpclient.Http2ResidualFixesRegressionTest' test
  • 68 tests passed

The local environment only provides JDK 21. The JDK 11 ./mvnw clean verify gate was not run locally; the repository CI matrix provides JDK 11 coverage.

Attribution

Codex on behalf of Pavel Ptashyts

[hyperxpro]

Nice catch on the contention, and keeping tryAcquireStream inside the lock was the right call. One thing on this loop though: every opener is polled out of the queue and pendingCount decremented before any of them run. If the first opener throws, the rest are already gone from pendingOpeners, so they never run, never get re-queued, and their reserved stream slots are never released. Before this change a throwing opener left the queue intact and the next releaseStream drained it.

I queued three openers with the first one throwing, then raised SETTINGS_MAX_CONCURRENT_STREAMS so all three drained in one pass:

before: ran=[1, 2, 3] active=3
after:  ran=[1]       active=3

Two requests stranded and two slots leaked, which is the silent-timeout class this file is built to prevent. It is not reachable through openHttp2Stream today because Netty swallows throwables in open() and in DefaultPromise.notifyListener0, but addPendingOpener(Runnable) and offerPendingOpener(Runnable) are public and take arbitrary Runnables.

Poll one and run one is smaller than what is here, keeps the openers outside the lock, drops the per drain ArrayList, and restores the old throw behaviour:

private void drainPendingOpeners() {
    while (true) {
        PendingOpener pending;
        synchronized (pendingLock) {
            if (pendingOpeners.isEmpty() || !tryAcquireStream()) {
                return;
            }
            pendingCount--;
            pending = pendingOpeners.poll();
        }
        pending.opener.run();
    }
}

The ArrayList and List imports stay as they are, failPendingOpeners still needs them. I ran this locally: all 49 tests in Http2ConnectionStateTest pass including both new ones, and the probe above goes back to ran=[1, 2, 3].

[pavel-ptashyts]

Agreed. I replaced the ready-list batch with a poll-one/run-one loop. Each stream slot and opener are now reserved atomically under pendingLock, then that opener runs after the lock is released. If it throws, later openers remain queued and no slots are pre-reserved for callbacks that did not run.

[hyperxpro]

These two cover the lock release nicely. One gap alongside them: neither drains more than a single opener, and multiplePendingOpenersExecuteInOrder just below releases one slot at a time, so the ready list never holds more than one entry. The batch drain this PR introduces has no coverage. Could you add a case that queues three openers and then calls updateMaxConcurrentStreams with a higher value so all three drain in one pass, asserting execution order and the final activeStreams count?

[pavel-ptashyts]

Added coverage for both multi-opener paths. One test raises the limit from one to four and verifies all three queued openers run in order with four active streams. A second test makes the first opener throw and verifies the remaining two stay queued and drain on the next release.

[hyperxpro]

Small doc point. "Either way no opener is left stranded" now has a third state it does not cover: an opener already dequeued into the ready list but not yet run, which failPendingOpeners can no longer see. Unreachable in practice given the event loop confinement, but the reasoning block down at failPendingOpeners no longer tells the whole story. The poll one run one form suggested above removes that state entirely, so this sentence stays accurate as written.

[pavel-ptashyts]

The ready-list intermediate state is gone. The drain now removes only the opener it is about to run, so the existing no-stranding reasoning remains accurate.