[grid] rework the retry of http requests #14917

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"),
     ],
 )

java/src/org/openqa/selenium/remote/http/RetryRequest.java

@@ -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.");
+    };
   }
 }

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<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
@@ -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);
   }
 }