From 66f1a4ad1f07f712df2e136e4d265d9ad30ed529 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Sautter?= Date: Thu, 14 Nov 2024 21:19:42 +0100 Subject: [PATCH] [grid] detect a client timeout while session creation #14743 --- .../distributor/local/LocalDistributor.java | 5 +- .../local/LocalNewSessionQueue.java | 41 ++-- .../openqa/selenium/grid/router/BUILD.bazel | 1 + .../selenium/grid/router/DistributedTest.java | 199 ++++++++++++++++++ 4 files changed, 220 insertions(+), 26 deletions(-) create mode 100644 java/test/org/openqa/selenium/grid/router/DistributedTest.java diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 6489324ca2c07..c0799b9c6d495 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/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, diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 619ecbc0d67db..66df209de1d58 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/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,7 +371,7 @@ public List getNextAvailable(Map 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 result) { @@ -375,35 +379,22 @@ public boolean complete( 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 result; private boolean complete; + private boolean canceled; public Data(Instant enqueued) { this.endTime = enqueued.plus(requestTimeout); @@ -476,14 +468,19 @@ public synchronized Either ge return result; } - public synchronized void setResult( + public synchronized void cancel() { + canceled = true; + } + + public synchronized boolean setResult( Either result) { - if (complete) { - return; + if (complete || canceled) { + return false; } this.result = result; complete = true; latch.countDown(); + return true; } } } diff --git a/java/test/org/openqa/selenium/grid/router/BUILD.bazel b/java/test/org/openqa/selenium/grid/router/BUILD.bazel index af5d143374150..40339643b5f29 100644 --- a/java/test/org/openqa/selenium/grid/router/BUILD.bazel +++ b/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", diff --git a/java/test/org/openqa/selenium/grid/router/DistributedTest.java b/java/test/org/openqa/selenium/grid/router/DistributedTest.java new file mode 100644 index 0000000000000..d22157a8d14e9 --- /dev/null +++ b/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 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("

Cheese

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