Skip to content

Commit

Permalink
[java] Remove retrying on timeout exception (#13224)
Browse files Browse the repository at this point in the history 
* [java] Remove retrying on timeout exception

Related to #12975
  • Loading branch information
@pujagani
pujagani authored Dec 4, 2023
1 parent b59a9fb commit b5a2e11
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 93 deletions.
17 changes: 0 additions & 17 deletions java/src/org/openqa/selenium/remote/http/RetryRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {

Expand All @@ -57,15 +55,6 @@ public class RetryRequest implements Filter {
e.getAttemptCount()))
.build();

// Retry on read timeout.
private static final RetryPolicy<HttpResponse> readTimeoutPolicy =
RetryPolicy.<HttpResponse>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<HttpResponse> serverErrorPolicy =
RetryPolicy.<HttpResponse>builder()
Expand All @@ -88,7 +77,6 @@ public HttpHandler apply(HttpHandler next) {
return req ->
Failsafe.with(fallback)
.compose(serverErrorPolicy)
.compose(readTimeoutPolicy)
.compose(connectionFailurePolicy)
.get(() -> next.execute(req));
}
Expand All @@ -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();
Expand Down
87 changes: 11 additions & 76 deletions java/test/org/openqa/selenium/remote/http/RetryRequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -110,12 +109,12 @@ void canReturnAppropriateFallbackResponseWithMultipleThreads()
ExecutorService executorService = Executors.newFixedThreadPool(2);
List<Callable<HttpResponse>> tasks = new ArrayList<>();

tasks.add(() -> handler1.execute(new HttpRequest(GET, "/")));
tasks.add(() -> client.execute(connectionTimeoutRequest));
tasks.add(() -> handler2.execute(new HttpRequest(GET, "/")));

List<Future<HttpResponse>> 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());
}
Expand Down Expand Up @@ -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());
Expand Down

0 comments on commit b5a2e11

Please sign in to comment.