diff --git a/java/src/org/openqa/selenium/remote/http/RetryRequest.java b/java/src/org/openqa/selenium/remote/http/RetryRequest.java index 203e374599908..0bc40d590207c 100644 --- a/java/src/org/openqa/selenium/remote/http/RetryRequest.java +++ b/java/src/org/openqa/selenium/remote/http/RetryRequest.java @@ -19,7 +19,6 @@ import static com.google.common.net.HttpHeaders.CONTENT_LENGTH; import static java.net.HttpURLConnection.HTTP_CLIENT_TIMEOUT; -import static java.net.HttpURLConnection.HTTP_GATEWAY_TIMEOUT; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; import static org.openqa.selenium.internal.Debug.getDebugLogLevel; @@ -33,7 +32,6 @@ import dev.failsafe.function.CheckedFunction; import java.net.ConnectException; import java.util.logging.Logger; -import org.openqa.selenium.TimeoutException; public class RetryRequest implements Filter { @@ -57,15 +55,6 @@ public class RetryRequest implements Filter { e.getAttemptCount())) .build(); - // Retry on read timeout. - private static final RetryPolicy readTimeoutPolicy = - RetryPolicy.builder() - .handle(TimeoutException.class) - .withMaxRetries(3) - .onRetry( - e -> LOG.log(getDebugLogLevel(), "Read timeout #{0}. Retrying.", e.getAttemptCount())) - .build(); - // Retry if server is unavailable or an internal server error occurs without response body. private static final RetryPolicy serverErrorPolicy = RetryPolicy.builder() @@ -88,7 +77,6 @@ public HttpHandler apply(HttpHandler next) { return req -> Failsafe.with(fallback) .compose(serverErrorPolicy) - .compose(readTimeoutPolicy) .compose(connectionFailurePolicy) .get(() -> next.execute(req)); } @@ -102,11 +90,6 @@ private static HttpResponse getFallback( .setStatus(HTTP_CLIENT_TIMEOUT) .setContent( asJson(ImmutableMap.of("value", ImmutableMap.of("message", "Connection failure")))); - } else if (exception instanceof TimeoutException) { - return new HttpResponse() - .setStatus(HTTP_GATEWAY_TIMEOUT) - .setContent( - asJson(ImmutableMap.of("value", ImmutableMap.of("message", "Read timeout")))); } else throw exception; } else if (executionAttemptedEvent.getLastResult() != null) { HttpResponse response = executionAttemptedEvent.getLastResult(); diff --git a/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java b/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java index d864368cfe085..de648f5301ce7 100644 --- a/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java +++ b/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java @@ -18,7 +18,6 @@ package org.openqa.selenium.remote.http; import static java.net.HttpURLConnection.HTTP_CLIENT_TIMEOUT; -import static java.net.HttpURLConnection.HTTP_GATEWAY_TIMEOUT; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; import static java.net.HttpURLConnection.HTTP_OK; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; @@ -56,7 +55,8 @@ public void setUp() throws MalformedURLException { ClientConfig.defaultConfig() .baseUrl(URI.create("http://localhost:2345").toURL()) .withRetries() - .readTimeout(Duration.ofSeconds(1)); + .readTimeout(Duration.ofSeconds(1)) + .connectionTimeout(Duration.ofSeconds(1)); client = HttpClient.Factory.createDefault().createClient(config); } @@ -82,8 +82,8 @@ void canReturnAppropriateFallbackResponse() { throw new TimeoutException(); }); - Assertions.assertEquals( - HTTP_GATEWAY_TIMEOUT, handler1.execute(new HttpRequest(GET, "/")).getStatus()); + Assertions.assertThrows( + TimeoutException.class, () -> handler1.execute(new HttpRequest(GET, "/"))); HttpHandler handler2 = new RetryRequest() @@ -96,12 +96,11 @@ void canReturnAppropriateFallbackResponse() { @Test void canReturnAppropriateFallbackResponseWithMultipleThreads() throws InterruptedException, ExecutionException { - HttpHandler handler1 = - new RetryRequest() - .andFinally( - (HttpRequest request) -> { - throw new TimeoutException(); - }); + AppServer server = new NettyAppServer(req -> new HttpResponse()); + + URI uri = URI.create(server.whereIs("/")); + HttpRequest connectionTimeoutRequest = + new HttpRequest(GET, String.format(REQUEST_PATH, uri.getHost(), uri.getPort())); HttpHandler handler2 = new RetryRequest() @@ -110,12 +109,12 @@ void canReturnAppropriateFallbackResponseWithMultipleThreads() ExecutorService executorService = Executors.newFixedThreadPool(2); List> tasks = new ArrayList<>(); - tasks.add(() -> handler1.execute(new HttpRequest(GET, "/"))); + tasks.add(() -> client.execute(connectionTimeoutRequest)); tasks.add(() -> handler2.execute(new HttpRequest(GET, "/"))); List> results = executorService.invokeAll(tasks); - Assertions.assertEquals(HTTP_GATEWAY_TIMEOUT, results.get(0).get().getStatus()); + Assertions.assertEquals(HTTP_CLIENT_TIMEOUT, results.get(0).get().getStatus()); Assertions.assertEquals(HTTP_UNAVAILABLE, results.get(1).get().getStatus()); } @@ -265,70 +264,6 @@ void shouldGetTheErrorResponseOnServerUnavailableError() { server.stop(); } - @Test - void shouldBeAbleToRetryARequestOnTimeout() { - AtomicInteger count = new AtomicInteger(0); - AppServer server = - new NettyAppServer( - req -> { - count.incrementAndGet(); - if (count.get() <= 3) { - try { - Thread.sleep(2000); - } catch (InterruptedException e) { - e.printStackTrace(); - } - } - return new HttpResponse(); - }); - server.start(); - - URI uri = URI.create(server.whereIs("/")); - HttpRequest request = - new HttpRequest(GET, String.format(REQUEST_PATH, uri.getHost(), uri.getPort())); - - HttpResponse response = client.execute(request); - assertThat(response).extracting(HttpResponse::getStatus).isEqualTo(HTTP_OK); - assertThat(count.get()).isEqualTo(4); - - server.stop(); - } - - @Test - void shouldBeAbleToGetErrorResponseOnRequestTimeout() { - AtomicInteger count = new AtomicInteger(0); - AppServer server = - new NettyAppServer( - req -> { - count.incrementAndGet(); - throw new TimeoutException(); - }); - server.start(); - - URI uri = URI.create(server.whereIs("/")); - HttpRequest request = - new HttpRequest(GET, String.format(REQUEST_PATH, uri.getHost(), uri.getPort())); - - HttpResponse response = client.execute(request); - - // The NettyAppServer passes the request through ErrorFilter. - // This maps the timeout exception to HTTP response code 500 and HTTP response body containing - // "timeout". - // RetryRequest retries if it gets a TimeoutException only. - // Parsing and inspecting the response body each time if HTTP response code 500 is not - // efficient. - // A potential solution can be updating the ErrorCodec to reflect the appropriate HTTP code - // (this is a breaking change). - // RetryRequest can then inspect just the HTTP response status code and retry. - - assertThat(response).extracting(HttpResponse::getStatus).isEqualTo(HTTP_INTERNAL_ERROR); - - // This should ideally be more than the number of retries configured i.e. greater than 3 - assertThat(count.get()).isEqualTo(1); - - server.stop(); - } - @Test void shouldBeAbleToRetryARequestOnConnectionFailure() { AppServer server = new NettyAppServer(req -> new HttpResponse());