Prevent browser to be created and unused in case a short session-requ…

…est-timeout is set (#12848) * Add logs * Prevent a session to be created while its timeout has expired * restore missing "remove" * Close timedout session on creation to prevent them to be staled * Correct stopping of expired sessions * Remove logs for final commit * removed a test that is now useless as request session timeout is now strict * Simplify session stopping if timeout expires * Add some tests * remove logs * Return value directly * remove unused method * Removed hard coded value * correct according to comments * Correct according to comments * Remove spaces * remove spaces * Remove unused import * Add comments to tell why session is valid * fix linter issues * Add a test when session time out * test improved * Running format script --------- Co-authored-by: titusfortner <[email protected]> Co-authored-by: Diego Molina <[email protected]> Co-authored-by: Diego Molina <[email protected]>
SeleniumHQ · Oct 19, 2023 · cee7f6b · cee7f6b
1 parent cfe7e1f
commit cee7f6b
Show file tree

Hide file tree

Showing 8 changed files with 385 additions and 51 deletions.
diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java
@@ -42,6 +42,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executor;
@@ -838,8 +839,42 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) {
           }
         }
 
-        sessionQueue.complete(reqId, response);
+        // '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
+        if (!isSessionValid && response.isRight()) {
+          LOG.log(
+              Debug.getDebugLogLevel(),
+              "Session for request {0} has been created but it has timed out, stopping it to avoid"
+                  + " stalled browser",
+              reqId.toString());
+          URI nodeURI = response.right().getSession().getUri();
+          Node node = getNodeFromURI(nodeURI);
+          if (node != null) {
+            node.stop(response.right().getSession().getId());
+          }
+        }
       }
     }
   }
+
+  protected Node getNodeFromURI(URI uri) {
+    Lock readLock = this.lock.readLock();
+    readLock.lock();
+    try {
+      Optional<NodeStatus> nodeStatus =
+          model.getSnapshot().stream()
+              .filter(node -> node.getExternalUri().equals(uri))
+              .findFirst();
+      if (nodeStatus.isPresent()) {
+        return nodes.get(nodeStatus.get().getNodeId());
+      } else {
+        return null;
+      }
+    } finally {
+      readLock.unlock();
+    }
+  }
 }
diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java
@@ -111,7 +111,7 @@ private RequestId requestIdFrom(Map<String, String> params) {
 
   public abstract List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes);
 
-  public abstract void complete(
+  public abstract boolean complete(
       RequestId reqId, Either<SessionNotCreatedException, CreateSessionResponse> result);
 
   public abstract int clearQueue();

diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java
@@ -17,6 +17,8 @@
 
 package org.openqa.selenium.grid.sessionqueue;
 
+import static java.util.Collections.singletonMap;
+import static org.openqa.selenium.remote.http.Contents.asJson;
 import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf;
 import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST;
 import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE;
@@ -50,9 +52,14 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException {
       HTTP_REQUEST.accept(span, req);
 
       CreateSessionResponse response = Contents.fromJson(req, CreateSessionResponse.class);
-      queue.complete(requestId, Either.right(response));
 
-      HttpResponse res = new HttpResponse();
+      // '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 = queue.complete(requestId, Either.right(response));
+
+      HttpResponse res =
+          new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid)));
+
       HTTP_RESPONSE.accept(span, res);
       return res;
     }

diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java
@@ -17,6 +17,8 @@
 
 package org.openqa.selenium.grid.sessionqueue;
 
+import static java.util.Collections.singletonMap;
+import static org.openqa.selenium.remote.http.Contents.asJson;
 import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf;
 import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST;
 import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE;
@@ -52,9 +54,13 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException {
 
       String message = Contents.fromJson(req, String.class);
       SessionNotCreatedException exception = new SessionNotCreatedException(message);
-      queue.complete(requestId, Either.left(exception));
 
-      HttpResponse res = new HttpResponse();
+      // '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 = queue.complete(requestId, Either.left(exception));
+
+      HttpResponse res =
+          new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid)));
       HTTP_RESPONSE.accept(span, res);
       return res;
     }

diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java
@@ -214,9 +214,6 @@ public HttpResponse addToQueue(SessionRequest request) {
       try {
 
         boolean sessionCreated = data.latch.await(requestTimeout.toMillis(), MILLISECONDS);
-        if (!(sessionCreated || isRequestInQueue(request.getRequestId()))) {
-          sessionCreated = data.latch.await(5000, MILLISECONDS);
-        }
 
         if (sessionCreated) {
           result = data.result;
@@ -293,6 +290,12 @@ public boolean retryAddToQueue(SessionRequest request) {
         if (!requests.containsKey(request.getRequestId())) {
           return false;
         }
+        if (isTimedOut(Instant.now(), requests.get(request.getRequestId()))) {
+          // as we try to re-add a session request that has already expired, force session timeout
+          failDueToTimeout(request.getRequestId());
+          // return true to avoid handleNewSessionRequest to call 'complete' an other time
+          return true;
+        }
 
         if (queue.contains(request)) {
           // No need to re-add this
@@ -330,19 +333,6 @@ public Optional<SessionRequest> remove(RequestId reqId) {
     }
   }
 
-  private boolean isRequestInQueue(RequestId requestId) {
-    Lock readLock = lock.readLock();
-    readLock.lock();
-
-    try {
-      Optional<SessionRequest> result =
-          queue.stream().filter(req -> req.getRequestId().equals(requestId)).findAny();
-      return result.isPresent();
-    } finally {
-      readLock.unlock();
-    }
-  }
-
   @Override
   public List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes) {
     Require.nonNull("Stereotypes", stereotypes);
@@ -378,8 +368,9 @@ public List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes
     }
   }
 
+  /** Returns true if the session is still valid (not timed out) */
   @Override
-  public void complete(
+  public boolean complete(
       RequestId reqId, Either<SessionNotCreatedException, CreateSessionResponse> result) {
     Require.nonNull("New session request", reqId);
     Require.nonNull("Result", result);
@@ -388,14 +379,17 @@ public void complete(
       Lock readLock = lock.readLock();
       readLock.lock();
       Data data;
+      boolean isSessionTimedOut = false;
       try {
         data = requests.get(reqId);
       } finally {
         readLock.unlock();
       }
 
       if (data == null) {
-        return;
+        return false;
+      } else {
+        isSessionTimedOut = isTimedOut(Instant.now(), data);
       }
 
       Lock writeLock = lock.writeLock();
@@ -409,6 +403,8 @@ public void complete(
       }
 
       data.setResult(result);
+
+      return !isSessionTimedOut;
     }
   }
 

diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java
@@ -154,7 +154,7 @@ public List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes
   }
 
   @Override
-  public void complete(
+  public boolean complete(
       RequestId reqId, Either<SessionNotCreatedException, CreateSessionResponse> result) {
     Require.nonNull("Request ID", reqId);
     Require.nonNull("Result", result);
@@ -171,7 +171,8 @@ public void complete(
     }
 
     HttpTracing.inject(tracer, tracer.getCurrentContext(), upstream);
-    client.with(addSecret).execute(upstream);
+    HttpResponse response = client.with(addSecret).execute(upstream);
+    return Values.get(response, Boolean.class);
   }
 
   @Override