Merge branch 'trunk' into rb_bidi_add_request_handler

Author: aguspe

.github/workflows/ci-python.yml

@@ -100,6 +100,8 @@ jobs:
       fail-fast: false
       matrix:
         include:
+          - browser: safari
+            os: macos
           - browser: chrome
             os: ubuntu
           - browser: edge
@@ -114,21 +116,3 @@ jobs:
       run: |
         bazel test --local_test_jobs 1 --flaky_test_attempts 3 //py:common-${{ matrix.browser }}-bidi
         bazel test --local_test_jobs 1 --flaky_test_attempts 3 //py:test-${{ matrix.browser }}
-
-  safari-tests:
-    name: Browser Tests
-    needs: build
-    uses: ./.github/workflows/bazel.yml
-    strategy:
-      fail-fast: false
-      matrix:
-        include:
-          - browser: safari
-            os: macos
-    with:
-      name: Integration Tests (${{ matrix.browser }}, ${{ matrix.os }})
-      browser: ${{ matrix.browser }}
-      os: ${{ matrix.os }}
-      cache-key: py-browser-${{ matrix.browser }}
-      run: |
-        bazel test --local_test_jobs 1 --flaky_test_attempts 3 //py:test-${{ matrix.browser }}

java/spotbugs-excludes.xml

@@ -230,4 +230,9 @@
     <Bug pattern="NM_CLASS_NAMING_CONVENTION"/>
   </Match>
 
+  <Match>
+    <Class name="org.openqa.selenium.manager.SeleniumManager"/>
+    <Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
+  </Match>
+
 </FindBugsFilter>

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

@@ -27,6 +27,7 @@
 import static org.openqa.selenium.remote.RemoteTags.CAPABILITIES_EVENT;
 import static org.openqa.selenium.remote.RemoteTags.SESSION_ID;
 import static org.openqa.selenium.remote.RemoteTags.SESSION_ID_EVENT;
+import static org.openqa.selenium.remote.http.HttpMethod.DELETE;
 import static org.openqa.selenium.remote.tracing.AttributeKey.SESSION_URI;
 import static org.openqa.selenium.remote.tracing.Tags.EXCEPTION;
 
@@ -79,6 +80,7 @@
 import org.openqa.selenium.grid.data.NodeStatus;
 import org.openqa.selenium.grid.data.NodeStatusEvent;
 import org.openqa.selenium.grid.data.RequestId;
+import org.openqa.selenium.grid.data.Session;
 import org.openqa.selenium.grid.data.SessionRequest;
 import org.openqa.selenium.grid.data.SessionRequestCapability;
 import org.openqa.selenium.grid.data.Slot;
@@ -109,6 +111,7 @@
 import org.openqa.selenium.internal.Require;
 import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.remote.http.HttpClient;
+import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.tracing.AttributeKey;
 import org.openqa.selenium.remote.tracing.AttributeMap;
 import org.openqa.selenium.remote.tracing.Span;
@@ -853,21 +856,34 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) {
           }
         }
 
-        // 'complete' will return 'true' if the session has not timed out during the creation
-        // process: it's still a valid session as it can be used by the client
         boolean isSessionValid = sessionQueue.complete(reqId, response);
-        // If the session request has timed out, tell the Node to remove the session, so that does
-        // not stall
+        // terminate invalid sessions to avoid stale sessions
         if (!isSessionValid && response.isRight()) {
           LOG.log(
               Level.INFO,
-              "Session for request {0} has been created but it has timed out, stopping it to avoid"
-                  + " stalled browser",
+              "Session for request {0} has been created but it has timed out or the connection"
+                  + " dropped, stopping it to avoid stalled browser",
               reqId.toString());
-          URI nodeURI = response.right().getSession().getUri();
-          Node node = getNodeFromURI(nodeURI);
+          Session session = response.right().getSession();
+          Node node = getNodeFromURI(session.getUri());
           if (node != null) {
-            node.stop(response.right().getSession().getId());
+            boolean deleted;
+            try {
+              // Attempt to stop the session
+              deleted =
+                  node.execute(new HttpRequest(DELETE, "/session/" + session.getId())).getStatus()
+                      == 200;
+            } catch (Exception e) {
+              LOG.log(
+                  Level.WARNING,
+                  String.format("Exception while trying to delete session %s", session.getId()),
+                  e);
+              deleted = false;
+            }
+            if (!deleted) {
+              // Kill the session
+              node.stop(session.getId());
+            }
           }
         }
       }

java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java

@@ -220,10 +220,14 @@ public HttpResponse addToQueue(SessionRequest request) {
           result = Either.left(new SessionNotCreatedException("New session request timed out"));
         }
       } catch (InterruptedException e) {
+        // the client will never see the session, ensure the session is disposed
+        data.cancel();
         Thread.currentThread().interrupt();
         result =
             Either.left(new SessionNotCreatedException("Interrupted when creating the session", e));
       } catch (RuntimeException e) {
+        // the client will never see the session, ensure the session is disposed
+        data.cancel();
         result =
             Either.left(
                 new SessionNotCreatedException("An error occurred creating the session", e));
@@ -367,43 +371,30 @@ public List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes
     }
   }
 
-  /** Returns true if the session is still valid (not timed out) */
+  /** Returns true if the session is still valid (not timed out and not canceled) */
   @Override
   public boolean complete(
       RequestId reqId, Either<SessionNotCreatedException, CreateSessionResponse> result) {
     Require.nonNull("New session request", reqId);
     Require.nonNull("Result", result);
     TraceContext context = contexts.getOrDefault(reqId, tracer.getCurrentContext());
     try (Span ignored = context.createSpan("sessionqueue.completed")) {
-      Lock readLock = lock.readLock();
-      readLock.lock();
       Data data;
-      boolean isSessionTimedOut = false;
-      try {
-        data = requests.get(reqId);
-      } finally {
-        readLock.unlock();
-      }
-
-      if (data == null) {
-        return false;
-      } else {
-        isSessionTimedOut = isTimedOut(Instant.now(), data);
-      }
-
       Lock writeLock = lock.writeLock();
       writeLock.lock();
       try {
-        requests.remove(reqId);
+        data = requests.remove(reqId);
         queue.removeIf(req -> reqId.equals(req.getRequestId()));
         contexts.remove(reqId);
       } finally {
         writeLock.unlock();
       }
 
-      data.setResult(result);
+      if (data == null) {
+        return false;
+      }
 
-      return !isSessionTimedOut;
+      return data.setResult(result);
     }
   }
 
@@ -466,6 +457,7 @@ private class Data {
     private final CountDownLatch latch = new CountDownLatch(1);
     private Either<SessionNotCreatedException, CreateSessionResponse> result;
     private boolean complete;
+    private boolean canceled;
 
     public Data(Instant enqueued) {
       this.endTime = enqueued.plus(requestTimeout);
@@ -476,14 +468,19 @@ public synchronized Either<SessionNotCreatedException, CreateSessionResponse> ge
       return result;
     }
 
-    public synchronized void setResult(
+    public synchronized void cancel() {
+      canceled = true;
+    }
+
+    public synchronized boolean setResult(
         Either<SessionNotCreatedException, CreateSessionResponse> result) {
-      if (complete) {
-        return;
+      if (complete || canceled) {
+        return false;
       }
       this.result = result;
       complete = true;
       latch.countDown();
+      return true;
     }
   }
 }

java/src/org/openqa/selenium/manager/SeleniumManager.java

@@ -213,7 +213,7 @@ private synchronized Path getBinary() {
         if (!binary.toFile().exists()) {
           String binaryPathInJar = String.format("%s/%s%s", folder, SELENIUM_MANAGER, extension);
           try (InputStream inputStream = this.getClass().getResourceAsStream(binaryPathInJar)) {
-            binary.getParent().toFile().mkdirs();
+            Files.createDirectories(binary.getParent());
             Files.copy(inputStream, binary);
           }
         }

java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java

@@ -98,6 +98,10 @@ private static Log fromJson(JsonInput input) {
           case "message":
             message = input.nextString();
             break;
+
+          default:
+            input.skipValue();
+            break;
         }
       }
       input.endObject();

java/test/org/openqa/selenium/grid/router/BUILD.bazel

@@ -4,6 +4,7 @@ load("//java:version.bzl", "TOOLS_JAVA_VERSION")
 load("//java/src/org/openqa/selenium/devtools:versions.bzl", "CDP_DEPS")
 
 LARGE_TESTS = [
+    "DistributedTest.java",
     "DistributedCdpTest.java",
     "NewSessionCreationTest.java",
     "RemoteWebDriverDownloadTest.java",