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]

Last change

[hyperxpro]

Behaviour is right now. The comment lost more than it needed to, though. The old block said why this loops at all, the Issue 2160 missed wakeup class, that tryAcquireStream gates both the cap and the draining state so the loop can never over open, and why poll is safe to dereference unchecked. What survived no longer says why the opener runs outside the lock, which is the point of the whole change.

Comments only, no behaviour change:

Suggested change

	private void drainPendingOpeners() {
	synchronized (pendingLock) {
	// Open as many queued requests as there are now-free stream slots. A single stream completion
	// frees exactly one slot (so this usually runs one opener), but a SETTINGS frame that RAISES
	// SETTINGS_MAX_CONCURRENT_STREAMS frees several at once — drain them all here rather than waking
	// only one and stalling the rest until the next completion (a missed-wakeup; the Issue #2160
	// silent-timeout class). tryAcquireStream() enforces the cap and the draining/closed gate, so
	// this never over-opens; every poll is under pendingLock, so a non-empty queue always yields a
	// non-null opener.
	while (!pendingOpeners.isEmpty() && tryAcquireStream()) {
	while (true) {
	PendingOpener pending;
	synchronized (pendingLock) {
	// A SETTINGS increase can free several slots at once. Reserve and dequeue one opener
	// atomically, then repeat so an exception cannot strand an already-dequeued batch.
	if (pendingOpeners.isEmpty() || !tryAcquireStream()) {
	return;
	}
	pendingCount--;
	pendingOpeners.poll().opener.run();
	pending = pendingOpeners.poll();
	}
	// Stream opening can invoke user callbacks such as onRequestSend.
	pending.opener.run();
	}
	}
	private void drainPendingOpeners() {
	// Open as many queued requests as there are now-free stream slots. A single stream completion
	// frees exactly one slot (so this usually runs one opener), but a SETTINGS frame that RAISES
	// SETTINGS_MAX_CONCURRENT_STREAMS frees several at once — drain them all here rather than waking
	// only one and stalling the rest until the next completion (a missed-wakeup; the Issue #2160
	// silent-timeout class). Poll ONE opener per iteration rather than reserving the whole batch up
	// front: an opener that throws must not strand the openers behind it, which would leak their
	// already-reserved slots and leave them un-drainable, since nothing ever re-queues them.
	while (true) {
	PendingOpener pending;
	synchronized (pendingLock) {
	// tryAcquireStream() enforces the cap and the draining/closed gate, so this never
	// over-opens; reserve the slot and dequeue its opener atomically under pendingLock. Every
	// poll is under pendingLock, so a queue seen non-empty here always yields a non-null opener.
	if (pendingOpeners.isEmpty() || !tryAcquireStream()) {
	return;
	}
	pendingCount--;
	pending = pendingOpeners.poll();
	}
	// Stream opening can invoke user callbacks such as onRequestSend. Run after releasing
	// pendingLock so a slow callback does not serialize unrelated submissions to this connection.
	pending.opener.run();
	}
	}

[pavel-ptashyts]

Agreed. I restored the key invariants in a shorter form: the comment now explains the SETTINGS-driven multi-opener drain, the Issue #2160 missed-wakeup risk, the capacity and draining/closed gates, why poll() is safe under pendingLock, and why each opener runs after the lock is released. This is in b63975d.

[hyperxpro]

Small thing, so this does not get misread later. The 2 here counts the slot the throwing opener itself reserved and never released. That leak predates this PR, the old code lost it the same way, and it is unreachable in production since the real opener is Http2StreamChannelBootstrap.open().addListener(...) and Netty swallows listener throwables. So this assertion pins current behaviour rather than blessing the leak. A one line comment saying so would save the next reader the trip.

[pavel-ptashyts]

Agreed. I added a one-line comment stating that the throwing public Runnable retains its reserved slot and that this matches the behavior from before this change. This is in b63975d.