[java] Fix 15634/ensure driver closed java

[iampopovich]

User description

🔗 Related Issues

partially fixes #15634 for java

💥 What does this PR do?

similar changes as for python in #15636

🔧 Implementation Notes

This pull request enhances the robustness of driver service management in Selenium by ensuring services are properly stopped when session creation fails. It also introduces unit tests across multiple browser drivers to validate this behavior. The most important changes include updates to RemoteWebDriver for cleanup logic and new tests for driver service cleanup across Chrome, Edge, Firefox, and Safari.

Driver Service Cleanup Enhancements:

• java/src/org/openqa/selenium/remote/RemoteWebDriver.java: Added logic to stop the driver service if session creation fails, preventing zombie processes. This includes a try-catch block in the startSession method and the use of DriverCommandExecutor for cleanup. [1] [2]

Unit Tests for Driver Service Cleanup:

• java/test/org/openqa/selenium/chrome/ChromeDriverServiceCleanupTest.java: Added a test to verify that the Chrome driver service stops when session creation fails due to invalid options.
• java/test/org/openqa/selenium/edge/EdgeDriverServiceTest.java: Added a test to ensure the Edge driver service stops when session creation fails due to invalid options.
• java/test/org/openqa/selenium/firefox/GeckoDriverServiceTest.java: Added a test to confirm that the Firefox driver service stops when session creation fails due to an invalid binary path.
• java/test/org/openqa/selenium/safari/SafariDriverServiceTest.java: Added a test to verify that the Safari driver service stops when session creation fails due to invalid capabilities.

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix, Tests

---

Description

• Fix driver service cleanup when session creation fails

• Add unit tests for service cleanup across Chrome, Edge, Firefox, Safari

• Prevent zombie processes by stopping services on session failure

• Enhance error handling in RemoteWebDriver session initialization

---

Changes diagram

flowchart LR
  A["Session Creation"] --> B["Try Block"]
  B --> C["Execute NEW_SESSION"]
  C --> D["Success?"]
  D -->|Yes| E["Set Capabilities & SessionId"]
  D -->|No| F["Catch Exception"]
  F --> G["Check DriverCommandExecutor"]
  G --> H["Stop Service"]
  H --> I["Throw Original Exception"]

Loading

---

Changes walkthrough 📝

	Relevant files
Bug fix	RemoteWebDriver.java Add session failure cleanup logic                                                java/src/org/openqa/selenium/remote/RemoteWebDriver.java Wrap session creation in try-catch block Add service cleanup logic when session fails Import DriverCommandExecutor for cleanup operations Prevent zombie processes by stopping services +46/-33
Tests	ChromeDriverServiceCleanupTest.java Add Chrome service cleanup test                                                    java/test/org/openqa/selenium/chrome/ChromeDriverServiceCleanupTest.java New test class for Chrome driver service cleanup Test service stops when session creation fails Use invalid user-data-dir to trigger failure Verify service is not running after failure +48/-0    EdgeDriverServiceTest.java Add Edge service cleanup test                                                        java/test/org/openqa/selenium/edge/EdgeDriverServiceTest.java Add new test method for service cleanup Import additional assertion methods Test Edge driver service stops on failure Use invalid user-data-dir argument +20/-0    GeckoDriverServiceTest.java Add Firefox service cleanup test                                                  java/test/org/openqa/selenium/firefox/GeckoDriverServiceTest.java Add service cleanup test for Firefox Import assertion methods for testing Use invalid binary path to trigger failure Verify service stops after session failure +21/-0    SafariDriverServiceTest.java Add Safari service cleanup test                                                    java/test/org/openqa/selenium/safari/SafariDriverServiceTest.java Add service cleanup test for Safari Import assertion utilities Use invalid capability to cause failure Test service stops when session creation fails +21/-0

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Exception Handling The catch block catches all exceptions but only handles DriverCommandExecutor cleanup. Other executor types may need different cleanup approaches, and the broad exception catch might mask important error details. } 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 } } throw e; } Test Inconsistency The Firefox test catches generic Exception instead of SessionNotCreatedException like other browser tests, which may not properly validate the specific failure scenario being tested. assertThatExceptionOfType(Exception.class) .isThrownBy(() -> new FirefoxDriver(serviceSpy, options)); Test Reliability The Safari test uses an invalid capability which may not reliably trigger session creation failure across different Safari versions or configurations, potentially leading to flaky tests. options.setCapability("invalidCapability", "invalidValue");

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Learned best practice	Add service state validation The cleanup logic should check if the service is running before attempting to close it to prevent unnecessary exceptions. Add a null check and service state validation before calling close() to ensure safer resource cleanup. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [286-296] } catch (Exception e) { // If session creation fails, stop the driver service to prevent zombie processes if (executor instanceof DriverCommandExecutor) { try { - ((DriverCommandExecutor) executor).close(); + DriverCommandExecutor driverExecutor = (DriverCommandExecutor) executor; + if (driverExecutor.getService() != null && driverExecutor.getService().isRunning()) { + driverExecutor.close(); + } } catch (Exception ignored) { // Ignore cleanup exceptions, we'll propagate the original failure } } throw e; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Ensure proper resource cleanup by closing sockets, processes, and drivers in finally blocks or using context managers, and check process state before attempting to terminate to prevent resource leaks and exceptions.	Low
Update

[pujagani]

The changes look good to me. Will wait for CI to pass before merging it.

[pujagani]

Thank you! @iampopovich