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