diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java index 38e4c9182..467e3d9ad 100644 --- a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java +++ b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java @@ -1222,12 +1222,21 @@ public Builder setFailedIpCooldownEnabled(boolean failedIpCooldownEnabled) { /** * @param failedIpCooldownPeriod how long a failed IP is deprioritized before it is re-probed; - * {@code null} resets to the default + * {@code null} resets to the default. Must not be negative; use + * {@link #setFailedIpCooldownEnabled(boolean)} with {@code false} + * to turn the cooldown off instead. * @return this + * @throws IllegalArgumentException if {@code failedIpCooldownPeriod} is negative * @see AsyncHttpClientConfig#getFailedIpCooldownPeriod() */ public Builder setFailedIpCooldownPeriod(Duration failedIpCooldownPeriod) { - this.failedIpCooldownPeriod = failedIpCooldownPeriod == null ? defaultFailedIpCooldownPeriod() : failedIpCooldownPeriod; + if (failedIpCooldownPeriod == null) { + this.failedIpCooldownPeriod = defaultFailedIpCooldownPeriod(); + } else if (failedIpCooldownPeriod.isNegative()) { + throw new IllegalArgumentException("failedIpCooldownPeriod must not be negative: " + failedIpCooldownPeriod); + } else { + this.failedIpCooldownPeriod = failedIpCooldownPeriod; + } return this; } 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 7dc874f67..a62fbcc54 100755 --- a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java +++ b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java @@ -89,6 +89,7 @@ import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.SocketAddress; +import java.time.Duration; import java.util.Iterator; import java.util.List; import java.util.Locale; @@ -132,8 +133,11 @@ public NettyRequestSender(AsyncHttpClientConfig config, ChannelManager channelMa this.nettyTimer = nettyTimer; this.clientState = clientState; requestFactory = new NettyRequestFactory(config); - ipCooldown = config.isFailedIpCooldownEnabled() - ? new FailedIpCooldownHolder(config.getFailedIpCooldownPeriod().toNanos(), System::nanoTime) + // Guard the period against a custom AsyncHttpClientConfig that enables the cooldown but returns a + // null period: leave the cooldown off rather than NPE while constructing the client. + Duration cooldownPeriod = config.getFailedIpCooldownPeriod(); + ipCooldown = config.isFailedIpCooldownEnabled() && cooldownPeriod != null + ? new FailedIpCooldownHolder(cooldownPeriod.toNanos(), System::nanoTime) : null; } diff --git a/client/src/test/java/org/asynchttpclient/FailedIpCooldownConfigTest.java b/client/src/test/java/org/asynchttpclient/FailedIpCooldownConfigTest.java index 94e7b7c34..a47799f24 100644 --- a/client/src/test/java/org/asynchttpclient/FailedIpCooldownConfigTest.java +++ b/client/src/test/java/org/asynchttpclient/FailedIpCooldownConfigTest.java @@ -22,6 +22,7 @@ import static org.asynchttpclient.Dsl.config; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; class FailedIpCooldownConfigTest { @@ -50,6 +51,18 @@ void nullPeriodResetsToDefault() { assertEquals(Duration.ofSeconds(10), config.getFailedIpCooldownPeriod()); } + @Test + void negativePeriodIsRejected() { + assertThrows(IllegalArgumentException.class, () -> config().setFailedIpCooldownPeriod(Duration.ofSeconds(-1))); + } + + @Test + void zeroPeriodIsAccepted() { + // zero is a valid (if degenerate) period — turning the cooldown off is done via setFailedIpCooldownEnabled(false) + AsyncHttpClientConfig config = config().setFailedIpCooldownPeriod(Duration.ZERO).build(); + assertEquals(Duration.ZERO, config.getFailedIpCooldownPeriod()); + } + @Test void copyConstructorPreservesValues() { AsyncHttpClientConfig source = config() diff --git a/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java b/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java index 8b5bffa8d..38060c63f 100644 --- a/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java +++ b/client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java @@ -38,6 +38,7 @@ import java.util.List; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; @@ -132,6 +133,28 @@ public Response onCompleted(Response response) { } } + // Records the IPs a single request targeted, in the order the connector attempted them, so a test can + // assert which IP a new connection tried first (the failed-IP cooldown re-orders that first choice). + private static final class OrderedAttemptRecorder extends AsyncCompletionHandler { + private final List attemptedIps; + + private OrderedAttemptRecorder(List attemptedIps) { + this.attemptedIps = attemptedIps; + } + + @Override + public void onTcpConnectAttempt(InetSocketAddress remoteAddress) { + if (remoteAddress != null && remoteAddress.getAddress() != null) { + attemptedIps.add(remoteAddress.getAddress().getHostAddress()); + } + } + + @Override + public Response onCompleted(Response response) { + return response; + } + } + // Returns the set of IPs the client targeted (first choice plus any failovers) across the requests. private Set runRequestsCapturingTargetedIps(AsyncHttpClientConfig config) throws Exception { Set attemptedIps = ConcurrentHashMap.newKeySet(); @@ -228,6 +251,47 @@ public void handle(String t, Request base, HttpServletRequest req, HttpServletRe } } + @Test + public void defaultModeDeprioritizesAFailedIpOnTheNextConnection() throws Exception { + // The headline behavior this PR adds: the failed-IP cooldown now applies in DEFAULT mode too, so a + // TCP-connect failure to an IP deprioritizes it (moves it to the back) on the next new connection. + // Server bound to 127.0.0.1 only, so 127.0.0.2 has no listener (refused on Linux / unreachable on macOS). + Server localServer = new Server(); + ServerConnector connector = addHttpConnector(localServer); + connector.setHost("127.0.0.1"); + localServer.setHandler(new EchoHandler()); + localServer.start(); + int localPort = connector.getLocalPort(); + try { + // Resolver hands back the dead IP first. keepAlive=false forces a fresh connection per request, + // so every request re-orders and connects (a pooled reuse would never re-resolve). DEFAULT mode + // (no setLoadBalance) — this is what previously always dialed the dead IP first. + NameResolver resolver = fixedResolver("127.0.0.2", "127.0.0.1"); + List firstAttemptPerRequest = new ArrayList<>(); + try (AsyncHttpClient client = asyncHttpClient(config().setKeepAlive(false).setMaxRequestRetry(0).build())) { + for (int i = 0; i < 4; i++) { + List attempts = new CopyOnWriteArrayList<>(); + Response response = client.executeRequest( + get("http://cooldown.test:" + localPort + "/").setNameResolver(resolver), + new OrderedAttemptRecorder(attempts)).get(TIMEOUT, SECONDS); + assertEquals(200, response.getStatusCode(), "request should succeed via failover to the reachable IP"); + firstAttemptPerRequest.add(attempts.get(0)); + } + } + // First request has nothing cooling yet, so it dials the dead IP first (resolver order) then fails over. + assertEquals("127.0.0.2", firstAttemptPerRequest.get(0), + "the first connection follows resolver order because no failure has been recorded yet"); + // Once 127.0.0.2's connect failure is recorded, the cooldown moves it to the back, so every later + // new connection dials the healthy 127.0.0.1 first — the point of extending the cooldown to DEFAULT mode. + for (int i = 1; i < firstAttemptPerRequest.size(); i++) { + assertEquals("127.0.0.1", firstAttemptPerRequest.get(i), + "DEFAULT mode must deprioritize the recently-failed IP on later new connections (request #" + i + ")"); + } + } finally { + localServer.stop(); + } + } + @Test public void roundRobinReResolvesAcrossSameHostPortChangingRedirect() throws Exception { // Regression test for the same-base redirect leak: round-robin caches the resolved addresses