Prevent grid from creating sessions that are about to timeout in queue (corrects issue #11881) (#12014)

bhecquet · diemol · web-flow · commit 9b42f09eb3f5 · 2023-05-22T13:21:05.000+02:00
* Add logs

* add more logs

* Control the status of session request before timeout

* Increase request session timeout by 5 seconds

This allow the extended waiting to be done without being interrupted by
timeoutSessions() method

* remove logs LocalNewSessionQueue.java

* remove logs

* remove logger import

* Add tests to check timeout behaviour

* timeoutSessions method now checks if session is in queue before deleting it

---------

Co-authored-by: Diego Molina &lt;diemol@users.noreply.github.com&gt;
diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java
@@ -164,11 +164,16 @@ private void timeoutSessions() {
     readLock.lock();
     Set<RequestId> ids;
     try {
-      ids =
-          requests.entrySet().stream()
-              .filter(entry -> isTimedOut(now, entry.getValue()))
-              .map(Map.Entry::getKey)
-              .collect(ImmutableSet.toImmutableSet());
+      ids = requests.entrySet().stream()
+        .filter(
+                  entry ->
+                      queue.stream()
+                          .anyMatch(
+                              sessionRequest ->
+                                  sessionRequest.getRequestId().equals(entry.getKey())))
+        .filter(entry -> isTimedOut(now, entry.getValue()))
+        .map(Map.Entry::getKey)
+        .collect(ImmutableSet.toImmutableSet());
     } finally {
       readLock.unlock();
     }
@@ -187,7 +192,6 @@ public HttpResponse addToQueue(SessionRequest request) {
     TraceContext context = TraceSessionRequest.extract(tracer, request);
     try (Span ignored = context.createSpan("sessionqueue.add_to_queue")) {
       contexts.put(request.getRequestId(), context);
-
       Data data = injectIntoQueue(request);
 
       if (isTimedOut(Instant.now(), data)) {
@@ -196,7 +200,13 @@ public HttpResponse addToQueue(SessionRequest request) {
 
       Either<SessionNotCreatedException, CreateSessionResponse> result;
       try {
-        if (data.latch.await(requestTimeout.toMillis(), MILLISECONDS)) {
+
+        boolean sessionCreated = data.latch.await(requestTimeout.toMillis(), MILLISECONDS);
+        if (!(sessionCreated || isRequestInQueue(request.getRequestId()))) {
+          sessionCreated = data.latch.await(5000, MILLISECONDS);
+        }
+
+        if (sessionCreated) {
           result = data.result;
         } else {
           result = Either.left(new SessionNotCreatedException("New session request timed out"));
@@ -308,6 +318,20 @@ 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);
diff --git a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java
@@ -170,7 +170,7 @@ private void waitUntilAddedToQueue(SessionRequest request) {
   @MethodSource("data")
   void shouldBeAbleToAddToQueueAndGetValidResponse(Supplier<TestData> supplier) {
     setup(supplier);
-
+    
     AtomicBoolean isPresent = new AtomicBoolean(false);
 
     new Thread(
@@ -207,6 +207,118 @@ void shouldBeAbleToAddToQueueAndGetValidResponse(Supplier<TestData> supplier) {
     assertThat(isPresent.get()).isTrue();
     assertEquals(HTTP_OK, httpResponse.getStatus());
   }
+  
+
+  @ParameterizedTest
+  @MethodSource("data")
+  void shouldBeAbleToAddToQueueWithTimeoutAndGetValidResponse(Supplier<TestData> supplier)  {
+    setup(supplier);
+
+    SessionRequest sessionRequestWithTimeout = new SessionRequest(
+      new RequestId(UUID.randomUUID()),
+      Instant.now(),
+      Set.of(W3C),
+      Set.of(CAPS),
+      Map.of(),
+      Map.of());
+
+    AtomicBoolean isPresent = new AtomicBoolean(false);
+
+    new Thread(() -> {
+      waitUntilAddedToQueue(sessionRequestWithTimeout);
+      isPresent.set(true);
+
+      Capabilities capabilities = new ImmutableCapabilities("browserName", "chrome");
+
+      try {
+        Thread.sleep(4000); // simulate session waiting in queue
+      } catch (InterruptedException e) {}
+
+      // remove request from queue
+      Map<Capabilities, Long> stereotypes = new HashMap<>();
+      stereotypes.put(new ImmutableCapabilities("browserName", "cheese"), 1L);
+      List<SessionRequest> requests = queue.getNextAvailable(stereotypes);
+
+      SessionId sessionId = new SessionId("123");
+      Session session =
+        new Session(
+          sessionId,
+          URI.create("https://example.com"),
+          CAPS,
+          capabilities,
+          Instant.now());
+      CreateSessionResponse sessionResponse = new CreateSessionResponse(
+        session,
+        JSON.toJson(
+            ImmutableMap.of(
+              "value", ImmutableMap.of(
+                "sessionId", sessionId,
+                "capabilities", capabilities)))
+          .getBytes(UTF_8));
+
+      try {
+        Thread.sleep(2000); // simulate session creation delay
+      } catch (InterruptedException e) {}
+      queue.complete(sessionRequestWithTimeout.getRequestId(), Either.right(sessionResponse));
+    }).start();
+
+    HttpResponse httpResponse = queue.addToQueue(sessionRequestWithTimeout);
+
+    assertThat(isPresent.get()).isTrue();
+    assertEquals(HTTP_OK, httpResponse.getStatus());
+  }
+
+
+  @ParameterizedTest
+  @MethodSource("data")
+  void shouldBeAbleToAddToQueueWithTimeoutAndTimeoutResponse(Supplier<TestData> supplier)  {
+    setup(supplier);
+
+    SessionRequest sessionRequestWithTimeout = new SessionRequest(
+      new RequestId(UUID.randomUUID()),
+      Instant.now(),
+      Set.of(W3C),
+      Set.of(CAPS),
+      Map.of(),
+      Map.of());
+
+    AtomicBoolean isPresent = new AtomicBoolean(false);
+
+    new Thread(() -> {
+      waitUntilAddedToQueue(sessionRequestWithTimeout);
+      isPresent.set(true);
+
+      Capabilities capabilities = new ImmutableCapabilities("browserName", "chrome");
+
+      try {
+        Thread.sleep(5500); // simulate session waiting in queue
+      } catch (InterruptedException e) {}
+
+      SessionId sessionId = new SessionId("123");
+      Session session =
+        new Session(
+          sessionId,
+          URI.create("https://example.com"),
+          CAPS,
+          capabilities,
+          Instant.now());
+      CreateSessionResponse sessionResponse = new CreateSessionResponse(
+        session,
+        JSON.toJson(
+            ImmutableMap.of(
+              "value", ImmutableMap.of(
+                "sessionId", sessionId,
+                "capabilities", capabilities)))
+          .getBytes(UTF_8));
+
+      queue.complete(sessionRequestWithTimeout.getRequestId(), Either.right(sessionResponse));
+    }).start();
+
+    HttpResponse httpResponse = queue.addToQueue(sessionRequestWithTimeout);
+
+    assertThat(isPresent.get()).isTrue();
+    assertEquals(HTTP_INTERNAL_ERROR, httpResponse.getStatus());
+  }
 
   @ParameterizedTest
   @MethodSource("data")
