From d066ac739a0a73646d8a7d3c8ec7c653e5674be9 Mon Sep 17 00:00:00 2001 From: Viet Nguyen Duc Date: Mon, 21 Oct 2024 02:59:06 +0000 Subject: [PATCH] [grid] Update for node shutdown gracefully Signed-off-by: Viet Nguyen Duc --- .../grid/node/ForwardWebDriverCommand.java | 13 ++++- .../org/openqa/selenium/grid/node/Node.java | 9 +--- .../selenium/grid/node/local/LocalNode.java | 17 +++++-- .../org/openqa/selenium/grid/e2e/BUILD.bazel | 20 ++++++++ .../selenium/grid/e2e/SessionTimeoutTest.java | 51 +++++++++++++++++++ .../openqa/selenium/grid/node/NodeTest.java | 44 ++++++++++++++-- 6 files changed, 138 insertions(+), 16 deletions(-) create mode 100644 java/test/org/openqa/selenium/grid/e2e/BUILD.bazel create mode 100644 java/test/org/openqa/selenium/grid/e2e/SessionTimeoutTest.java diff --git a/java/src/org/openqa/selenium/grid/node/ForwardWebDriverCommand.java b/java/src/org/openqa/selenium/grid/node/ForwardWebDriverCommand.java index 458dfff372cbe..8e723ed03062f 100644 --- a/java/src/org/openqa/selenium/grid/node/ForwardWebDriverCommand.java +++ b/java/src/org/openqa/selenium/grid/node/ForwardWebDriverCommand.java @@ -17,12 +17,16 @@ package org.openqa.selenium.grid.node; +import static org.openqa.selenium.remote.HttpSessionId.getSessionId; + import org.openqa.selenium.internal.Require; +import org.openqa.selenium.remote.SessionId; import org.openqa.selenium.remote.http.HttpHandler; import org.openqa.selenium.remote.http.HttpRequest; import org.openqa.selenium.remote.http.HttpResponse; +import org.openqa.selenium.remote.http.Routable; -class ForwardWebDriverCommand implements HttpHandler { +class ForwardWebDriverCommand implements HttpHandler, Routable { private final Node node; @@ -30,6 +34,13 @@ class ForwardWebDriverCommand implements HttpHandler { this.node = Require.nonNull("Node", node); } + @Override + public boolean matches(HttpRequest req) { + return getSessionId(req.getUri()) + .map(id -> node.isSessionOwner(new SessionId(id))) + .orElse(false); + } + @Override public HttpResponse execute(HttpRequest req) { return node.executeWebDriverCommand(req); diff --git a/java/src/org/openqa/selenium/grid/node/Node.java b/java/src/org/openqa/selenium/grid/node/Node.java index 5d54bb3e2981d..f2665548ae76b 100644 --- a/java/src/org/openqa/selenium/grid/node/Node.java +++ b/java/src/org/openqa/selenium/grid/node/Node.java @@ -146,14 +146,7 @@ protected Node( Json json = new Json(); routes = combine( - // "getSessionId" is aggressive about finding session ids, so this needs to be the last - // route that is checked. - matching( - req -> - getSessionId(req.getUri()) - .map(SessionId::new) - .map(this::isSessionOwner) - .orElse(false)) + matching(req -> getSessionId(req.getUri()).map(SessionId::new).isPresent()) .to(() -> new ForwardWebDriverCommand(this)) .with(spanDecorator("node.forward_command")), new CustomLocatorHandler(this, registrationSecret, customLocators), diff --git a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java index 2283e8573042d..7304db8a87847 100644 --- a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java +++ b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java @@ -297,7 +297,13 @@ protected LocalNode( heartbeatPeriod.getSeconds(), TimeUnit.SECONDS); - Runtime.getRuntime().addShutdownHook(new Thread(this::stopAllSessions)); + Runtime.getRuntime() + .addShutdownHook( + new Thread( + () -> { + stopAllSessions(); + drain(); + })); new JMXHelper().register(this); } @@ -316,7 +322,6 @@ private void stopTimedOutSession(RemovalNotification not } // Attempt to stop the session slot.stop(); - this.sessionToDownloadsDir.invalidate(id); // Decrement pending sessions if Node is draining if (this.isDraining()) { int done = pendingSessions.decrementAndGet(); @@ -473,8 +478,6 @@ public Either newSession( sessionToDownloadsDir.put(session.getId(), uuidForSessionDownloads); currentSessions.put(session.getId(), slotToUse); - checkSessionCount(); - SessionId sessionId = session.getId(); Capabilities caps = session.getCapabilities(); SESSION_ID.accept(span, sessionId); @@ -513,6 +516,8 @@ public Either newSession( span.addEvent("Unable to create session with the driver", attributeMap); return Either.left(possibleSession.left()); } + } finally { + checkSessionCount(); } } @@ -765,6 +770,10 @@ public HttpResponse uploadFile(HttpRequest req, SessionId id) { public void stop(SessionId id) throws NoSuchSessionException { Require.nonNull("Session ID", id); + if (sessionToDownloadsDir.getIfPresent(id) != null) { + sessionToDownloadsDir.invalidate(id); + } + SessionSlot slot = currentSessions.getIfPresent(id); if (slot == null) { throw new NoSuchSessionException("Cannot find session with id: " + id); diff --git a/java/test/org/openqa/selenium/grid/e2e/BUILD.bazel b/java/test/org/openqa/selenium/grid/e2e/BUILD.bazel new file mode 100644 index 0000000000000..da013678c69c1 --- /dev/null +++ b/java/test/org/openqa/selenium/grid/e2e/BUILD.bazel @@ -0,0 +1,20 @@ +load("@rules_jvm_external//:defs.bzl", "artifact") +load("//java:defs.bzl", "JUNIT5_DEPS", "java_selenium_test_suite") + +java_selenium_test_suite( + name = "large-tests", + size = "large", + srcs = glob(["*.java"]), + browsers = [ + "chrome", + ], + deps = [ + "//java/src/org/openqa/selenium:core", + "//java/src/org/openqa/selenium/chrome", + "//java/src/org/openqa/selenium/grid", + "//java/src/org/openqa/selenium/remote/http", + artifact("com.google.guava:guava"), + artifact("org.junit.jupiter:junit-jupiter-api"), + artifact("org.assertj:assertj-core"), + ] + JUNIT5_DEPS, +) diff --git a/java/test/org/openqa/selenium/grid/e2e/SessionTimeoutTest.java b/java/test/org/openqa/selenium/grid/e2e/SessionTimeoutTest.java new file mode 100644 index 0000000000000..146318db830bc --- /dev/null +++ b/java/test/org/openqa/selenium/grid/e2e/SessionTimeoutTest.java @@ -0,0 +1,51 @@ +// 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.e2e; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.URI; +import org.assertj.core.api.Assumptions; +import org.junit.jupiter.api.Test; +import org.openqa.selenium.NoSuchSessionException; +import org.openqa.selenium.WebDriver; +import org.openqa.selenium.chrome.ChromeOptions; +import org.openqa.selenium.grid.Bootstrap; +import org.openqa.selenium.remote.RemoteWebDriver; + +public class SessionTimeoutTest { + @Test + void testSessionTimeout() throws Exception { + Assumptions.assumeThat(System.getProperty("webdriver.chrome.binary")).isNull(); + Bootstrap.main(("hub --host 127.0.0.1 --port 4444").split(" ")); + Bootstrap.main( + ("node --host 127.0.0.1 --port 5555 --session-timeout 15 --selenium-manager true") + .split(" ")); + + var options = new ChromeOptions(); + options.addArguments("--disable-search-engine-choice-screen"); + options.addArguments("--headless=new"); + + WebDriver driver = new RemoteWebDriver(URI.create("http://localhost:4444").toURL(), options); + driver.get("http://localhost:4444"); + Thread.sleep(15000); + NoSuchSessionException exception = assertThrows(NoSuchSessionException.class, driver::getTitle); + assertTrue(exception.getMessage().startsWith("Cannot find session with id:")); + } +} diff --git a/java/test/org/openqa/selenium/grid/node/NodeTest.java b/java/test/org/openqa/selenium/grid/node/NodeTest.java index b1c50ed0dc65a..c2a66d2162c4b 100644 --- a/java/test/org/openqa/selenium/grid/node/NodeTest.java +++ b/java/test/org/openqa/selenium/grid/node/NodeTest.java @@ -23,6 +23,8 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.InstanceOfAssertFactories.MAP; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.openqa.selenium.json.Json.MAP_TYPE; import static org.openqa.selenium.remote.http.Contents.string; import static org.openqa.selenium.remote.http.HttpMethod.DELETE; @@ -102,7 +104,9 @@ class NodeTest { private Tracer tracer; private EventBus bus; private LocalNode local; + private LocalNode local2; private Node node; + private Node node2; private ImmutableCapabilities stereotype; private ImmutableCapabilities caps; private URI uri; @@ -150,6 +154,7 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { builder = builder.enableManagedDownloads(true).sessionTimeout(Duration.ofSeconds(1)); } local = builder.build(); + local2 = builder.build(); node = new RemoteNode( @@ -160,6 +165,16 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { registrationSecret, local.getSessionTimeout(), ImmutableSet.of(caps)); + + node2 = + new RemoteNode( + tracer, + new PassthroughHttpClient.Factory(local2), + new NodeId(UUID.randomUUID()), + uri, + registrationSecret, + local2.getSessionTimeout(), + ImmutableSet.of(caps)); } @Test @@ -371,13 +386,36 @@ void shouldOnlyRespondToWebDriverCommandsForSessionsTheNodeOwns() { assertThatEither(response).isRight(); Session session = response.right().getSession(); + Either response2 = + node2.newSession(createSessionRequest(caps)); + assertThatEither(response2).isRight(); + Session session2 = response2.right().getSession(); + + // Assert that should respond to commands for sessions Node 1 owns HttpRequest req = new HttpRequest(POST, String.format("/session/%s/url", session.getId())); assertThat(local.matches(req)).isTrue(); assertThat(node.matches(req)).isTrue(); - req = new HttpRequest(POST, String.format("/session/%s/url", UUID.randomUUID())); - assertThat(local.matches(req)).isFalse(); - assertThat(node.matches(req)).isFalse(); + // Assert that should respond to commands for sessions Node 2 owns + HttpRequest req2 = new HttpRequest(POST, String.format("/session/%s/url", session2.getId())); + assertThat(local2.matches(req2)).isTrue(); + assertThat(node2.matches(req2)).isTrue(); + + // Assert that should not respond to commands for sessions Node 1 does not own + NoSuchSessionException exception = + assertThrows(NoSuchSessionException.class, () -> node.execute(req2)); + assertTrue( + exception + .getMessage() + .startsWith(String.format("Cannot find session with id: %s", session2.getId()))); + + // Assert that should not respond to commands for sessions Node 2 does not own + NoSuchSessionException exception2 = + assertThrows(NoSuchSessionException.class, () -> node2.execute(req)); + assertTrue( + exception2 + .getMessage() + .startsWith(String.format("Cannot find session with id: %s", session.getId()))); } @Test