From 868bd59bf410c33823026868c0243ff18c29434a Mon Sep 17 00:00:00 2001 From: Puja Jagani Date: Fri, 29 Sep 2023 20:16:45 +0530 Subject: [PATCH 1/4] [java] Ensure retry mechanism does not swallow an exception --- .../selenium/remote/http/RetryRequest.java | 80 +++++---- .../remote/http/RetryRequestTest.java | 152 ++++++++++++++++++ 2 files changed, 201 insertions(+), 31 deletions(-) diff --git a/java/src/org/openqa/selenium/remote/http/RetryRequest.java b/java/src/org/openqa/selenium/remote/http/RetryRequest.java index aae3f32f28479..ef2c326558f90 100644 --- a/java/src/org/openqa/selenium/remote/http/RetryRequest.java +++ b/java/src/org/openqa/selenium/remote/http/RetryRequest.java @@ -30,16 +30,62 @@ import dev.failsafe.Fallback; import dev.failsafe.RetryPolicy; import java.net.ConnectException; -import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; import org.openqa.selenium.TimeoutException; public class RetryRequest implements Filter { private static final Logger LOG = Logger.getLogger(RetryRequest.class.getName()); - private static final AtomicReference fallBackResponse = new AtomicReference<>(); - private static final Fallback fallback = Fallback.of(fallBackResponse::get); + private static final Fallback fallback = + Fallback.of( + executionAttemptedEvent -> { + if (executionAttemptedEvent.getLastException() != null) { + Exception exception = (Exception) executionAttemptedEvent.getLastException(); + if (exception.getCause() instanceof ConnectException) { + return new HttpResponse() + .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() instanceof HttpResponse) { + HttpResponse response = (HttpResponse) executionAttemptedEvent.getLastResult(); + if ((response.getStatus() == HTTP_INTERNAL_ERROR + && Integer.parseInt(response.getHeader(CONTENT_LENGTH)) == 0) + || response.getStatus() == HTTP_UNAVAILABLE) { + return new HttpResponse() + .setStatus(response.getStatus()) + .setContent( + asJson( + ImmutableMap.of( + "value", ImmutableMap.of("message", "Internal server error")))); + } + } + return executionAttemptedEvent.getLastResult(); + }); + + private static final Fallback connectionFailureFallback = + Fallback.builder( + e -> { + return new HttpResponse() + .setStatus(HTTP_CLIENT_TIMEOUT) + .setContent( + asJson( + ImmutableMap.of( + "value", ImmutableMap.of("message", "Connection failure")))); + }) + .handleIf(failure -> failure.getCause() instanceof ConnectException) + .build(); // Retry on connection error. private static final RetryPolicy connectionFailurePolicy = @@ -52,15 +98,6 @@ public class RetryRequest implements Filter { getDebugLogLevel(), "Connection failure #{0}. Retrying.", e.getAttemptCount())) - .onRetriesExceeded( - e -> - fallBackResponse.set( - new HttpResponse() - .setStatus(HTTP_CLIENT_TIMEOUT) - .setContent( - asJson( - ImmutableMap.of( - "value", ImmutableMap.of("message", "Connection failure")))))) .build(); // Retry on read timeout. @@ -70,15 +107,6 @@ public class RetryRequest implements Filter { .withMaxRetries(3) .onRetry( e -> LOG.log(getDebugLogLevel(), "Read timeout #{0}. Retrying.", e.getAttemptCount())) - .onRetriesExceeded( - e -> - fallBackResponse.set( - new HttpResponse() - .setStatus(HTTP_GATEWAY_TIMEOUT) - .setContent( - asJson( - ImmutableMap.of( - "value", ImmutableMap.of("message", "Read timeout")))))) .build(); // Retry if server is unavailable or an internal server error occurs without response body. @@ -96,16 +124,6 @@ public class RetryRequest implements Filter { getDebugLogLevel(), "Failure due to server error #{0}. Retrying.", e.getAttemptCount())) - .onRetriesExceeded( - e -> - fallBackResponse.set( - new HttpResponse() - .setStatus(((HttpResponse) e.getResult()).getStatus()) - .setContent( - asJson( - ImmutableMap.of( - "value", - ImmutableMap.of("message", "Internal server error")))))) .build(); @Override diff --git a/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java b/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java index 4b14fdb013bec..810936c668738 100644 --- a/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java +++ b/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java @@ -18,8 +18,10 @@ 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; import static org.assertj.core.api.Assertions.assertThat; import static org.openqa.selenium.remote.http.Contents.asJson; import static org.openqa.selenium.remote.http.HttpMethod.GET; @@ -28,9 +30,18 @@ import java.net.MalformedURLException; import java.net.URI; import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.openqa.selenium.TimeoutException; import org.openqa.selenium.environment.webserver.AppServer; import org.openqa.selenium.environment.webserver.NettyAppServer; import org.openqa.selenium.remote.http.netty.NettyClient; @@ -50,6 +61,66 @@ public void setUp() throws MalformedURLException { client = new NettyClient.Factory().createClient(config); } + @Test + void canThrowUnexpectedException() { + HttpHandler handler = + new RetryRequest() + .andFinally( + (HttpRequest request) -> { + throw new UnsupportedOperationException("Testing"); + }); + + Assertions.assertThrows( + UnsupportedOperationException.class, () -> handler.execute(new HttpRequest(GET, "/"))); + } + + @Test + void canReturnAppropriateFallbackResponse() { + HttpHandler handler1 = + new RetryRequest() + .andFinally( + (HttpRequest request) -> { + throw new TimeoutException(); + }); + + Assertions.assertEquals( + HTTP_GATEWAY_TIMEOUT, handler1.execute(new HttpRequest(GET, "/")).getStatus()); + + HttpHandler handler2 = + new RetryRequest() + .andFinally((HttpRequest request) -> new HttpResponse().setStatus(HTTP_UNAVAILABLE)); + + Assertions.assertEquals( + HTTP_UNAVAILABLE, handler2.execute(new HttpRequest(GET, "/")).getStatus()); + } + + @Test + void canReturnAppropriateFallbackResponseWithMultipleThreads() + throws InterruptedException, ExecutionException { + HttpHandler handler1 = + new RetryRequest() + .andFinally( + (HttpRequest request) -> { + throw new TimeoutException(); + }); + + HttpHandler handler2 = + new RetryRequest() + .andFinally((HttpRequest request) -> new HttpResponse().setStatus(HTTP_UNAVAILABLE)); + + ExecutorService executorService = Executors.newFixedThreadPool(2); + List> tasks = new ArrayList<>(); + + tasks.add(() -> handler1.execute(new HttpRequest(GET, "/"))); + 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_UNAVAILABLE, results.get(1).get().getStatus()); + } + @Test void shouldBeAbleToHandleARequest() { AtomicInteger count = new AtomicInteger(0); @@ -98,6 +169,28 @@ void shouldBeAbleToRetryARequestOnInternalServerError() { server.stop(); } + @Test + void shouldBeAbleToGetTheErrorResponseOnInternalServerError() { + AtomicInteger count = new AtomicInteger(0); + AppServer server = + new NettyAppServer( + req -> { + count.incrementAndGet(); + return new HttpResponse().setStatus(500); + }); + 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_INTERNAL_ERROR); + assertThat(count.get()).isGreaterThanOrEqualTo(3); + + server.stop(); + } + @Test void shouldNotRetryRequestOnInternalServerErrorWithContent() { AtomicInteger count = new AtomicInteger(0); @@ -149,6 +242,30 @@ void shouldRetryRequestOnServerUnavailableError() { server.stop(); } + @Test + void shouldGetTheErrorResponseOnServerUnavailableError() { + AtomicInteger count = new AtomicInteger(0); + AppServer server = + new NettyAppServer( + req -> { + count.incrementAndGet(); + return new HttpResponse() + .setStatus(503) + .setContent(asJson(ImmutableMap.of("error", "server down"))); + }); + 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_UNAVAILABLE); + assertThat(count.get()).isEqualTo(3); + + server.stop(); + } + @Test void shouldBeAbleToRetryARequestOnTimeout() { AtomicInteger count = new AtomicInteger(0); @@ -178,6 +295,41 @@ void shouldBeAbleToRetryARequestOnTimeout() { 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()); From 87ed3427c5743b2aea55e0527c939df7979c6c37 Mon Sep 17 00:00:00 2001 From: Puja Jagani Date: Tue, 3 Oct 2023 15:39:50 +0530 Subject: [PATCH 2/4] [java] Remove unused code --- .../openqa/selenium/remote/http/RetryRequest.java | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/java/src/org/openqa/selenium/remote/http/RetryRequest.java b/java/src/org/openqa/selenium/remote/http/RetryRequest.java index ef2c326558f90..e95c80e69210f 100644 --- a/java/src/org/openqa/selenium/remote/http/RetryRequest.java +++ b/java/src/org/openqa/selenium/remote/http/RetryRequest.java @@ -74,19 +74,6 @@ public class RetryRequest implements Filter { return executionAttemptedEvent.getLastResult(); }); - private static final Fallback connectionFailureFallback = - Fallback.builder( - e -> { - return new HttpResponse() - .setStatus(HTTP_CLIENT_TIMEOUT) - .setContent( - asJson( - ImmutableMap.of( - "value", ImmutableMap.of("message", "Connection failure")))); - }) - .handleIf(failure -> failure.getCause() instanceof ConnectException) - .build(); - // Retry on connection error. private static final RetryPolicy connectionFailurePolicy = RetryPolicy.builder() From e6211c123fb945948c1fb6643f0bb8b4d077cf2d Mon Sep 17 00:00:00 2001 From: Puja Jagani Date: Tue, 3 Oct 2023 17:07:33 +0530 Subject: [PATCH 3/4] [java] Extract fallback construction into a method. Add strong typing. --- .../selenium/remote/http/RetryRequest.java | 90 +++++++++---------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/java/src/org/openqa/selenium/remote/http/RetryRequest.java b/java/src/org/openqa/selenium/remote/http/RetryRequest.java index e95c80e69210f..04f38a87697b2 100644 --- a/java/src/org/openqa/selenium/remote/http/RetryRequest.java +++ b/java/src/org/openqa/selenium/remote/http/RetryRequest.java @@ -29,6 +29,9 @@ 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.logging.Logger; import org.openqa.selenium.TimeoutException; @@ -37,46 +40,12 @@ public class RetryRequest implements Filter { private static final Logger LOG = Logger.getLogger(RetryRequest.class.getName()); - private static final Fallback fallback = - Fallback.of( - executionAttemptedEvent -> { - if (executionAttemptedEvent.getLastException() != null) { - Exception exception = (Exception) executionAttemptedEvent.getLastException(); - if (exception.getCause() instanceof ConnectException) { - return new HttpResponse() - .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() instanceof HttpResponse) { - HttpResponse response = (HttpResponse) executionAttemptedEvent.getLastResult(); - if ((response.getStatus() == HTTP_INTERNAL_ERROR - && Integer.parseInt(response.getHeader(CONTENT_LENGTH)) == 0) - || response.getStatus() == HTTP_UNAVAILABLE) { - return new HttpResponse() - .setStatus(response.getStatus()) - .setContent( - asJson( - ImmutableMap.of( - "value", ImmutableMap.of("message", "Internal server error")))); - } - } - return executionAttemptedEvent.getLastResult(); - }); + private static final Fallback fallback = Fallback.of( + (CheckedFunction, ? extends HttpResponse>) RetryRequest::getFallback); // Retry on connection error. - private static final RetryPolicy connectionFailurePolicy = - RetryPolicy.builder() + private static final RetryPolicy connectionFailurePolicy = + RetryPolicy.builder() .handleIf(failure -> failure.getCause() instanceof ConnectException) .withMaxRetries(3) .onRetry( @@ -88,8 +57,8 @@ public class RetryRequest implements Filter { .build(); // Retry on read timeout. - private static final RetryPolicy readTimeoutPolicy = - RetryPolicy.builder() + private static final RetryPolicy readTimeoutPolicy = + RetryPolicy.builder() .handle(TimeoutException.class) .withMaxRetries(3) .onRetry( @@ -97,13 +66,13 @@ public class RetryRequest implements Filter { .build(); // Retry if server is unavailable or an internal server error occurs without response body. - private static final RetryPolicy serverErrorPolicy = - RetryPolicy.builder() + private static final RetryPolicy serverErrorPolicy = + RetryPolicy.builder() .handleResultIf( response -> - ((HttpResponse) response).getStatus() == HTTP_INTERNAL_ERROR - && Integer.parseInt(((HttpResponse) response).getHeader(CONTENT_LENGTH)) == 0) - .handleResultIf(response -> ((HttpResponse) response).getStatus() == HTTP_UNAVAILABLE) + response.getStatus() == HTTP_INTERNAL_ERROR + && Integer.parseInt((response).getHeader(CONTENT_LENGTH)) == 0) + .handleResultIf(response -> (response).getStatus() == HTTP_UNAVAILABLE) .withMaxRetries(2) .onRetry( e -> @@ -122,4 +91,35 @@ public HttpHandler apply(HttpHandler next) { .compose(connectionFailurePolicy) .get(() -> next.execute(req)); } + + 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(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(); + if ((response.getStatus() == HTTP_INTERNAL_ERROR + && Integer.parseInt(response.getHeader(CONTENT_LENGTH)) == 0) + || response.getStatus() == HTTP_UNAVAILABLE) { + return new HttpResponse() + .setStatus(response.getStatus()) + .setContent( + asJson( + ImmutableMap.of("value", ImmutableMap.of("message", "Internal server error")))); + } + } + return executionAttemptedEvent.getLastResult(); + } } From e5bb4a13c7702b62f58df6afbe4d8ea7f8f17b09 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Thu, 5 Oct 2023 20:15:28 -0500 Subject: [PATCH 4/4] fix linting issues --- .../org/openqa/selenium/remote/http/RetryRequest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/java/src/org/openqa/selenium/remote/http/RetryRequest.java b/java/src/org/openqa/selenium/remote/http/RetryRequest.java index 04f38a87697b2..203e374599908 100644 --- a/java/src/org/openqa/selenium/remote/http/RetryRequest.java +++ b/java/src/org/openqa/selenium/remote/http/RetryRequest.java @@ -31,7 +31,6 @@ import dev.failsafe.RetryPolicy; import dev.failsafe.event.ExecutionAttemptedEvent; import dev.failsafe.function.CheckedFunction; - import java.net.ConnectException; import java.util.logging.Logger; import org.openqa.selenium.TimeoutException; @@ -40,8 +39,10 @@ public class RetryRequest implements Filter { private static final Logger LOG = Logger.getLogger(RetryRequest.class.getName()); - private static final Fallback fallback = Fallback.of( - (CheckedFunction, ? extends HttpResponse>) RetryRequest::getFallback); + private static final Fallback fallback = + Fallback.of( + (CheckedFunction, ? extends HttpResponse>) + RetryRequest::getFallback); // Retry on connection error. private static final RetryPolicy connectionFailurePolicy = @@ -106,8 +107,7 @@ private static HttpResponse getFallback( .setStatus(HTTP_GATEWAY_TIMEOUT) .setContent( asJson(ImmutableMap.of("value", ImmutableMap.of("message", "Read timeout")))); - } else - throw exception; + } else throw exception; } else if (executionAttemptedEvent.getLastResult() != null) { HttpResponse response = executionAttemptedEvent.getLastResult(); if ((response.getStatus() == HTTP_INTERNAL_ERROR