Prevent browser to be created and unused in case a short session-request-timeout is set

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

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();

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

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

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

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