Skip to content

Commit

Permalink
[grid] rework the retry of http requests #14917 (#14924)
Browse files Browse the repository at this point in the history 
Co-authored-by: Diego Molina <[email protected]>
  • Loading branch information
@joerg1985 @diemol
joerg1985 and diemol authored Dec 30, 2024
1 parent 03146da commit 93483c5
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 83 deletions.
2 changes: 0 additions & 2 deletions java/src/org/openqa/selenium/remote/http/BUILD.bazel
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
load("@rules_jvm_external//:defs.bzl", "artifact")
load("//java:defs.bzl", "java_export")
load("//java:version.bzl", "SE_VERSION")

Expand All @@ -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"),
],
)
114 changes: 45 additions & 69 deletions java/src/org/openqa/selenium/remote/http/RetryRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,89 +17,65 @@

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<HttpResponse> fallback =
Fallback.of(
(CheckedFunction<ExecutionAttemptedEvent<? extends HttpResponse>, ? extends HttpResponse>)
RetryRequest::getFallback);

// Retry on connection error.
private static final RetryPolicy<HttpResponse> connectionFailurePolicy =
RetryPolicy.<HttpResponse>builder()
.handleIf(failure -> failure.getCause() instanceof ConnectException)
.withMaxRetries(3)
.onRetry(
e ->
LOG.log(
getDebugLogLevel(),
"Connection failure #{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()
.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();
private static final int RETRIES_ON_CONNECTION_FAILURE = 3;
private static final int RETRIES_ON_SERVER_ERROR = 2;
private static final int NEEDED_ATTEMPTS =
Math.max(RETRIES_ON_CONNECTION_FAILURE, RETRIES_ON_SERVER_ERROR) + 1;

@Override
public HttpHandler apply(HttpHandler next) {
return req ->
Failsafe.with(fallback)
.compose(serverErrorPolicy)
.compose(connectionFailurePolicy)
.get(() -> next.execute(req));
}
return req -> {
// start to preform the request in a loop, to allow retries
for (int i = 0; i < NEEDED_ATTEMPTS; i++) {
HttpResponse response;

try {
response = next.execute(req);
} catch (RuntimeException ex) {
// detect a connection failure we would like to retry
boolean isConnectionFailure = ex.getCause() instanceof ConnectException;

// must be a connection failure and check whether we have retries left for this
if (isConnectionFailure && i < RETRIES_ON_CONNECTION_FAILURE) {
LOG.log(LOG_LEVEL, "Retry #" + (i + 1) + " on ConnectException", ex);
continue;
}

private static HttpResponse getFallback(
ExecutionAttemptedEvent<? extends HttpResponse> 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"))));
// not a connection failure or retries exceeded, rethrow and let the caller handle this
throw ex;
}

// detect a server error we would like to retry
boolean isServerError =
(response.getStatus() == HTTP_INTERNAL_ERROR && response.getContent().length() == 0)
|| response.getStatus() == HTTP_UNAVAILABLE;

// must be a server error and check whether we have retries left for this
if (isServerError && i < RETRIES_ON_SERVER_ERROR) {
LOG.log(LOG_LEVEL, "Retry #" + (i + 1) + " on ServerError: " + response.getStatus());
continue;
}

// not a server error or retries exceeded, return the result to the caller
return response;
}
}
return executionAttemptedEvent.getLastResult();

// This should not be reachable, we either retry or fail before. The only way to get here
// is to set the static final int fields above to inconsistent values.
throw new IllegalStateException("Effective unreachable code reached, check constants.");
};
}
}
102 changes: 90 additions & 12 deletions java/test/org/openqa/selenium/remote/http/RetryRequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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<Callable<HttpResponse>> 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<Future<HttpResponse>> 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
Expand Down Expand Up @@ -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<UncheckedIOException> 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<HttpResponse> 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);
}
}

0 comments on commit 93483c5

Please sign in to comment.