[java] Do not leak the HttpClient on startup failure by joerg1985 · Pull Request #18047 · SeleniumHQ/selenium

joerg1985 · 2026-09-17T15:51:37Z

🔗 Related Issues

When new RemoteWebDriver(...) fails to start a session, the HttpClient inside the command executor is never closed. It leaks until garbage collection.

💥 What does this PR do?

HttpCommandExecutor implements Closeable. close() closes its HttpClient.
TracedCommandExecutor implements Closeable. close() closes the executor it wraps. Tracing is on by default, so this is the executor most users get.
DriverCommandExecutor.close() also closes its HttpClient now.
RemoteWebDriver closes the executor if it implements Closeable:
- when starting the session fails
- in quit(), even if the QUIT command fails

🔧 Implementation Notes

If closing the executor fails, the error is logged at FINE. It never hides the original exception.
Closing twice is safe. HttpClient is a Closeable, and Closeable.close() must have no effect when called again.

💡 Additional Considerations

Behavior change: if you pass your own executor to new RemoteWebDriver(executor, caps), it is now closed when the session fails to start. You can't reuse that executor to retry. Create a new one instead. Retrying with new RemoteWebDriver(url, caps) is not affected, because it creates a new executor each time.

If users need to reuse their executor, we can add a way for RemoteWebDriver to only close executors it created itself.

🤖 AI assistance

No substantial AI assistance used
AI assisted (complete below)
- Tool(s): Claude Code
- What was generated: the follow-up commit (5cd71aa): closing in TracedCommandExecutor, the RemoteWebDriver and DriverCommandExecutor clean-up changes, and the new unit tests
- I reviewed all AI output and can explain the change

🔄 Types of changes

Bug fix (backwards compatible)

🤖 Generated with Claude Code

qodo-code-review · 2026-09-17T16:55:42Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

qodo-code-review · 2026-10-01T13:26:33Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. ~~HTTP client cleanup lacks a regression test~~ ✓ Resolved

Description

RemoteWebDriver.startSession() now closes any HttpCommandExecutor on failure, but the focused
initialization test verifies only a mocked DriverCommandExecutor. A failed session using a plain
HTTP executor—the newly covered case—has no test asserting that its client closes and the original
failure propagates.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R324-326]

+      try (var hce =
+          executor instanceof HttpCommandExecutor ? (HttpCommandExecutor) executor : null) {
+        throw e;

Evidence
The changed type check extends failure cleanup to plain HTTP executors, while the focused test
exercises only a mocked driver-service executor and verifies its close() call.
AGENTS.md: Cover Solutions With Small, Contract-Faithful Tests
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[322-328]
java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[105-116]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The startup-failure cleanup adds support for plain HTTP executors without a focused test of that path.

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[105-116]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[322-328]

## Recommended Fix
Add a small test with a plain HTTP executor and a controllable client. Make session creation fail, then assert that the client closes and the original exception propagates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Default remote sessions still leak on failure~~ ✓ Resolved

Description

startSession closes the executor only when it is directly an HttpCommandExecutor, but the
default tracing configuration wraps it in TracedCommandExecutor. When session creation fails
before a session ID is assigned, the constructor’s subsequent quit() returns immediately, leaving
the wrapped HTTP client open.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R324-325]
+      try (var hce =
+          executor instanceof HttpCommandExecutor ? (HttpCommandExecutor) executor : null) {

Evidence
Default tracing constructs a TracedCommandExecutor around the HTTP executor; that wrapper does not
expose close. The failure path tests the wrapper’s type, and the constructor’s fallback quit cannot
clean up before a session ID exists.
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[183-196]
java/src/org/openqa/selenium/remote/TracedCommandExecutor.java[22-51]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[311-328]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[246-254]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[551-553]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Default tracing wraps the HTTP command executor, so the new direct-type check skips cleanup on session-start failure.
## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[322-328]
- java/src/org/openqa/selenium/remote/TracedCommandExecutor.java[22-51]
## Recommended Fix
Make the tracing wrapper forward closure to its delegate, and have the startup-failure path close that wrapper while preserving the original exception.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Quit fails for clients closed only once~~ ✓ Resolved

Description

HttpCommandExecutor.execute() closes the HTTP client after a successful QUIT response, and
RemoteWebDriver.quit() then closes the same executor through try-with-resources. When a caller
supplies a client whose close() is not idempotent, that second close can throw even though the
quit command succeeded.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R555-556]

+    try (HttpCommandExecutor httpCommandExecutor =
+        (executor instanceof HttpCommandExecutor) ? (HttpCommandExecutor) executor : null) {

Evidence
The successful QUIT path calls client.close() before returning, while the new try-with-resources
block subsequently invokes the executor’s close(), which calls client.close() again. Callers can
supply their own HttpClient, and its interface provides no idempotency guarantee.
AGENTS.md: Preserve Public API and ABI Compatibility and Deprecate Before Removal
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[555-568]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[91-108]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[252-257]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[275-278]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[86-94]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[250-260]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[555-569]
java/src/org/openqa/selenium/remote/http/HttpClient.java[23-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A successful quit closes the HTTP client in `execute()` and again through `RemoteWebDriver.quit()` cleanup, which a caller-supplied client may not tolerate.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[555-569]
- java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[252-277]

## Recommended Fix
Route successful-QUIT and explicit executor cleanup through one idempotent close operation so the underlying HTTP client is closed at most once, while retaining cleanup when the quit command fails.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. ~~Failed setup leaves browser sessions open~~ ✓ Resolved

Description

startSession() calls closeExecutor() even when it has already assigned sessionId, closing the
HTTP client before the constructor’s recovery quit() runs. If createBiDi() throws while creating
its WebSocket client, the subsequent QUIT uses the closed client, so the remote session is not
terminated.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R323-325]

+      // 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();

Evidence
The session ID is assigned immediately before createBiDi(), whose client factory call can throw.
The constructor then calls quit(), but the new catch-block cleanup has already closed the HTTP
client; the JDK implementation clears its client and shuts down its executor on close.
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[319-326]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[244-251]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[459-471]
java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java[578-603]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failure after `startSession()` assigns a session ID closes the command executor before the constructor can send QUIT, leaving the remote session active.
## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[319-325]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[244-251]
## Recommended Fix
Close the executor immediately on startup failure only when no session ID exists. When a session ID exists, let the constructor attempt `quit()` before closing the executor; preserve the original startup exception if cleanup fails.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context sources

Review mode: ⚖️ Balanced: This behavioral change alters RemoteWebDriver startup-failure cleanup and session teardown ordering, with meaningful resource-lifecycle and remote-session correctness risk despite being localized.

Tip of the day

💡 Did you know, you can keep summaries lean with Findings visible per group, which tucks the rest behind a View link

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit f9b2474

Results up to commit 7e5404c ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~HTTP client cleanup lacks a regression test~~ ✓ Resolved

Description

RemoteWebDriver.startSession() now closes any HttpCommandExecutor on failure, but the focused
initialization test verifies only a mocked DriverCommandExecutor. A failed session using a plain
HTTP executor—the newly covered case—has no test asserting that its client closes and the original
failure propagates.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R324-326]

+      try (var hce =
+          executor instanceof HttpCommandExecutor ? (HttpCommandExecutor) executor : null) {
+        throw e;

Evidence
The changed type check extends failure cleanup to plain HTTP executors, while the focused test
exercises only a mocked driver-service executor and verifies its close() call.
AGENTS.md: Cover Solutions With Small, Contract-Faithful Tests
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[322-328]
java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[105-116]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The startup-failure cleanup adds support for plain HTTP executors without a focused test of that path.

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[105-116]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[322-328]

## Recommended Fix
Add a small test with a plain HTTP executor and a controllable client. Make session creation fail, then assert that the client closes and the original exception propagates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Quit fails for clients closed only once~~ ✓ Resolved

Description

HttpCommandExecutor.execute() closes the HTTP client after a successful QUIT response, and
RemoteWebDriver.quit() then closes the same executor through try-with-resources. When a caller
supplies a client whose close() is not idempotent, that second close can throw even though the
quit command succeeded.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R555-556]

+    try (HttpCommandExecutor httpCommandExecutor =
+        (executor instanceof HttpCommandExecutor) ? (HttpCommandExecutor) executor : null) {

Evidence
The successful QUIT path calls client.close() before returning, while the new try-with-resources
block subsequently invokes the executor’s close(), which calls client.close() again. Callers can
supply their own HttpClient, and its interface provides no idempotency guarantee.
AGENTS.md: Preserve Public API and ABI Compatibility and Deprecate Before Removal
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[555-568]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[91-108]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[252-257]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[275-278]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[86-94]
java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[250-260]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[555-569]
java/src/org/openqa/selenium/remote/http/HttpClient.java[23-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A successful quit closes the HTTP client in `execute()` and again through `RemoteWebDriver.quit()` cleanup, which a caller-supplied client may not tolerate.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[555-569]
- java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[252-277]

## Recommended Fix
Route successful-QUIT and explicit executor cleanup through one idempotent close operation so the underlying HTTP client is closed at most once, while retaining cleanup when the quit command fails.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Default remote sessions still leak on failure~~ ✓ Resolved

Description

startSession closes the executor only when it is directly an HttpCommandExecutor, but the
default tracing configuration wraps it in TracedCommandExecutor. When session creation fails
before a session ID is assigned, the constructor’s subsequent quit() returns immediately, leaving
the wrapped HTTP client open.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R324-325]
+      try (var hce =
+          executor instanceof HttpCommandExecutor ? (HttpCommandExecutor) executor : null) {

Evidence
Default tracing constructs a TracedCommandExecutor around the HTTP executor; that wrapper does not
expose close. The failure path tests the wrapper’s type, and the constructor’s fallback quit cannot
clean up before a session ID exists.
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[183-196]
java/src/org/openqa/selenium/remote/TracedCommandExecutor.java[22-51]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[311-328]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[246-254]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[551-553]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Default tracing wraps the HTTP command executor, so the new direct-type check skips cleanup on session-start failure.
## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[322-328]
- java/src/org/openqa/selenium/remote/TracedCommandExecutor.java[22-51]
## Recommended Fix
Make the tracing wrapper forward closure to its delegate, and have the startup-failure path close that wrapper while preserving the original exception.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit 5cd71aa ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Failed setup leaves browser sessions open~~ ✓ Resolved

Description

startSession() calls closeExecutor() even when it has already assigned sessionId, closing the
HTTP client before the constructor’s recovery quit() runs. If createBiDi() throws while creating
its WebSocket client, the subsequent QUIT uses the closed client, so the remote session is not
terminated.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R323-325]

+      // 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();

Evidence
The session ID is assigned immediately before createBiDi(), whose client factory call can throw.
The constructor then calls quit(), but the new catch-block cleanup has already closed the HTTP
client; the JDK implementation clears its client and shuts down its executor on close.
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[319-326]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[244-251]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[459-471]
java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java[578-603]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failure after `startSession()` assigns a session ID closes the command executor before the constructor can send QUIT, leaving the remote session active.
## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[319-325]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[244-251]
## Recommended Fix
Close the executor immediately on startup failure only when no session ID exists. When a session ID exists, let the constructor attempt `quit()` before closing the executor; preserve the original startup exception if cleanup fails.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tracing is enabled by default, so RemoteWebDriver usually wraps the HttpCommandExecutor in a TracedCommandExecutor, which the previous HttpCommandExecutor type check skipped. TracedCommandExecutor now forwards close() to its delegate, and RemoteWebDriver closes any Closeable executor when session start fails or on quit(), without letting a close failure mask the original outcome. Also replaces the try-with-resources clean-up idioms with plain try/finally and adds unit tests for plain, traced and quit paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

qodo-code-review · 2026-10-01T14:31:26Z

Code review by qodo was updated up to the latest commit 5cd71aa

If startSession() fails after the session id is set (e.g. while setting up BiDi), the constructor calls quit() to end the remote session. Closing the executor in startSession() first made that QUIT fail, leaving the remote session open. Only close it there when there is no session id; otherwise quit() sends QUIT and then closes the executor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

qodo-code-review · 2026-10-01T14:49:16Z

Code review by qodo was updated up to the latest commit e610932

qodo-code-review · 2026-10-01T14:51:51Z

No code changes since the last review — review skipped

selenium-ci added the C-java Java Bindings label Sep 17, 2026

joerg1985 force-pushed the close-http-executor branch from 29f6d51 to 7b117e1 Compare September 17, 2026 15:55

[java] Do not leak the HttpClient on startup failure

100ac7a

joerg1985 force-pushed the close-http-executor branch from 7b117e1 to 100ac7a Compare September 17, 2026 16:22

joerg1985 marked this pull request as ready for review September 17, 2026 16:55

diemol mentioned this pull request Sep 30, 2026

[java] Restore HttpCommandExecutor client and httpClientFactory fields as deprecated #18098

Merged

2 of 3 tasks

Merge branch 'trunk' into close-http-executor

7e5404c

qodo-code-review Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread java/src/org/openqa/selenium/remote/RemoteWebDriver.java Outdated

Comment thread java/src/org/openqa/selenium/remote/RemoteWebDriver.java Outdated

Comment thread java/src/org/openqa/selenium/remote/RemoteWebDriver.java Outdated

qodo-code-review Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread java/src/org/openqa/selenium/remote/RemoteWebDriver.java Outdated

Merge branch 'trunk' into close-http-executor

f9b2474

diemol approved these changes Oct 1, 2026

View reviewed changes

diemol merged commit a786547 into trunk Oct 1, 2026
33 checks passed

diemol deleted the close-http-executor branch October 1, 2026 15:23

diemol mentioned this pull request Oct 1, 2026

[java] Add RemoteWebDriver.getClientFactory() and move HttpCommandExecutor field removal to 4.53 #18111

Merged

1 of 3 tasks

Navigation Menu

Repository navigation

[java] Do not leak the HttpClient on startup failure - #18047

[java] Do not leak the HttpClient on startup failure#18047
diemol merged 5 commits into
trunkfrom
close-http-executor

joerg1985 commented Sep 17, 2026 •

edited by diemol

Loading

Uh oh!

qodo-code-review Bot commented Sep 17, 2026

Uh oh!

qodo-code-review Bot commented Oct 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Oct 1, 2026

Uh oh!

qodo-code-review Bot commented Oct 1, 2026

Uh oh!

qodo-code-review Bot commented Oct 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

joerg1985 commented Sep 17, 2026 • edited by diemol Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🤖 AI assistance

🔄 Types of changes

Uh oh!

qodo-code-review Bot commented Sep 17, 2026

Qodo reviews are paused for this user.

Uh oh!

qodo-code-review Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Oct 1, 2026

Uh oh!

qodo-code-review Bot commented Oct 1, 2026

Uh oh!

qodo-code-review Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joerg1985 commented Sep 17, 2026 •

edited by diemol

Loading

qodo-code-review Bot commented Oct 1, 2026 •

edited

Loading

qodo-code-review Bot commented Oct 1, 2026 •

edited

Loading