[grid] detect a client timeout while session creation #14743

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

@@ -853,11 +853,8 @@ 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,

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/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",

java/test/org/openqa/selenium/grid/router/DistributedTest.java

@@ -0,0 +1,199 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium.grid.router;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.StringReader;
+import java.time.Duration;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.openqa.selenium.SessionNotCreatedException;
+import org.openqa.selenium.WebDriver;
+import org.openqa.selenium.grid.config.MapConfig;
+import org.openqa.selenium.grid.config.MemoizedConfig;
+import org.openqa.selenium.grid.config.TomlConfig;
+import org.openqa.selenium.grid.router.DeploymentTypes.Deployment;
+import org.openqa.selenium.grid.server.BaseServerOptions;
+import org.openqa.selenium.grid.server.Server;
+import org.openqa.selenium.json.Json;
+import org.openqa.selenium.json.JsonInput;
+import org.openqa.selenium.netty.server.NettyServer;
+import org.openqa.selenium.remote.RemoteWebDriver;
+import org.openqa.selenium.remote.http.ClientConfig;
+import org.openqa.selenium.remote.http.Contents;
+import org.openqa.selenium.remote.http.HttpClient;
+import org.openqa.selenium.remote.http.HttpMethod;
+import org.openqa.selenium.remote.http.HttpRequest;
+import org.openqa.selenium.remote.http.HttpResponse;
+import org.openqa.selenium.testing.Safely;
+import org.openqa.selenium.testing.TearDownFixture;
+import org.openqa.selenium.testing.drivers.Browser;
+
+class DistributedTest {
+
+  private final List<TearDownFixture> tearDowns = new LinkedList<>();
+  private Server<?> server;
+  private Browser browser;
+  private Server<?> appServer;
+
+  @BeforeEach
+  public void setupServers() {
+    browser = Objects.requireNonNull(Browser.detect());
+
+    Deployment deployment =
+        DeploymentTypes.DISTRIBUTED.start(
+            browser.getCapabilities(),
+            new TomlConfig(
+                new StringReader(
+                    "[node]\n"
+                        + "driver-implementation = "
+                        + String.format("\"%s\"", browser.displayName())
+                        + "\n"
+                        + "session-timeout = 240"
+                        + "\n"
+                        + "override-max-sessions = true"
+                        + "\n"
+                        + "max-sessions = 2")));
+    tearDowns.add(deployment);
+
+    server = deployment.getServer();
+
+    appServer =
+        new NettyServer(
+            new BaseServerOptions(new MemoizedConfig(new MapConfig(Map.of()))),
+            req -> {
+              try {
+                Thread.sleep(2000);
+              } catch (InterruptedException e) {
+                throw new RuntimeException(e);
+              }
+              return new HttpResponse().setContent(Contents.string("<h1>Cheese</h1>", UTF_8));
+            });
+
+    tearDowns.add(() -> appServer.stop());
+    appServer.start();
+  }
+
+  @AfterEach
+  public void tearDown() {
+    tearDowns.parallelStream().forEach(Safely::safelyCall);
+  }
+
+  @Test
+  void clientTimeoutDoesNotLeakARunningBrowser() throws Exception {
+    assertThat(server.isStarted()).isTrue();
+
+    // using nanoTime is intentionally, the clock in WSL2 is jumping
+    var start = System.nanoTime();
+
+    // one healthy to ensure the distributed grid is working as expected
+    WebDriver healthy =
+        RemoteWebDriver.builder()
+            .oneOf(browser.getCapabilities())
+            .config(
+                ClientConfig.defaultConfig()
+                    .baseUrl(server.getUrl())
+                    // ensures the time taken * 2 is smaller than session-timeout
+                    .readTimeout(Duration.ofSeconds(90)))
+            .build();
+
+    var end = System.nanoTime();
+
+    try {
+      // provoke the client to run into a http timeout
+      SessionNotCreatedException nce =
+          Assertions.assertThrows(
+              SessionNotCreatedException.class,
+              () ->
+                  RemoteWebDriver.builder()
+                      .oneOf(browser.getCapabilities())
+                      .config(
+                          ClientConfig.defaultConfig()
+                              .baseUrl(server.getUrl())
+                              .readTimeout(Duration.ofMillis(600)))
+                      .build());
+
+      assertThat(nce.getMessage()).contains("TimeoutException");
+
+      Thread.sleep(
+          // ensure the grid has some time to start the browser
+          Duration.ofNanos((end - start) * 2)
+              // and shutdown the browser
+              .plusSeconds(20)
+              .toMillis());
+
+      HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl());
+      try {
+        HttpRequest request = new HttpRequest(HttpMethod.POST, "/graphql");
+        request.setContent(Contents.utf8String("{\"query\": \"{grid { sessionCount }}\"}"));
+        HttpResponse response = client.execute(request);
+
+        JsonInput input = new Json().newInput(Contents.reader(response));
+        int sessionCount = -1;
+
+        input.beginObject();
+        while (input.hasNext()) {
+          switch (input.nextName()) {
+            case "data":
+              input.beginObject();
+              while (input.hasNext()) {
+                switch (input.nextName()) {
+                  case "grid":
+                    input.beginObject();
+                    while (input.hasNext()) {
+                      switch (input.nextName()) {
+                        case "sessionCount":
+                          sessionCount = input.read(Integer.class);
+                          break;
+                        default:
+                          input.skipValue();
+                          break;
+                      }
+                    }
+                    input.endObject();
+                    break;
+                  default:
+                    input.skipValue();
+                    break;
+                }
+              }
+              input.endObject();
+              break;
+            default:
+              input.skipValue();
+              break;
+          }
+        }
+
+        Assertions.assertEquals(1, sessionCount);
+      } finally {
+        Safely.safelyCall(client::close);
+      }
+    } finally {
+      Safely.safelyCall(healthy::close);
+    }
+  }
+}