From 00b0e3a4d6da65a04064f9b170e2046204287446 Mon Sep 17 00:00:00 2001 From: Pavel Ptashyts <49400901+pavel-ptashyts@users.noreply.github.com> Date: Thu, 2 Jul 2026 12:41:15 +0200 Subject: [PATCH] Schedule the round-robin request timeout once, not twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In ROUND_ROBIN mode every request resolves up front (it needs the resolved IP to key the pool), and resolveAddresses scheduled the request timeout there. On a pooled hit the reuse path (sendRequestWithOpenChannel) then scheduled it again, and the second schedule cancelled the first (NettyResponseFuture.setTimeoutsHolder) — a redundant TimeoutsHolder allocation plus a timer-wheel insert and cancel on every keep-alive round-robin request. Make the up-front resolveAddresses skip scheduling (new scheduleTimeout=false argument) and let the reuse-or-connect path schedule exactly once: sendRequestWithOpenChannel for a pooled hit (unchanged), or the round-robin branch of sendRequestWithNewChannel for a new connection (added). DEFAULT mode is unchanged — its new-channel resolveAddresses still passes true. Redirect/retry per-hop timeout reset is preserved, since each path still schedules a fresh holder per attempt. One behavioural nuance: in ROUND_ROBIN the request timeout no longer bounds the up-front DNS resolve itself (only the reuse/connect that follows). That resolve is a cache hit in the steady state, and an uncached one is still bounded by the resolver's own timeout. Adds a RoundRobinSendTypeTest case asserting a round-robin request still hits the request timeout (guarding the new-connection path's schedule); existing round-robin send/failover/redirect and load-balance config tests pass unchanged. No public API change (resolveAddresses is private). Fixes finding #6. --- .../netty/request/NettyRequestSender.java | 37 +++++++++++----- .../RoundRobinSendTypeTest.java | 42 +++++++++++++++++++ 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java index a92789fb7..937565f40 100755 --- a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java +++ b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java @@ -202,11 +202,11 @@ private ListenableFuture sendRequestRoundRobin(Request request, AsyncHand // Round-robin resolves up front — before the pool check and before the per-host semaphore — so // every eligible request resolves first, even one that immediately reuses a pooled connection and - // even a single-IP host. With a caching resolver this is cheap. One side effect on a pooled hit: - // the request timeout is scheduled here (in resolveAddresses) and again in - // sendRequestWithOpenChannel; the second schedule cancels the first (see - // NettyResponseFuture.setTimeoutsHolder), so it's redundant work, not a leak. - resolveAddresses(request, proxyServer, newFuture, asyncHandler).addListener(new SimpleFutureListener>() { + // even a single-IP host. With a caching resolver this is cheap. Pass scheduleTimeout=false: the + // reuse-or-connect path reached from dispatchResolved schedules the request timeout exactly once + // (sendRequestWithOpenChannel for a pooled hit, or the round-robin branch of sendRequestWithNewChannel + // for a new connection), rather than scheduling it here and then cancelling it on a pooled hit. + resolveAddresses(request, proxyServer, newFuture, asyncHandler, false).addListener(new SimpleFutureListener>() { @Override protected void onSuccess(List addresses) { @@ -447,14 +447,17 @@ private ListenableFuture sendRequestWithNewChannel(Request request, Proxy } // In round-robin mode the addresses were already resolved (and rotated) before polling the pool, - // so reuse them directly instead of resolving a second time. + // so reuse them directly instead of resolving a second time. The up-front resolve deliberately did + // NOT schedule the request timeout (see sendRequestRoundRobin), so schedule it here — once — for this + // new-connection path, before connecting. List roundRobinAddresses = future.getRoundRobinAddresses(); if (roundRobinAddresses != null) { + scheduleRequestTimeout(future, roundRobinAddresses.get(0)); connectWithAddresses(request, proxy, future, asyncHandler, roundRobinAddresses); return future; } - resolveAddresses(request, proxy, future, asyncHandler).addListener(new SimpleFutureListener>() { + resolveAddresses(request, proxy, future, asyncHandler, true).addListener(new SimpleFutureListener>() { @Override protected void onSuccess(List addresses) { @@ -494,20 +497,34 @@ private void connectWithAddresses(Request request, ProxyServer proxy, NettyR } } - private Future> resolveAddresses(Request request, ProxyServer proxy, NettyResponseFuture future, AsyncHandler asyncHandler) { + /** + * Resolves the request's remote addresses. When {@code scheduleTimeout} is {@code true} the request + * timeout is scheduled here, before resolution — the behaviour the DEFAULT-mode new-channel path relies + * on so the timeout also bounds DNS. ROUND_ROBIN resolves up front for every request (it needs the IP to + * key the pool) and passes {@code false}: scheduling here and then again on the reuse-or-connect path + * would allocate a {@code TimeoutsHolder} and a timer entry only to cancel them on a pooled hit. Instead + * the chosen path schedules exactly once — {@link #sendRequestWithOpenChannel} for a reuse, or the + * round-robin branch of {@link #sendRequestWithNewChannel} for a new connection. + */ + private Future> resolveAddresses(Request request, ProxyServer proxy, NettyResponseFuture future, + AsyncHandler asyncHandler, boolean scheduleTimeout) { Uri uri = request.getUri(); final Promise> promise = ImmediateEventExecutor.INSTANCE.newPromise(); if (proxy != null && !proxy.isIgnoredForHost(uri.getHost()) && proxy.getProxyType().isHttp()) { int port = ProxyType.HTTPS.equals(proxy.getProxyType()) || uri.isSecured() ? proxy.getSecuredPort() : proxy.getPort(); InetSocketAddress unresolvedRemoteAddress = InetSocketAddress.createUnresolved(proxy.getHost(), port); - scheduleRequestTimeout(future, unresolvedRemoteAddress); + if (scheduleTimeout) { + scheduleRequestTimeout(future, unresolvedRemoteAddress); + } return resolveHostname(request, unresolvedRemoteAddress, asyncHandler); } else { int port = uri.getExplicitPort(); InetSocketAddress unresolvedRemoteAddress = InetSocketAddress.createUnresolved(uri.getHost(), port); - scheduleRequestTimeout(future, unresolvedRemoteAddress); + if (scheduleTimeout) { + scheduleRequestTimeout(future, unresolvedRemoteAddress); + } if (request.getAddress() != null) { // bypass resolution diff --git a/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java b/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java index f6c09d3ac..8b5bffa8d 100644 --- a/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java +++ b/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java @@ -38,6 +38,8 @@ import java.util.List; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; import static java.util.concurrent.TimeUnit.SECONDS; import static org.asynchttpclient.Dsl.asyncHttpClient; @@ -46,6 +48,8 @@ import static org.asynchttpclient.test.TestUtils.TIMEOUT; import static org.asynchttpclient.test.TestUtils.addHttpConnector; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; /** * End-to-end coverage for {@link LoadBalance#ROUND_ROBIN}: a single host that resolves to several @@ -186,6 +190,44 @@ public void roundRobinFailsOverWhenSelectedIpIsDown() throws Exception { } } + @Test + public void roundRobinRequestStillHitsRequestTimeout() throws Exception { + // Regression guard: round-robin resolves up front WITHOUT scheduling the request timeout there; the + // reuse-or-connect path must still schedule it exactly once. Point at a server that never responds + // within the request timeout and assert the request times out (i.e. the new-connection round-robin + // path did schedule the timeout). + Server slow = new Server(); + ServerConnector connector = addHttpConnector(slow); + slow.setHandler(new AbstractHandler() { + @Override + public void handle(String t, Request base, HttpServletRequest req, HttpServletResponse resp) { + base.setHandled(true); + try { + Thread.sleep(4000); // never responds within the 500ms request timeout below + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + }); + slow.start(); + int slowPort = connector.getLocalPort(); + try { + NameResolver resolver = fixedResolver("127.0.0.1"); + try (AsyncHttpClient client = asyncHttpClient(config() + .setLoadBalance(LoadBalance.ROUND_ROBIN) + .setRequestTimeout(java.time.Duration.ofMillis(500)) + .setMaxRequestRetry(0).build())) { + ExecutionException thrown = assertThrows(ExecutionException.class, () -> + client.executeRequest(get("http://roundrobin.test:" + slowPort + "/").setNameResolver(resolver)) + .get(TIMEOUT, SECONDS)); + assertInstanceOf(TimeoutException.class, thrown.getCause(), + "round-robin request must still hit the request timeout; got " + thrown.getCause()); + } + } finally { + slow.stop(); + } + } + @Test public void roundRobinReResolvesAcrossSameHostPortChangingRedirect() throws Exception { // Regression test for the same-base redirect leak: round-robin caches the resolved addresses