diff --git a/java/src/org/openqa/selenium/remote/HttpCommandExecutor.java b/java/src/org/openqa/selenium/remote/HttpCommandExecutor.java index c004fca8481d0..5bf064078b1d5 100644 --- a/java/src/org/openqa/selenium/remote/HttpCommandExecutor.java +++ b/java/src/org/openqa/selenium/remote/HttpCommandExecutor.java @@ -23,6 +23,7 @@ import static org.openqa.selenium.remote.DriverCommand.QUIT; import static org.openqa.selenium.remote.HttpSessionId.getSessionId; +import java.io.Closeable; import java.io.IOException; import java.net.URL; import java.util.Collections; @@ -39,7 +40,7 @@ import org.openqa.selenium.remote.http.HttpRequest; import org.openqa.selenium.remote.http.HttpResponse; -public class HttpCommandExecutor implements CommandExecutor { +public class HttpCommandExecutor implements CommandExecutor, Closeable { private final URL remoteServer; protected final Map additionalCommands; @@ -270,4 +271,9 @@ public Response execute(Command command) throws IOException { throw e; } } + + @Override + public void close() { + client.close(); + } } diff --git a/java/src/org/openqa/selenium/remote/RemoteWebDriver.java b/java/src/org/openqa/selenium/remote/RemoteWebDriver.java index 20bd09a9cb273..8a8732f86592a 100644 --- a/java/src/org/openqa/selenium/remote/RemoteWebDriver.java +++ b/java/src/org/openqa/selenium/remote/RemoteWebDriver.java @@ -25,6 +25,7 @@ import com.google.common.net.MediaType; import java.io.BufferedInputStream; +import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.net.MalformedURLException; @@ -98,7 +99,6 @@ import org.openqa.selenium.remote.http.Contents; import org.openqa.selenium.remote.http.HttpClient; import org.openqa.selenium.remote.http.jdk.ConnectionException; -import org.openqa.selenium.remote.service.DriverCommandExecutor; import org.openqa.selenium.remote.tracing.TracedHttpClient; import org.openqa.selenium.remote.tracing.opentelemetry.OpenTelemetryTracer; import org.openqa.selenium.virtualauthenticator.Credential; @@ -320,13 +320,11 @@ protected void startSession(Capabilities capabilities) { sessionId = new SessionId(response.getSessionId()); this.biDi = createBiDi(); } catch (Exception e) { - // If session creation fails, stop the driver service to prevent zombie processes - if (executor instanceof DriverCommandExecutor) { - try { - ((DriverCommandExecutor) executor).close(); - } catch (Exception ignored) { - // Ignore cleanup exceptions, we'll propagate the original failure - } + // Without a session id quit() is a no-op, so this is the only chance to release the + // executor's HTTP client and threads; with one, the constructor's quit() must still be able + // to end the remote session before it closes the executor + if (sessionId == null) { + closeExecutor(); } throw e; } @@ -566,6 +564,19 @@ public void quit() { execute(DriverCommand.QUIT); } finally { sessionId = null; + closeExecutor(); + } + } + + private void closeExecutor() { + if (!(executor instanceof Closeable)) { + return; + } + try { + ((Closeable) executor).close(); + } catch (Exception e) { + // Never mask the failure (or success) of the operation that triggered the clean-up + LOG.log(Level.FINE, "Failed to close the command executor", e); } } diff --git a/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java b/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java index 89a51edfd2cbb..3f8aa7f42d68c 100644 --- a/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java +++ b/java/src/org/openqa/selenium/remote/TracedCommandExecutor.java @@ -17,13 +17,14 @@ package org.openqa.selenium.remote; +import java.io.Closeable; import java.io.IOException; import java.util.Map; import java.util.Objects; import org.openqa.selenium.remote.tracing.Span; import org.openqa.selenium.remote.tracing.Tracer; -public class TracedCommandExecutor implements CommandExecutor { +public class TracedCommandExecutor implements CommandExecutor, Closeable { private final CommandExecutor delegate; private final Tracer tracer; @@ -51,4 +52,11 @@ public Response execute(Command command) throws IOException { return delegate.execute(command); } } + + @Override + public void close() throws IOException { + if (delegate instanceof Closeable) { + ((Closeable) delegate).close(); + } + } } diff --git a/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java b/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java index 177e89d2f076e..2326df0beeaf7 100644 --- a/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java +++ b/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java @@ -219,6 +219,10 @@ Response invokeExecute(Command command) throws IOException { @Override public void close() { - shutdownGracefully(NAME, executorService); + try { + shutdownGracefully(NAME, executorService); + } finally { + super.close(); + } } } diff --git a/java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java b/java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java index 5759b4f253ea8..2286b36c4dc08 100644 --- a/java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java +++ b/java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java @@ -24,6 +24,7 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -36,9 +37,12 @@ import java.io.IOException; import java.io.UncheckedIOException; +import java.net.ConnectException; import java.net.MalformedURLException; import java.net.URL; import java.time.Duration; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import java.util.UUID; import org.jspecify.annotations.NullMarked; @@ -49,12 +53,15 @@ import org.openqa.selenium.ImmutableCapabilities; import org.openqa.selenium.Platform; import org.openqa.selenium.SessionNotCreatedException; +import org.openqa.selenium.WebDriverException; 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.remote.service.DriverCommandExecutor; +import org.openqa.selenium.remote.tracing.empty.NullTracer; @Tag("UnitTests") class RemoteWebDriverInitializationTest { @@ -116,6 +123,102 @@ void closesDriverCommandExecutorWhenSessionCreationFailsAfterNewSessionResponse( verify(executor).close(); } + @Test + void closesHttpClientWhenSessionCreationFails() throws MalformedURLException { + HttpClient client = unreachableRemoteEnd(); + CommandExecutor executor = new HttpCommandExecutor(client, new URL("http://localhost:4444/")); + + assertThatExceptionOfType(WebDriverException.class) + .isThrownBy(() -> new RemoteWebDriver(executor, new ImmutableCapabilities())); + + verify(client).close(); + } + + @Test + void closesHttpClientOfTracedExecutorWhenSessionCreationFails() throws MalformedURLException { + HttpClient client = unreachableRemoteEnd(); + CommandExecutor executor = + new TracedCommandExecutor( + new HttpCommandExecutor(client, new URL("http://localhost:4444/")), new NullTracer()); + + assertThatExceptionOfType(WebDriverException.class) + .isThrownBy(() -> new RemoteWebDriver(executor, new ImmutableCapabilities())); + + verify(client).close(); + } + + @Test + void quitClosesHttpClientEvenIfQuitCommandFails() throws MalformedURLException { + HttpClient client = mock(HttpClient.class); + when(client.execute(any())) + .thenReturn(newSessionResponse()) + .thenThrow(new UncheckedIOException(new ConnectException("Connection refused"))); + RemoteWebDriver driver = + new RemoteWebDriver( + new HttpCommandExecutor(client, new URL("http://localhost:4444/")), + new ImmutableCapabilities()); + + assertThatExceptionOfType(WebDriverException.class).isThrownBy(driver::quit); + + verify(client).close(); + } + + @Test + void endsRemoteSessionBeforeClosingHttpClientWhenSetupFailsAfterSessionStarted() + throws MalformedURLException { + List events = new ArrayList<>(); + HttpClient client = mock(HttpClient.class); + when(client.execute(any())) + .thenAnswer( + invocation -> { + HttpRequest request = invocation.getArgument(0); + events.add(request.getMethod().name()); + return request.getMethod() == HttpMethod.POST + ? newSessionResponse(Map.of("webSocketUrl", "ws://localhost:4444/session/bidi")) + : new HttpResponse() + .setStatus(200) + .setContent(Contents.asJson(singletonMap("value", null))); + }); + doAnswer(invocation -> events.add("close")).when(client).close(); + HttpClient.Factory webSocketClientFactory = mock(HttpClient.Factory.class); + when(webSocketClientFactory.createClient(any(ClientConfig.class))) + .thenThrow(new IllegalStateException("Stub WebSocket client failure")); + + assertThatExceptionOfType(IllegalStateException.class) + .isThrownBy( + () -> + new RemoteWebDriver( + new HttpCommandExecutor(client, new URL("http://localhost:4444/")), + new ImmutableCapabilities(), + webSocketClientFactory, + ClientConfig.defaultConfig())) + .withMessage("Stub WebSocket client failure"); + + assertThat(events).startsWith("POST", "DELETE", "close"); + } + + private static HttpClient unreachableRemoteEnd() { + HttpClient client = mock(HttpClient.class); + when(client.execute(any())) + .thenThrow(new UncheckedIOException(new ConnectException("Connection refused"))); + return client; + } + + private static HttpResponse newSessionResponse() { + return newSessionResponse(Map.of()); + } + + private static HttpResponse newSessionResponse(Map capabilities) { + return new HttpResponse() + .setStatus(200) + .setContent( + Contents.asJson( + singletonMap( + "value", + Map.of( + "sessionId", UUID.randomUUID().toString(), "capabilities", capabilities)))); + } + @Test void constructorShouldThrowIfExecutorReturnsAResponseWithNullValueOnAnAttemptToStartASession() { CommandExecutor executor = WebDriverFixture.prepareExecutorMock(nullValueResponder);