Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion java/src/org/openqa/selenium/remote/HttpCommandExecutor.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String, CommandInfo> additionalCommands;
Expand Down Expand Up @@ -270,4 +271,9 @@ public Response execute(Command command) throws IOException {
throw e;
}
}

@Override
public void close() {
client.close();
}
}
27 changes: 19 additions & 8 deletions java/src/org/openqa/selenium/remote/RemoteWebDriver.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}
}

Expand Down
10 changes: 9 additions & 1 deletion java/src/org/openqa/selenium/remote/TracedCommandExecutor.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ Response invokeExecute(Command command) throws IOException {

@Override
public void close() {
shutdownGracefully(NAME, executorService);
try {
shutdownGracefully(NAME, executorService);
} finally {
super.close();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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<String> 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<String, Object> 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);
Expand Down
Loading