From 35b4af84b1056a90ecbe745ff90920ebfa57bd32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Sautter?= Date: Fri, 20 Dec 2024 15:19:45 +0100 Subject: [PATCH] [grid] rework the retry of http requests #14917 --- .../openqa/selenium/remote/http/BUILD.bazel | 2 - .../selenium/remote/http/RetryRequest.java | 103 ++++++------------ .../remote/http/RetryRequestTest.java | 102 +++++++++++++++-- 3 files changed, 124 insertions(+), 83 deletions(-) diff --git a/java/src/org/openqa/selenium/remote/http/BUILD.bazel b/java/src/org/openqa/selenium/remote/http/BUILD.bazel index 9cb037c5b2e5a..e3a780dbbbbec 100644 --- a/java/src/org/openqa/selenium/remote/http/BUILD.bazel +++ b/java/src/org/openqa/selenium/remote/http/BUILD.bazel @@ -1,4 +1,3 @@ -load("@rules_jvm_external//:defs.bzl", "artifact") load("//java:defs.bzl", "java_export") load("//java:version.bzl", "SE_VERSION") @@ -20,6 +19,5 @@ java_export( "//java:auto-service", "//java/src/org/openqa/selenium:core", "//java/src/org/openqa/selenium/json", - artifact("dev.failsafe:failsafe"), ], ) diff --git a/java/src/org/openqa/selenium/remote/http/RetryRequest.java b/java/src/org/openqa/selenium/remote/http/RetryRequest.java index c9cb8f8883dbd..b75259a401e94 100644 --- a/java/src/org/openqa/selenium/remote/http/RetryRequest.java +++ b/java/src/org/openqa/selenium/remote/http/RetryRequest.java @@ -17,89 +17,54 @@ package org.openqa.selenium.remote.http; -import static java.net.HttpURLConnection.HTTP_CLIENT_TIMEOUT; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; -import static org.openqa.selenium.internal.Debug.getDebugLogLevel; -import static org.openqa.selenium.remote.http.Contents.asJson; -import dev.failsafe.Failsafe; -import dev.failsafe.Fallback; -import dev.failsafe.RetryPolicy; -import dev.failsafe.event.ExecutionAttemptedEvent; -import dev.failsafe.function.CheckedFunction; import java.net.ConnectException; -import java.util.Map; +import java.util.logging.Level; import java.util.logging.Logger; +import org.openqa.selenium.internal.Debug; public class RetryRequest implements Filter { private static final Logger LOG = Logger.getLogger(RetryRequest.class.getName()); + private static final Level LOG_LEVEL = Debug.getDebugLogLevel(); - private static final Fallback fallback = - Fallback.of( - (CheckedFunction, ? extends HttpResponse>) - RetryRequest::getFallback); + @Override + public HttpHandler apply(HttpHandler next) { + return req -> { + int retriesOnConnectException = 3; + int retriesOnServerError = 2; + int maxAttempts = Math.max(retriesOnConnectException, retriesOnServerError) + 1; - // Retry on connection error. - private static final RetryPolicy connectionFailurePolicy = - RetryPolicy.builder() - .handleIf(failure -> failure.getCause() instanceof ConnectException) - .withMaxRetries(3) - .onRetry( - e -> - LOG.log( - getDebugLogLevel(), - "Connection failure #{0}. Retrying.", - e.getAttemptCount())) - .build(); + for (int i = 0; i < maxAttempts; i++) { + HttpResponse response; - // Retry if server is unavailable or an internal server error occurs without response body. - private static final RetryPolicy serverErrorPolicy = - RetryPolicy.builder() - .handleResultIf( - response -> - response.getStatus() == HTTP_INTERNAL_ERROR - && Integer.parseInt((response).getHeader(HttpHeader.ContentLength.getName())) - == 0) - .handleResultIf(response -> (response).getStatus() == HTTP_UNAVAILABLE) - .withMaxRetries(2) - .onRetry( - e -> - LOG.log( - getDebugLogLevel(), - "Failure due to server error #{0}. Retrying.", - e.getAttemptCount())) - .build(); + try { + response = next.execute(req); + } catch (RuntimeException ex) { + if (ex.getCause() instanceof ConnectException && i < retriesOnConnectException) { + LOG.log(LOG_LEVEL, "Retry #" + (i + 1) + " on ConnectException", ex); + continue; + } - @Override - public HttpHandler apply(HttpHandler next) { - return req -> - Failsafe.with(fallback) - .compose(serverErrorPolicy) - .compose(connectionFailurePolicy) - .get(() -> next.execute(req)); - } + throw ex; + } + + boolean isServerError = + (response.getStatus() == HTTP_INTERNAL_ERROR && response.getContent().length() == 0) + || response.getStatus() == HTTP_UNAVAILABLE; - private static HttpResponse getFallback( - ExecutionAttemptedEvent executionAttemptedEvent) throws Exception { - if (executionAttemptedEvent.getLastException() != null) { - Exception exception = (Exception) executionAttemptedEvent.getLastException(); - if (exception.getCause() instanceof ConnectException) { - return new HttpResponse() - .setStatus(HTTP_CLIENT_TIMEOUT) - .setContent(asJson(Map.of("value", Map.of("message", "Connection failure")))); - } else throw exception; - } else if (executionAttemptedEvent.getLastResult() != null) { - HttpResponse response = executionAttemptedEvent.getLastResult(); - if ((response.getStatus() == HTTP_INTERNAL_ERROR - && Integer.parseInt(response.getHeader(HttpHeader.ContentLength.getName())) == 0) - || response.getStatus() == HTTP_UNAVAILABLE) { - return new HttpResponse() - .setStatus(response.getStatus()) - .setContent(asJson(Map.of("value", Map.of("message", "Internal server error")))); + if (isServerError && i < retriesOnServerError) { + LOG.log(LOG_LEVEL, "Retry #" + (i + 1) + " on ServerError: " + response.getStatus()); + continue; + } + + return response; } - } - return executionAttemptedEvent.getLastResult(); + + throw new IllegalStateException( + "Effective unreachable code reached, please raise a bug ticket."); + }; } } diff --git a/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java b/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java index de648f5301ce7..a2fd87ccb020e 100644 --- a/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java +++ b/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java @@ -17,7 +17,6 @@ package org.openqa.selenium.remote.http; -import static java.net.HttpURLConnection.HTTP_CLIENT_TIMEOUT; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; import static java.net.HttpURLConnection.HTTP_OK; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; @@ -26,6 +25,9 @@ import static org.openqa.selenium.remote.http.HttpMethod.GET; import com.google.common.collect.ImmutableMap; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.ConnectException; import java.net.MalformedURLException; import java.net.URI; import java.time.Duration; @@ -37,6 +39,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -73,6 +76,29 @@ void canThrowUnexpectedException() { UnsupportedOperationException.class, () -> handler.execute(new HttpRequest(GET, "/"))); } + @Test + void noUnexpectedRetry() { + AtomicInteger count = new AtomicInteger(); + HttpHandler handler = + new RetryRequest() + .andFinally( + (HttpRequest request) -> { + if (count.getAndIncrement() == 0) { + throw new StackOverflowError("Testing"); + } else { + throw new UncheckedIOException("More testing", new IOException()); + } + }); + + Assertions.assertThrows( + StackOverflowError.class, () -> handler.execute(new HttpRequest(GET, "/"))); + Assertions.assertEquals(1, count.get()); + + Assertions.assertThrows( + UncheckedIOException.class, () -> handler.execute(new HttpRequest(GET, "/"))); + Assertions.assertEquals(2, count.get()); + } + @Test void canReturnAppropriateFallbackResponse() { HttpHandler handler1 = @@ -106,17 +132,23 @@ void canReturnAppropriateFallbackResponseWithMultipleThreads() new RetryRequest() .andFinally((HttpRequest request) -> new HttpResponse().setStatus(HTTP_UNAVAILABLE)); - ExecutorService executorService = Executors.newFixedThreadPool(2); + ExecutorService executorService = Executors.newFixedThreadPool(3); List> tasks = new ArrayList<>(); - tasks.add(() -> client.execute(connectionTimeoutRequest)); - tasks.add(() -> handler2.execute(new HttpRequest(GET, "/"))); + for (int i = 0; i < 1024; i++) { + tasks.add(() -> client.execute(connectionTimeoutRequest)); + tasks.add(() -> handler2.execute(new HttpRequest(GET, "/"))); + } List> results = executorService.invokeAll(tasks); - Assertions.assertEquals(HTTP_CLIENT_TIMEOUT, results.get(0).get().getStatus()); + for (int i = 0; i < 1024; i++) { + int offset = i * 2; + Assertions.assertThrows(ExecutionException.class, () -> results.get(offset).get()); + Assertions.assertEquals(HTTP_UNAVAILABLE, results.get(offset + 1).get().getStatus()); + } - Assertions.assertEquals(HTTP_UNAVAILABLE, results.get(1).get().getStatus()); + executorService.shutdown(); } @Test @@ -266,13 +298,59 @@ void shouldGetTheErrorResponseOnServerUnavailableError() { @Test void shouldBeAbleToRetryARequestOnConnectionFailure() { - AppServer server = new NettyAppServer(req -> new HttpResponse()); + AtomicInteger count = new AtomicInteger(0); + HttpHandler handler = + new RetryRequest() + .andFinally( + (HttpRequest request) -> { + if (count.getAndIncrement() < 2) { + throw new UncheckedIOException(new ConnectException()); + } else { + return new HttpResponse(); + } + }); - URI uri = URI.create(server.whereIs("/")); - HttpRequest request = - new HttpRequest(GET, String.format(REQUEST_PATH, uri.getHost(), uri.getPort())); + HttpRequest request = new HttpRequest(GET, "/"); + HttpResponse response = handler.execute(request); - HttpResponse response = client.execute(request); - assertThat(response).extracting(HttpResponse::getStatus).isEqualTo(HTTP_CLIENT_TIMEOUT); + assertThat(response).extracting(HttpResponse::getStatus).isEqualTo(HTTP_OK); + assertThat(count.get()).isEqualTo(3); + } + + @Test + void shouldRethrowOnConnectFailure() { + AtomicInteger count = new AtomicInteger(0); + AtomicReference lastThrown = new AtomicReference<>(); + HttpHandler handler = + new RetryRequest() + .andFinally( + (HttpRequest request) -> { + count.getAndIncrement(); + lastThrown.set(new UncheckedIOException(new ConnectException())); + throw lastThrown.get(); + }); + + UncheckedIOException thrown = + Assertions.assertThrows( + UncheckedIOException.class, () -> handler.execute(new HttpRequest(GET, "/"))); + assertThat(thrown).isSameAs(lastThrown.get()); + assertThat(count.get()).isEqualTo(4); + } + + @Test + void shouldDeliverUnmodifiedServerErrors() { + AtomicInteger count = new AtomicInteger(0); + AtomicReference lastResponse = new AtomicReference<>(); + HttpHandler handler = + new RetryRequest() + .andFinally( + (HttpRequest request) -> { + count.getAndIncrement(); + lastResponse.set(new HttpResponse().setStatus(500)); + return lastResponse.get(); + }); + + assertThat(handler.execute(new HttpRequest(GET, "/"))).isSameAs(lastResponse.get()); + assertThat(count.get()).isEqualTo(3); } }