Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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();
}
}
26 changes: 17 additions & 9 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,14 +320,9 @@ 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
}
}
// quit() is a no-op without a session id, so this is the only chance to release the
// executor's HTTP client and threads before the caller retries with a new driver
closeExecutor();
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Outdated
throw e;
}
}
Expand Down Expand Up @@ -566,6 +561,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 @@ -36,6 +36,7 @@

import java.io.IOException;
import java.io.UncheckedIOException;
import java.net.ConnectException;
import java.net.MalformedURLException;
import java.net.URL;
import java.time.Duration;
Expand All @@ -49,12 +50,14 @@
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.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 +119,65 @@ 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();
}

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 new HttpResponse()
.setStatus(200)
.setContent(
Contents.asJson(
singletonMap(
"value",
Map.of(
"sessionId", UUID.randomUUID().toString(),
"capabilities", new ImmutableCapabilities().asMap()))));
}

@Test
void constructorShouldThrowIfExecutorReturnsAResponseWithNullValueOnAnAttemptToStartASession() {
CommandExecutor executor = WebDriverFixture.prepareExecutorMock(nullValueResponder);
Expand Down
Loading