[grid] improved the new session handling when the queue is empty (#12385)

joerg1985 · diemol · web-flow · commit b22d08dc2b67 · 2023-07-21T11:38:13.000+02:00
Co-authored-by: Diego Molina &lt;diemol@users.noreply.github.com&gt;
diff --git a/java/src/org/openqa/selenium/ImmutableCapabilities.java b/java/src/org/openqa/selenium/ImmutableCapabilities.java
@@ -135,22 +135,7 @@ public ImmutableCapabilities(
   }
 
   public ImmutableCapabilities(Capabilities other) {
-    Require.nonNull("Capabilities", other);
-
-    Map<String, Object> delegate = new TreeMap<>();
-    other
-        .getCapabilityNames()
-        .forEach(
-            name -> {
-              Require.nonNull("Capability name", name);
-              Object value = other.getCapability(name);
-              Require.nonNull("Capability value", value);
-
-              setCapability(delegate, name, value);
-            });
-
-    this.delegate = Collections.unmodifiableMap(delegate);
-    this.hashCode = SharedCapabilitiesMethods.hashCode(this);
+    this(other.asMap());
   }
 
   public ImmutableCapabilities(Map<?, ?> capabilities) {
diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java
@@ -762,7 +762,7 @@ public void run() {
         loop = !inQueue.isEmpty();
       } else {
         inQueue = null;
-        loop = true;
+        loop = !sessionQueue.peekEmpty();
       }
       while (loop) {
         // We deliberately run this outside of a lock: if we're unsuccessful
diff --git a/java/src/org/openqa/selenium/grid/node/config/NodeOptions.java b/java/src/org/openqa/selenium/grid/node/config/NodeOptions.java
@@ -46,6 +46,7 @@
 import java.util.stream.IntStream;
 import java.util.stream.StreamSupport;
 import org.openqa.selenium.Capabilities;
+import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.PersistentCapabilities;
 import org.openqa.selenium.Platform;
 import org.openqa.selenium.SessionNotCreatedException;
@@ -190,7 +191,7 @@ public Duration getHeartbeatPeriod() {
 
   public Map<Capabilities, Collection<SessionFactory>> getSessionFactories(
       /* Danger! Java stereotype ahead! */
-      Function<Capabilities, Collection<SessionFactory>> factoryFactory) {
+      Function<ImmutableCapabilities, Collection<SessionFactory>> factoryFactory) {
 
     LOG.log(Level.INFO, "Detected {0} available processors", DEFAULT_MAX_SESSIONS);
     boolean overrideMaxSessions =
@@ -336,7 +337,7 @@ private SessionFactory createSessionFactory(String clazz, Capabilities stereotyp
   }
 
   private void addDriverConfigs(
-      Function<Capabilities, Collection<SessionFactory>> factoryFactory,
+      Function<ImmutableCapabilities, Collection<SessionFactory>> factoryFactory,
       ImmutableMultimap.Builder<Capabilities, SessionFactory> sessionFactories) {
     Multimap<WebDriverInfo, SessionFactory> driverConfigs = HashMultimap.create();
     config
@@ -459,10 +460,11 @@ private void addDriverConfigs(
                         .max(Comparator.comparingInt(builder -> builder.score(stereotype)))
                         .ifPresent(
                             builder -> {
+                              ImmutableCapabilities immutable = new ImmutableCapabilities(stereotype);
                               int maxDriverSessions = getDriverMaxSessions(info, driverMaxSessions);
                               for (int i = 0; i < maxDriverSessions; i++) {
                                 driverConfigs.putAll(
-                                    driverInfoConfig, factoryFactory.apply(stereotype));
+                                    driverInfoConfig, factoryFactory.apply(immutable));
                               }
                             });
                   });
@@ -554,7 +556,7 @@ private void addSpecificDrivers(
   }
 
   private Map<WebDriverInfo, Collection<SessionFactory>> discoverDrivers(
-      int maxSessions, Function<Capabilities, Collection<SessionFactory>> factoryFactory) {
+      int maxSessions, Function<ImmutableCapabilities, Collection<SessionFactory>> factoryFactory) {
 
     if (!config.getBool(NODE_SECTION, "detect-drivers").orElse(DEFAULT_DETECT_DRIVERS)) {
       return ImmutableMap.of();
@@ -595,9 +597,10 @@ private Map<WebDriverInfo, Collection<SessionFactory>> discoverDrivers(
               .max(Comparator.comparingInt(builder -> builder.score(caps)))
               .ifPresent(
                   builder -> {
+                    ImmutableCapabilities immutable = new ImmutableCapabilities(caps);
                     int maxDriverSessions = getDriverMaxSessions(info, maxSessions);
                     for (int i = 0; i < maxDriverSessions; i++) {
-                      toReturn.putAll(info, factoryFactory.apply(caps));
+                      toReturn.putAll(info, factoryFactory.apply(immutable));
                     }
                   });
         });
diff --git a/java/src/org/openqa/selenium/grid/node/local/LocalNodeFactory.java b/java/src/org/openqa/selenium/grid/node/local/LocalNodeFactory.java
@@ -25,7 +25,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.ServiceLoader;
-import org.openqa.selenium.Capabilities;
+import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.grid.config.Config;
 import org.openqa.selenium.grid.data.DefaultSlotMatcher;
 import org.openqa.selenium.grid.data.SlotMatcher;
@@ -101,7 +101,7 @@ private static Collection<SessionFactory> createSessionFactory(
       HttpClient.Factory clientFactory,
       Duration sessionTimeout,
       List<DriverService.Builder<?, ?>> builders,
-      Capabilities stereotype) {
+      ImmutableCapabilities stereotype) {
     ImmutableList.Builder<SessionFactory> toReturn = ImmutableList.builder();
     SlotMatcher slotMatcher = new DefaultSlotMatcher();
     String webDriverExecutablePath =
diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java
@@ -96,6 +96,13 @@ private RequestId requestIdFrom(Map<String, String> params) {
     return new RequestId(UUID.fromString(params.get("requestId")));
   }
 
+  /**
+   * A fast-path to detect if the queue is empty, returns false if there is no fast-path available.
+   *
+   * @return true if the queue is empty, false if it is not empty or unknown
+   */
+  public abstract boolean peekEmpty();
+
   public abstract HttpResponse addToQueue(SessionRequest request);
 
   public abstract boolean retryAddToQueue(SessionRequest request);
diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java
@@ -185,6 +185,17 @@ private boolean isTimedOut(Instant now, Data data) {
     return data.endTime.isBefore(now);
   }
 
+  @Override
+  public boolean peekEmpty() {
+    Lock readLock = lock.readLock();
+    readLock.lock();
+    try {
+      return requests.isEmpty() && queue.isEmpty() ;
+    } finally {
+      readLock.unlock();
+    }
+  }
+
   @Override
   public HttpResponse addToQueue(SessionRequest request) {
     Require.nonNull("New session request", request);
diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java
@@ -90,6 +90,12 @@ public static NewSessionQueue create(Config config) {
     }
   }
 
+  @Override
+  public boolean peekEmpty() {
+    // we have no fast-path, assume we might be not empty
+    return false;
+  }
+
   @Override
   public HttpResponse addToQueue(SessionRequest request) {
     HttpRequest upstream = new HttpRequest(POST, "/se/grid/newsessionqueue/session");

Original file line number	Diff line number	Diff line change
`@@ -762,7 +762,7 @@ public void run() {`
`762`	`762`	`loop = !inQueue.isEmpty();`
`763`	`763`	`} else {`
`764`	`764`	`inQueue = null;`
`765`		`- loop = true;`
	`765`	`+ loop = !sessionQueue.peekEmpty();`
`766`	`766`	`}`
`767`	`767`	`while (loop) {`
`768`	`768`	`// We deliberately run this outside of a lock: if we're unsuccessful`
Original file line number	Diff line number	Diff line change
`@@ -90,6 +90,12 @@ public static NewSessionQueue create(Config config) {`
`90`	`90`	`}`
`91`	`91`	`}`
`92`	`92`
	`93`	`+ @Override`
	`94`	`+ public boolean peekEmpty() {`
	`95`	`+ // we have no fast-path, assume we might be not empty`
	`96`	`+ return false;`
	`97`	`+ }`
	`98`	`+`
`93`	`99`	`@Override`
`94`	`100`	`public HttpResponse addToQueue(SessionRequest request) {`
`95`	`101`	`HttpRequest upstream = new HttpRequest(POST, "/se/grid/newsessionqueue/session");`