Delegates to the configured {@link HxConfig#getRetryCondition()}, which by default - * retries on IOException - typically network-level issues such as connection timeouts, - * connection refused, etc. Override to decide independently of the configuration. + * retries network-level IOExceptions such as connection resets and connect timeouts, but + * not a request timeout that elapsed after the request was sent - see + * {@link HxConfig#defaultRetryCondition(Throwable)}. Override to decide independently of + * the configuration. * * @param throwable the exception that occurred during the request * @return true if the request should be retried, false otherwise @@ -624,7 +631,7 @@ protected boolean shouldRetryOnException(Throwable throwable) { final Predicate condition = config.getRetryCondition(); return condition != null ? condition.test(throwable) - : throwable instanceof IOException; + : HxConfig.defaultRetryCondition(throwable); } /** diff --git a/lib-httpx/src/main/java/io/seqera/http/HxConfig.java b/lib-httpx/src/main/java/io/seqera/http/HxConfig.java index 4af30a9f..6ba1e414 100644 --- a/lib-httpx/src/main/java/io/seqera/http/HxConfig.java +++ b/lib-httpx/src/main/java/io/seqera/http/HxConfig.java @@ -17,10 +17,13 @@ package io.seqera.http; +import java.io.IOException; import java.net.Authenticator; import java.net.CookiePolicy; import java.net.ProxySelector; import java.net.http.HttpClient; +import java.net.http.HttpConnectTimeoutException; +import java.net.http.HttpTimeoutException; import java.time.Duration; import java.util.Set; import java.util.function.Predicate; @@ -42,6 +45,8 @@ *
{@link HttpTimeoutException} extends {@link IOException}, so a plain "retry any + * IOException" rule retries request timeouts and the real bound on a call becomes + * {@code maxAttempts × timeout} instead of {@code timeout}. Such a timeout is also + * ambiguous - the server may have received the request and be processing it - so re-sending + * risks duplicating a non-idempotent operation. {@link HttpConnectTimeoutException} is a + * subclass raised before the request is sent and stays retryable. + * + *
This mirrors the defaults of other clients: OkHttp recovers from a socket timeout only + * when the request has not been sent, and Apache HttpClient 5 treats the whole + * {@code InterruptedIOException} family as non-retriable. The exclusion here is narrower + * than Apache's - it is scoped to the {@link HttpTimeoutException} that {@code java.net.http} + * raises for a request timeout, so a {@code SocketTimeoutException} stays retryable. + * + *
Callers that want timeouts retried can opt back in with + * {@code retryCondition(t -> t instanceof IOException)}, or compose with this rule, for + * example {@code retryCondition(t -> HxConfig.defaultRetryCondition(t) || t instanceof MyEx)}. + * + * @param throwable the exception raised while sending the request + * @return true if the request should be retried, false otherwise + */ + public static boolean defaultRetryCondition(Throwable throwable) { + if (!(throwable instanceof IOException)) + return false; + if (throwable instanceof HttpTimeoutException) + // Retry only a connect timeout. It is raised while establishing the connection, so the + // request never reached the server: nothing ran, re-sending cannot duplicate anything, + // and the next attempt may pick a healthy endpoint. A bare HttpTimeoutException instead + // means the request timeout elapsed after the request was sent - the server may well + // have received it and still be working on it, so re-sending risks running a + // non-idempotent operation twice, and only pushes the worst case out to + // maxAttempts x timeout without making an answer more likely. + return throwable instanceof HttpConnectTimeoutException; + return true; + } private Duration delay = Duration.ofMillis(500); private Duration maxDelay = Duration.ofSeconds(30); diff --git a/lib-httpx/src/test/groovy/io/seqera/http/HxClientRetryIntegrationTest.groovy b/lib-httpx/src/test/groovy/io/seqera/http/HxClientRetryIntegrationTest.groovy index 0b8eee75..73054333 100644 --- a/lib-httpx/src/test/groovy/io/seqera/http/HxClientRetryIntegrationTest.groovy +++ b/lib-httpx/src/test/groovy/io/seqera/http/HxClientRetryIntegrationTest.groovy @@ -22,6 +22,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.* import java.net.http.HttpClient import java.net.http.HttpRequest import java.net.http.HttpResponse +import java.net.http.HttpTimeoutException import java.time.Duration import java.util.concurrent.ExecutionException import java.util.function.Predicate @@ -620,6 +621,65 @@ class HxClientRetryIntegrationTest extends Specification { wireMockServer.verify(1, getRequestedFor(urlEqualTo('/api/subclass-no-retry'))) } + def 'should not retry a request timeout by default'() { + given: 'default retry condition, 3 attempts' + def config = HxConfig.newBuilder() + .maxAttempts(3) + .delay(Duration.ofMillis(50)) + .build() + def client = HxClient.newBuilder().config(config).build() + + and: 'server always responds slower than the request timeout' + wireMockServer.stubFor(get(urlEqualTo('/api/slow-default')) + .willReturn(aResponse().withStatus(200).withFixedDelay(1000))) + + and: + def request = HttpRequest.newBuilder() + .uri(URI.create("http://localhost:${wireMockServer.port()}/api/slow-default")) + .timeout(Duration.ofMillis(150)) + .GET() + .build() + + when: + client.send(request, HttpResponse.BodyHandlers.ofString()) + + then: + thrown(HttpTimeoutException) + + and: 'the call is bounded by its timeout, not by maxAttempts x timeout' + wireMockServer.verify(1, getRequestedFor(urlEqualTo('/api/slow-default'))) + } + + def 'should retry a request timeout when the caller opts back in'() { + given: 'the pre-2.5.0 rule supplied explicitly' + def config = HxConfig.newBuilder() + .maxAttempts(3) + .delay(Duration.ofMillis(50)) + .retryCondition({ Throwable t -> t instanceof IOException } as Predicate) + .build() + def client = HxClient.newBuilder().config(config).build() + + and: 'server always responds slower than the request timeout' + wireMockServer.stubFor(get(urlEqualTo('/api/slow-optin')) + .willReturn(aResponse().withStatus(200).withFixedDelay(1000))) + + and: + def request = HttpRequest.newBuilder() + .uri(URI.create("http://localhost:${wireMockServer.port()}/api/slow-optin")) + .timeout(Duration.ofMillis(150)) + .GET() + .build() + + when: + client.send(request, HttpResponse.BodyHandlers.ofString()) + + then: + thrown(HttpTimeoutException) + + and: 'all 3 attempts are made' + wireMockServer.verify(3, getRequestedFor(urlEqualTo('/api/slow-optin'))) + } + /** * Subclass overriding the documented retry-on-exception extension point */ diff --git a/lib-httpx/src/test/groovy/io/seqera/http/HxConfigTest.groovy b/lib-httpx/src/test/groovy/io/seqera/http/HxConfigTest.groovy index 825dd060..481e25c0 100644 --- a/lib-httpx/src/test/groovy/io/seqera/http/HxConfigTest.groovy +++ b/lib-httpx/src/test/groovy/io/seqera/http/HxConfigTest.groovy @@ -17,8 +17,13 @@ package io.seqera.http +import java.net.ConnectException import java.net.CookiePolicy +import java.net.SocketTimeoutException +import java.net.http.HttpConnectTimeoutException +import java.net.http.HttpTimeoutException import java.time.Duration +import java.util.function.Predicate import io.seqera.util.retry.Retryable import spock.lang.Specification @@ -98,6 +103,61 @@ class HxConfigTest extends Specification { config.retryCondition.test(new Exception('generic exception')) == false } + def 'should apply the default retry condition - #exception.class.simpleName is retried: #expected'() { + expect: + HxConfig.defaultRetryCondition(exception) == expected + + and: 'the condition wired into a default config agrees with the static rule' + HxConfig.newBuilder().build().retryCondition.test(exception) == expected + + where: + exception || expected + // I/O failures are retried + new IOException('io error') || true + new ConnectException('connection refused') || true + new SocketTimeoutException('socket timeout') || true + new FileNotFoundException('not found') || true + // HttpConnectTimeoutException extends HttpTimeoutException: raised before the + // request was sent, so re-sending is safe + new HttpConnectTimeoutException('connect timeout') || true + // HttpTimeoutException extends IOException, which is why a plain + // 'instanceof IOException' rule retried it - the server may have processed the request + new HttpTimeoutException('request timeout') || false + // non-I/O failures are never retried + new RuntimeException('runtime error') || false + new IllegalArgumentException('invalid arg') || false + new NullPointerException('null pointer') || false + new Exception('generic exception') || false + } + + def 'should allow composing with the default retry condition'() { + when: 'the documented composition pattern - the default plus one extra failure' + def config = HxConfig.newBuilder() + .retryCondition({ Throwable t -> + HxConfig.defaultRetryCondition(t) || t instanceof IllegalStateException + } as Predicate) + .build() + + then: 'the extra failure is retried' + config.retryCondition.test(new IllegalStateException('transient')) == true + + and: 'the default rule still applies to everything else' + config.retryCondition.test(new IOException('io error')) == true + config.retryCondition.test(new HttpConnectTimeoutException('connect timeout')) == true + config.retryCondition.test(new HttpTimeoutException('request timeout')) == false + config.retryCondition.test(new RuntimeException('runtime error')) == false + } + + def 'should allow opting back in to retrying request timeouts'() { + when: 'the pre-2.5.0 rule is supplied explicitly' + def config = HxConfig.newBuilder() + .retryCondition({ Throwable t -> t instanceof IOException } as Predicate) + .build() + + then: + config.retryCondition.test(new HttpTimeoutException('request timed out')) == true + } + def 'should create config with custom retry condition'() { given: def customCondition = { throwable -> throwable instanceof IllegalArgumentException }