Merge branch 'trunk' into renovate/com.google.googlejavaformat-google-java-format-1.x

Author: pujagani

.bazelversion

@@ -1 +1 @@
-7.3.1
+7.4.1

.github/workflows/stale.yml

@@ -21,7 +21,7 @@ jobs:
           stale-issue-message: 'This issue is stale because it has been open 280 days with no activity. Remove stale label or comment or this will be closed in 14 days.'
           close-issue-message: 'This issue was closed because it has been stalled for 14 days with no activity.'
           stale-issue-label: 'I-stale'
-          days-before-stale: 280
+          days-before-stale: 180
           days-before-close: 14
       - uses: actions/stale@v9
         with:

MODULE.bazel

@@ -12,7 +12,7 @@ bazel_dep(name = "contrib_rules_jvm", version = "0.27.0")
 bazel_dep(name = "platforms", version = "0.0.10")
 
 # Required for the closure rules
-bazel_dep(name = "protobuf", version = "29.1", dev_dependency = True, repo_name = "com_google_protobuf")
+bazel_dep(name = "protobuf", version = "29.2", dev_dependency = True, repo_name = "com_google_protobuf")
 
 # Required for rules_rust to import the crates properly
 bazel_dep(name = "rules_cc", version = "0.0.9", dev_dependency = True)

java/src/org/openqa/selenium/grid/distributor/GridModel.java

@@ -400,7 +400,12 @@ public void release(SessionId id) {
     }
   }
 
-  public void reserve(NodeStatus status, Slot slot) {
+  /**
+   * A helper to reserve a slot of a node. The writeLock must be acquired outside to ensure the view
+   * of the NodeStatus is the current state, otherwise concurrent calls to amend will work with an
+   * outdated view of slots.
+   */
+  private void reserve(NodeStatus status, Slot slot) {
     Instant now = Instant.now();
 
     Slot reserved =
@@ -490,30 +495,29 @@ public void updateHealthCheckCount(NodeId id, Availability availability) {
     }
   }
 
+  /**
+   * A helper to replace the availability and a slot of a node. The writeLock must be acquired
+   * outside to ensure the view of the NodeStatus is the current state, otherwise concurrent calls
+   * to amend will work with an outdated view of slots.
+   */
   private void amend(Availability availability, NodeStatus status, Slot slot) {
     Set<Slot> newSlots = new HashSet<>(status.getSlots());
     newSlots.removeIf(s -> s.getId().equals(slot.getId()));
     newSlots.add(slot);
 
     NodeStatus node = getNode(status.getNodeId());
 
-    Lock writeLock = lock.writeLock();
-    writeLock.lock();
-    try {
-      nodes.remove(node);
-      nodes.add(
-          new NodeStatus(
-              status.getNodeId(),
-              status.getExternalUri(),
-              status.getMaxSessionCount(),
-              newSlots,
-              availability,
-              status.getHeartbeatPeriod(),
-              status.getSessionTimeout(),
-              status.getVersion(),
-              status.getOsInfo()));
-    } finally {
-      writeLock.unlock();
-    }
+    nodes.remove(node);
+    nodes.add(
+        new NodeStatus(
+            status.getNodeId(),
+            status.getExternalUri(),
+            status.getMaxSessionCount(),
+            newSlots,
+            availability,
+            status.getHeartbeatPeriod(),
+            status.getSessionTimeout(),
+            status.getVersion(),
+            status.getOsInfo()));
   }
 }

java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java

@@ -355,6 +355,7 @@ public LocalDistributor add(Node node) {
     // An exception occurs if Node heartbeat has started but the server is not ready.
     // Unhandled exception blocks the event-bus thread from processing any event henceforth.
     NodeStatus initialNodeStatus;
+    Runnable healthCheck;
     try {
       initialNodeStatus = node.getStatus();
       if (initialNodeStatus.getAvailability() != UP) {
@@ -363,8 +364,17 @@ public LocalDistributor add(Node node) {
         // We do not need to add this Node for now.
         return this;
       }
-      model.add(initialNodeStatus);
-      nodes.put(node.getId(), node);
+      // Extract the health check
+      healthCheck = asRunnableHealthCheck(node);
+      Lock writeLock = lock.writeLock();
+      writeLock.lock();
+      try {
+        nodes.put(node.getId(), node);
+        model.add(initialNodeStatus);
+        allChecks.put(node.getId(), healthCheck);
+      } finally {
+        writeLock.unlock();
+      }
     } catch (Exception e) {
       LOG.log(
           Debug.getDebugLogLevel(),
@@ -373,10 +383,6 @@ public LocalDistributor add(Node node) {
       return this;
     }
 
-    // Extract the health check
-    Runnable healthCheck = asRunnableHealthCheck(node);
-    allChecks.put(node.getId(), healthCheck);
-
     updateNodeStatus(initialNodeStatus, healthCheck);
 
     LOG.info(
@@ -415,7 +421,15 @@ private void updateNodeStatus(NodeStatus status, Runnable healthCheck) {
 
   private Runnable runNodeHealthChecks() {
     return () -> {
-      ImmutableMap<NodeId, Runnable> nodeHealthChecks = ImmutableMap.copyOf(allChecks);
+      ImmutableMap<NodeId, Runnable> nodeHealthChecks;
+      Lock readLock = this.lock.readLock();
+      readLock.lock();
+      try {
+        nodeHealthChecks = ImmutableMap.copyOf(allChecks);
+      } finally {
+        readLock.unlock();
+      }
+
       for (Runnable nodeHealthCheck : nodeHealthChecks.values()) {
         GuardedRunnable.guard(nodeHealthCheck).run();
       }

java/src/org/openqa/selenium/grid/node/local/LocalNode.java

@@ -377,8 +377,10 @@ public boolean isReady() {
   @VisibleForTesting
   @ManagedAttribute(name = "CurrentSessions")
   public int getCurrentSessionCount() {
+    // we need the exact size, see javadoc of Cache.size
+    long n = currentSessions.asMap().values().stream().count();
     // It seems wildly unlikely we'll overflow an int
-    return Math.toIntExact(currentSessions.size());
+    return Math.toIntExact(n);
   }
 
   @VisibleForTesting
@@ -1005,6 +1007,9 @@ public HealthCheck getHealthCheck() {
   public void drain() {
     bus.fire(new NodeDrainStarted(getId()));
     draining = true;
+    // Ensure the pendingSessions counter will not be decremented by timed out sessions not included
+    // in the currentSessionCount and the NodeDrainComplete will be raised to early.
+    currentSessions.cleanUp();
     int currentSessionCount = getCurrentSessionCount();
     if (currentSessionCount == 0) {
       LOG.info("Firing node drain complete message");

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