diff --git a/client/src/main/java/org/asynchttpclient/LoadBalance.java b/client/src/main/java/org/asynchttpclient/LoadBalance.java index e219897c6..5a6eec042 100644 --- a/client/src/main/java/org/asynchttpclient/LoadBalance.java +++ b/client/src/main/java/org/asynchttpclient/LoadBalance.java @@ -68,6 +68,12 @@ public enum LoadBalance { * {@code maxConnectionsPerHost} at least as large as the resolved-IP count for full per-IP * spreading. The default is unlimited, so this bound only applies when a finite per-host limit is * configured. + *
  • Since the per-host permit is held for the connection's lifetime, each live round-robin HTTP/2 + * connection also occupies a global {@code maxConnections} slot until it closes, and pool + * {@code connectionTtl}/idle reaping do not apply (these connections live in the HTTP/2 registry, + * kept alive by PING, until GOAWAY or the connection drops). When a GOAWAY starts draining a + * connection its permit is released immediately, so a replacement can be opened without waiting + * for in-flight streams to finish.
  • *
  • The address order comes straight from the configured * {@link io.netty.resolver.InetNameResolver}; this mode does not re-sort it. For the * rotation to map consistently across requests, use a resolver that returns the addresses diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java index 151ca7d05..e047d215e 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java @@ -932,6 +932,12 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception if (pk != null) { removeHttp2Connection(pk, ctx.channel()); } + // Free the round-robin per-host permit at drain start instead of at channel close. + // A draining connection rejects new streams but can stay open while in-flight + // streams finish, and holding the permit that whole time blocks a replacement + // connection (issue #2214). Once-only: the closeFuture release becomes a no-op, + // and in DEFAULT mode no hook is installed so this does nothing. + connState.releasePermitOnce(); // Fail requests still queued for a stream slot: a draining connection accepts no // new streams, so they can never be opened here and would otherwise wait until the // connection finally closes. Fail them now so they retry on a fresh connection (the diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/Http2ConnectionState.java b/client/src/main/java/org/asynchttpclient/netty/channel/Http2ConnectionState.java index d1dee9e0a..580ab0799 100644 --- a/client/src/main/java/org/asynchttpclient/netty/channel/Http2ConnectionState.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/Http2ConnectionState.java @@ -23,6 +23,7 @@ import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; /** @@ -75,6 +76,11 @@ private static final class PendingOpener { // opening request from acquiring a slot. private final AtomicBoolean redundant = new AtomicBoolean(false); private volatile Object partitionKey; + // Releases the per-host permit this connection holds in ROUND_ROBIN mode (installed by + // NettyConnectListener, absent in DEFAULT mode). The GOAWAY handler and the channel closeFuture race + // to fire it; getAndSet(null) makes it run at most once, since a double release would push the + // semaphore above maxConnectionsPerHost. + private final AtomicReference permitRelease = new AtomicReference<>(); public boolean tryAcquireStream() { if (draining.get() || closed.get()) { @@ -264,4 +270,24 @@ public void setPartitionKey(Object partitionKey) { public Object getPartitionKey() { return partitionKey; } + + /** + * Installs the action that releases this connection's per-host permit (round-robin mode only). + * {@link #releasePermitOnce()} runs it at most once. + */ + public void setPermitRelease(Runnable release) { + permitRelease.set(release); + } + + /** + * Runs the installed permit-release action the first time it is called; later calls do nothing, as do + * calls when no action was installed (DEFAULT mode). Fired at drain start (GOAWAY) and on channel + * close, whichever comes first. + */ + public void releasePermitOnce() { + Runnable release = permitRelease.getAndSet(null); + if (release != null) { + release.run(); + } + } } diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java b/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java index 2a9f21800..9f0f75bd1 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java @@ -283,10 +283,23 @@ private void releaseSemaphoreImmediately(Object partitionKeyLock) { */ private void registerHttp2AndManageSemaphore(Channel channel, Object partitionKeyLock) { // Register under the future's partition key so the H2 connection is found by the same key the - // pool is polled with — including the IP-aware key used by LoadBalance.ROUND_ROBIN. - channelManager.registerHttp2Connection(future.getPartitionKey(), channel); - if (future.getPartitionKey() instanceof RoundRobinPartitionKey) { - attachSemaphoreToChannelClose(channel, partitionKeyLock); + // pool is polled with, including the IP-aware key used by LoadBalance.ROUND_ROBIN. Read the key + // once: it is volatile (repinned on IP failover) and both uses below must agree. + Object partitionKey = future.getPartitionKey(); + channelManager.registerHttp2Connection(partitionKey, channel); + if (partitionKey instanceof RoundRobinPartitionKey) { + // The permit must be freed as soon as the connection stops serving new requests: on close, or + // at drain start after GOAWAY (issue #2214). Both paths go through releasePermitOnce(), which + // guarantees a single release; a double release would push the semaphore above the cap. The + // state attribute is always present after upgradePipelineToHttp2, the fallback is just + // belt and braces. + Http2ConnectionState state = channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); + if (state != null && connectionSemaphore != null && partitionKeyLock != null) { + state.setPermitRelease(() -> connectionSemaphore.releaseChannelLock(partitionKeyLock)); + channel.closeFuture().addListener(f -> state.releasePermitOnce()); + } else { + attachSemaphoreToChannelClose(channel, partitionKeyLock); + } } else { releaseSemaphoreImmediately(partitionKeyLock); } diff --git a/client/src/test/java/org/asynchttpclient/BasicHttp2Test.java b/client/src/test/java/org/asynchttpclient/BasicHttp2Test.java index 3ec04899d..9e97a7b42 100644 --- a/client/src/test/java/org/asynchttpclient/BasicHttp2Test.java +++ b/client/src/test/java/org/asynchttpclient/BasicHttp2Test.java @@ -614,6 +614,50 @@ public void http2RoundRobinCapsLiveConnectionsAtMaxConnectionsPerHost() throws E } } + /** + * Issue #2214 drain-permit fix: with {@code maxConnectionsPerHost=1} a single live round-robin + * connection saturates the per-host cap, so its permit must be released when the server's GOAWAY starts + * draining it, not when it finally closes. The first request is a long-running stream that keeps the + * connection open across the drain (the GOAWAY carries a high lastStreamId so the stream survives it, + * and the request is never awaited); the second request must then open a replacement connection promptly + * instead of failing with {@code TooManyConnectionsPerHostException} after stalling for + * {@code connectTimeout}. + */ + @Test + public void http2RoundRobinGoawayReleasesPermitForReplacementConnection() throws Exception { + io.netty.resolver.NameResolver resolver = multiIpResolver("127.0.0.1", "127.0.0.2"); + try (AsyncHttpClient client = http2ClientWithConfig(b -> b.setLoadBalance(LoadBalance.ROUND_ROBIN) + .setMaxConnectionsPerHost(1).setMaxRequestRetry(0) + .setConnectTimeout(Duration.ofSeconds(1)).setRequestTimeout(Duration.ofSeconds(60)))) { + + // Long-running stream that keeps its connection (and, before the fix, the only permit) busy. + client.executeRequest(org.asynchttpclient.Dsl.get(httpsUrl("/delay/30000")).setNameResolver(resolver)); + + // Wait until the server has accepted the connection. + long deadline = System.currentTimeMillis() + 5000; + while (serverChildChannels.size() < 1 && System.currentTimeMillis() < deadline) { + Thread.sleep(20); + } + assertEquals(1, serverChildChannels.size(), "exactly one HTTP/2 connection should be established"); + Thread.sleep(300); + + // GOAWAY with a high lastStreamId leaves the in-flight stream running, so the connection + // stays open and draining. + Channel parent = serverChildChannels.iterator().next(); + parent.writeAndFlush(new io.netty.handler.codec.http2.DefaultHttp2GoAwayFrame(Http2Error.NO_ERROR) + .setExtraStreamIds(1000)).sync(); + Thread.sleep(300); + + // Must open a replacement connection; before the fix this failed with + // TooManyConnectionsPerHostException because the draining connection still held the permit. + Response replacement = client.executeRequest( + org.asynchttpclient.Dsl.get(httpsUrl("/ok")).setNameResolver(resolver)).get(10, SECONDS); + assertEquals(200, replacement.getStatusCode(), + "a request after GOAWAY must open a replacement connection, not fail with " + + "TooManyConnectionsPerHostException while the draining connection pins the permit"); + } + } + /** * Creates an AHC client with a specific request timeout. */ diff --git a/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2DrainPermitTest.java b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2DrainPermitTest.java new file mode 100644 index 000000000..f9dc9d467 --- /dev/null +++ b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2DrainPermitTest.java @@ -0,0 +1,212 @@ +/* + * Copyright (c) 2026 AsyncHttpClient Project. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.asynchttpclient.netty.channel; + +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.util.HashedWheelTimer; +import io.netty.util.Timer; +import org.asynchttpclient.AsyncHttpClientConfig; +import org.asynchttpclient.exception.TooManyConnectionsException; +import org.asynchttpclient.exception.TooManyConnectionsPerHostException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.net.InetAddress; + +import static org.asynchttpclient.Dsl.config; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression tests for the round-robin GOAWAY drain-permit fix (issue #2214): the per-host permit held for + * an HTTP/2 connection's lifetime must be released when the connection starts draining, not when it finally + * closes, and released exactly once across the GOAWAY path and the channel closeFuture. The wiring mirrors + * {@link NettyConnectListener}'s round-robin branch and {@link ChannelManager}'s GOAWAY handler, without + * network I/O; the end-to-end path over a real socket is covered by {@code BasicHttp2Test}. + */ +class ChannelManagerHttp2DrainPermitTest { + + private static final String BASE = "https://host:443"; + + private ChannelManager channelManager; + private Timer timer; + + @BeforeEach + void setUp() { + AsyncHttpClientConfig cfg = config().build(); + timer = new HashedWheelTimer(); + channelManager = new ChannelManager(cfg, timer); + } + + @AfterEach + void tearDown() { + if (channelManager != null) { + channelManager.close(); + } + if (timer != null) { + timer.stop(); + } + } + + private static RoundRobinPartitionKey rrKey(String base, String ip) throws Exception { + return new RoundRobinPartitionKey(base, InetAddress.getByName(ip)); + } + + /** + * Registers an active round-robin HTTP/2 connection holding a per-host permit, wired as + * {@link NettyConnectListener} does: permit keyed by the base key, connection registered under the + * IP-aware key, release hook plus closeFuture listener installed. + */ + private EmbeddedChannel registerRrConnectionHoldingPermit(ConnectionSemaphore semaphore, Object baseKey, + Object registryKey) { + EmbeddedChannel channel = new EmbeddedChannel(); + Http2ConnectionState state = new Http2ConnectionState(); + channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).set(state); + channelManager.registerHttp2Connection(registryKey, channel); + // Mirror NettyConnectListener.registerHttp2AndManageSemaphore's round-robin branch. + state.setPermitRelease(() -> semaphore.releaseChannelLock(baseKey)); + channel.closeFuture().addListener(f -> state.releasePermitOnce()); + return channel; + } + + /** Available per-host permits for {@code baseKey} under a {@link PerHostConnectionSemaphore}. */ + private static int availablePerHost(PerHostConnectionSemaphore semaphore, Object baseKey) { + return semaphore.getFreeConnectionsForHost(baseKey).availablePermits(); + } + + /** + * GOAWAY on a still-open connection must free the per-host permit at drain start, and the later + * channel close must not release a second one. + */ + @Test + void goawayReleasesPerHostPermitImmediatelyAndCloseDoesNotDoubleRelease() throws Exception { + PerHostConnectionSemaphore semaphore = new PerHostConnectionSemaphore(1, 0); + RoundRobinPartitionKey registryKey = rrKey(BASE, "127.0.0.1"); + semaphore.acquireChannelLock(BASE); + assertEquals(0, availablePerHost(semaphore, BASE), "permit is held after acquire"); + + EmbeddedChannel channel = registerRrConnectionHoldingPermit(semaphore, BASE, registryKey); + Http2ConnectionState state = channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); + // An in-flight stream keeps the connection open through the drain. + assertTrue(state.tryAcquireStream()); + + // Simulate GOAWAY exactly as ChannelManager's handler does. + state.setDraining(0); + channelManager.removeHttp2Connection(state.getPartitionKey(), channel); + state.releasePermitOnce(); + + assertEquals(1, availablePerHost(semaphore, BASE), + "the per-host permit must be freed at drain start, while the channel is still open"); + assertTrue(channel.isOpen(), "the draining connection is still open (its stream is in flight)"); + assertNull(channelManager.pollHttp2SiblingConnection(BASE), "a draining connection is not offered for reuse"); + + // The in-flight stream ends and the channel closes: closeFuture must NOT release a second permit. + state.releaseStream(); + channel.close().sync(); + assertEquals(1, availablePerHost(semaphore, BASE), + "channel close after a GOAWAY drain must not release a second permit (never exceed the cap)"); + } + + /** + * A connection that closes without ever receiving a GOAWAY (normal drop, TCP reset) must still release + * its permit exactly once, via the closeFuture; a stray later releasePermitOnce is a no-op. + */ + @Test + void closeWithoutGoawayReleasesPermitExactlyOnce() throws Exception { + PerHostConnectionSemaphore semaphore = new PerHostConnectionSemaphore(1, 0); + RoundRobinPartitionKey registryKey = rrKey(BASE, "127.0.0.1"); + semaphore.acquireChannelLock(BASE); + + EmbeddedChannel channel = registerRrConnectionHoldingPermit(semaphore, BASE, registryKey); + Http2ConnectionState state = channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); + assertEquals(0, availablePerHost(semaphore, BASE)); + + channel.close().sync(); + assertEquals(1, availablePerHost(semaphore, BASE), "close releases the permit once"); + + // A late GOAWAY-style release (or any extra call) must not push the semaphore above the cap. + state.releasePermitOnce(); + assertEquals(1, availablePerHost(semaphore, BASE), "a second release must be a no-op"); + } + + /** + * With a per-host cap of 1, a new connection to the same host must be admissible again once the drain + * releases the permit; without the fix the draining connection pins the only permit and a replacement + * fails with {@link TooManyConnectionsPerHostException}. + */ + @Test + void replacementConnectionIsAdmissibleAfterDrainRelease() throws Exception { + PerHostConnectionSemaphore semaphore = new PerHostConnectionSemaphore(1, 0); + RoundRobinPartitionKey registryKey = rrKey(BASE, "127.0.0.1"); + semaphore.acquireChannelLock(BASE); + + EmbeddedChannel channel = registerRrConnectionHoldingPermit(semaphore, BASE, registryKey); + Http2ConnectionState state = channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); + assertTrue(state.tryAcquireStream()); // keep it open through the drain + + // Before the fix, the permit is still held here, so this acquire fails. + assertThrows(TooManyConnectionsPerHostException.class, () -> semaphore.acquireChannelLock(BASE), + "sanity: the cap is exhausted while the connection holds its permit"); + + // GOAWAY drain releases the permit. + state.setDraining(0); + channelManager.removeHttp2Connection(state.getPartitionKey(), channel); + state.releasePermitOnce(); + + // A replacement connection can now be opened immediately, without waiting for the drain to finish. + assertDoesNotThrow(() -> semaphore.acquireChannelLock(BASE), + "after the drain release a replacement connection must be admissible"); + + state.releaseStream(); + channel.close().sync(); + } + + /** + * With a combined limiter the round-robin permit pins both a per-host and a global slot. Releasing it + * at drain start must free the global slot too, so unrelated hosts are not starved. + */ + @Test + void goawayReleasesGlobalSlotSoUnrelatedHostIsNotStarved() throws Exception { + // Global cap 1, per-host cap 1: a single live round-robin connection saturates the global limit. + CombinedConnectionSemaphore semaphore = new CombinedConnectionSemaphore(1, 1, 0); + Object otherHost = "https://other:443"; + RoundRobinPartitionKey registryKey = rrKey(BASE, "127.0.0.1"); + semaphore.acquireChannelLock(BASE); + + EmbeddedChannel channel = registerRrConnectionHoldingPermit(semaphore, BASE, registryKey); + Http2ConnectionState state = channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); + assertTrue(state.tryAcquireStream()); // keep it open through the drain + + // The global slot is pinned: even an unrelated host cannot connect. + assertThrows(TooManyConnectionsException.class, () -> semaphore.acquireChannelLock(otherHost), + "sanity: the global cap is exhausted while the draining connection holds its slot"); + + // GOAWAY drain releases the combined (global + per-host) permit. + state.setDraining(0); + channelManager.removeHttp2Connection(state.getPartitionKey(), channel); + state.releasePermitOnce(); + + assertDoesNotThrow(() -> semaphore.acquireChannelLock(otherHost), + "releasing the draining connection's permit must free the global slot for an unrelated host"); + + state.releaseStream(); + channel.close().sync(); + } +} diff --git a/client/src/test/java/org/asynchttpclient/netty/channel/Http2ConnectionStateTest.java b/client/src/test/java/org/asynchttpclient/netty/channel/Http2ConnectionStateTest.java index a3ae6ed84..ed7a051c2 100644 --- a/client/src/test/java/org/asynchttpclient/netty/channel/Http2ConnectionStateTest.java +++ b/client/src/test/java/org/asynchttpclient/netty/channel/Http2ConnectionStateTest.java @@ -831,4 +831,70 @@ public void voidAddPendingOpenerWrapperStillQueuesAndRuns() { state.releaseStream(); assertEquals(1, ran.get(), "the void addPendingOpener wrapper must delegate to offerPendingOpener"); } + + // ------------------------------------------------------------------------- + // Once-only permit release (round-robin GOAWAY drain permit, issue #2214) + // ------------------------------------------------------------------------- + + @Test + public void releasePermitOnceIsNoOpWhenNoHookInstalled() { + // DEFAULT mode never installs a hook; releasePermitOnce must be a safe no-op there. + Http2ConnectionState state = new Http2ConnectionState(); + assertDoesNotThrow(state::releasePermitOnce); + } + + @Test + public void releasePermitOnceRunsHookExactlyOnce() { + // The hook must fire on the first call and never again: the GOAWAY handler and the channel + // closeFuture both call releasePermitOnce. + Http2ConnectionState state = new Http2ConnectionState(); + AtomicInteger releases = new AtomicInteger(0); + state.setPermitRelease(releases::incrementAndGet); + + state.releasePermitOnce(); + assertEquals(1, releases.get(), "first releasePermitOnce must run the hook"); + + state.releasePermitOnce(); + state.releasePermitOnce(); + assertEquals(1, releases.get(), "subsequent releasePermitOnce calls must be no-ops"); + } + + @Test + public void releasePermitOnceIsAtomicUnderConcurrency() throws InterruptedException { + // Many threads call releasePermitOnce at once; the hook must run exactly once total. + int rounds = 2000; + AtomicInteger totalReleases = new AtomicInteger(0); + int numThreads = 8; + ExecutorService executor = Executors.newFixedThreadPool(numThreads); + try { + for (int r = 0; r < rounds; r++) { + Http2ConnectionState state = new Http2ConnectionState(); + AtomicInteger releasesThisRound = new AtomicInteger(0); + state.setPermitRelease(releasesThisRound::incrementAndGet); + + CyclicBarrier barrier = new CyclicBarrier(numThreads); + CountDownLatch done = new CountDownLatch(numThreads); + for (int t = 0; t < numThreads; t++) { + executor.submit(() -> { + try { + barrier.await(); + state.releasePermitOnce(); + } catch (Exception ignored) { + // barrier interruption is not expected in this test + } finally { + done.countDown(); + } + }); + } + assertTrue(done.await(10, TimeUnit.SECONDS)); + assertEquals(1, releasesThisRound.get(), + "releasePermitOnce must run the hook exactly once even under concurrent callers"); + totalReleases.addAndGet(releasesThisRound.get()); + } + } finally { + executor.shutdown(); + assertTrue(executor.awaitTermination(30, TimeUnit.SECONDS)); + } + assertEquals(rounds, totalReleases.get(), "exactly one release per round"); + } }