WIP [dotnet] Reusable DriverService instance by Drivers

[nvborisenko]

User description

DriverService instance seems to be reusable by Drivers, but in fact no. As soon as Quit command is handled by Drivers, then underlying DriverService is disposed silently.

https://github.com/SeleniumHQ/selenium/blame/215e20b139f9af0c2db76b800472daed0633c73c/dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs#L128 - this is bad design.

Description

Motivation and Context

Fixes #14624 and #14633

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

enhancement

---

Description

• Introduced a disposeService parameter across various driver classes to control the disposal of driver services.
• Updated constructors and methods to accommodate the new disposeService parameter.
• Removed the DriverServiceCommandExecutor class and replaced its functionality with direct service management.
• Enhanced the disposal process in the WebDriver class to ensure proper resource management.

---

Changes walkthrough 📝

Relevant files

Enhancement

9 files ChromeDriver.cs Add disposeService parameter to ChromeDriver constructors dotnet/src/webdriver/Chrome/ChromeDriver.cs Added disposeService parameter to constructors. Updated constructor calls to include disposeService. Added method to handle custom Chrome commands. +14/-3    ChromiumDriver.cs Enhance ChromiumDriver with service disposal control          dotnet/src/webdriver/Chromium/ChromiumDriver.cs Introduced disposeDriverService field. Modified command executor method to start service. Updated dispose method to handle driver service disposal. +16/-11  EdgeDriver.cs Update EdgeDriver constructor for service disposal              dotnet/src/webdriver/Edge/EdgeDriver.cs Updated constructor to include disposeService parameter. +1/-1      FirefoxDriver.cs Update FirefoxDriver to start service in executor                dotnet/src/webdriver/Firefox/FirefoxDriver.cs Renamed command executor method to start service. Updated constructor to use new method. +6/-3      InternetExplorerDriver.cs Modify InternetExplorerDriver to start service in executor dotnet/src/webdriver/IE/InternetExplorerDriver.cs Renamed command executor method to start service. Updated constructor to use new method. +6/-3      DriverServiceCommandExecutor.cs Remove DriverServiceCommandExecutor class                                dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs Removed DriverServiceCommandExecutor class. +0/-163  SafariDriver.cs Update SafariDriver to start service in executor                  dotnet/src/webdriver/Safari/SafariDriver.cs Renamed command executor method to start service. Updated constructor to use new method. +6/-3      WebDriver.cs Ensure executor disposal in WebDriver class                            dotnet/src/webdriver/WebDriver.cs Added disposal of executor in Dispose method. +1/-0      DriverFactory.cs Modify DriverFactory to pass service in driver instantiation dotnet/test/common/Environment/DriverFactory.cs Updated driver instantiation to include service parameter. +1/-2

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Resource Leak The driverService field is not being disposed in all scenarios. Ensure proper disposal of resources. Code Duplication Multiple constructors with similar logic. Consider refactoring to reduce duplication. Naming Inconsistency Method renamed from GenerateDriverServiceCommandExecutor to StartDriverServiceCommandExecutor. Ensure consistency across all driver classes.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add null check before disposing of the executor to prevent potential exceptions Consider wrapping the executor disposal in a null check to prevent potential null reference exceptions. dotnet/src/webdriver/WebDriver.cs [732] -this.executor.Dispose(); +this.executor?.Dispose(); Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding a null check before disposing of the executor is a good practice to prevent potential null reference exceptions, which could lead to runtime errors. This suggestion addresses a possible issue effectively.	8
	Add null check for the service parameter to prevent potential null reference exceptions Consider adding a null check before using the service parameter in the CreateDriverWithOptions method to prevent potential null reference exceptions. dotnet/test/common/Environment/DriverFactory.cs [158] +if (service == null) +{ + throw new ArgumentNullException(nameof(service)); +} driver = (IWebDriver)Activator.CreateInstance(driverType, service, true); Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion to add a null check for the service parameter is important to prevent null reference exceptions, which could cause runtime errors. This is a valuable addition to ensure robustness in the method.	8
Enhancement	✅ Implement constructor chaining to reduce code duplication and improve maintainability Suggestion Impact: The suggestion to implement constructor chaining was applied, reducing code duplication by removing the disposeService parameter from the constructor calls. code diff: @@ -136,12 +136,7 @@ /// <param name="service">The <see cref="ChromeDriverService"/> to use.</param> /// <param name="options">The <see cref="ChromeOptions"/> used to initialize the driver.</param> public ChromeDriver(ChromeDriverService service, ChromeOptions options) - : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService: true) - { - } - - public ChromeDriver(ChromeDriverService service, ChromeOptions options, bool disposeService) - : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService) + : this(service, options, RemoteWebDriver.DefaultCommandTimeout) { } @@ -152,13 +147,7 @@ /// <param name="options">The <see cref="ChromeOptions"/> to be used with the Chrome driver.</param> /// <param name="commandTimeout">The maximum amount of time to wait for each command.</param> public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout) - : base(service, options, commandTimeout, disposeService: true) - { - this.AddCustomChromeCommands(); - } - - public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout, bool disposeService) - : base(service, options, commandTimeout, disposeService) + : base(service, options, commandTimeout) { Consider using a constructor chaining pattern to reduce code duplication across the various constructors. This can help maintain consistency and make future updates easier. dotnet/src/webdriver/Chrome/ChromeDriver.cs [138-164] public ChromeDriver(ChromeDriverService service, ChromeOptions options) - : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService: true) + : this(service, options, RemoteWebDriver.DefaultCommandTimeout) { } public ChromeDriver(ChromeDriverService service, ChromeOptions options, bool disposeService) : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService) { } public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout) - : base(service, options, commandTimeout, disposeService: true) + : this(service, options, commandTimeout, true) { - this.AddCustomChromeCommands(); } public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout, bool disposeService) : base(service, options, commandTimeout, disposeService) { this.AddCustomChromeCommands(); } Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to implement constructor chaining is valid as it reduces code duplication and enhances maintainability. The improved code correctly applies the chaining pattern, making the constructors more concise and easier to update in the future.	7
	Use null-coalescing assignment operator for more concise field initialization Consider using a null-coalescing assignment operator ??= to simplify the initialization of the driverService field if it's not already set. dotnet/src/webdriver/Chromium/ChromiumDriver.cs [131-136] protected ChromiumDriver(ChromiumDriverService service, ChromiumOptions options, TimeSpan commandTimeout, bool disposeService) : base(StartDriverServiceCommandExecutor(service, options, commandTimeout), ConvertOptionsToCapabilities(options)) { - this.driverService = service; + this.driverService ??= service; this.disposeDriverService = disposeService; this.optionsCapabilityName = options.CapabilityName; } Apply this suggestion Suggestion importance[1-10]: 5 Why: The suggestion to use a null-coalescing assignment operator is correct and makes the code more concise. However, the impact is minor since the field is being set in the constructor and is unlikely to be null at this point.	5

💡 Need additional feedback ? start a PR chat

[nvborisenko]

To move further we have to decide:

• Just fix "incorrect/old" behavior, which is not source/binary breaking change, but changes "old" behavior
• Or keep "old" behavior and introduce an ability to specify whether ChromeDriver should dispose underlying ChromeDriverService (chromedriver.exe)

@YevgeniyShunevych your input is appreciated here.

[nvborisenko]

I vote for just fix "old" behavior. If user instantiates DriverService object, he is responsible to Dispose it. This is general rule to Dispose everything which is Disposable. So let's fix/break it. If new issues will be reported, then we can say "You instantiated, you should dispose it".

[nvborisenko]

Reusing shared driver service instance is ~2x faster.

Simple test: start 50 chrome browsers simultaneously:

• using new instance of service: Duration: 1.2 min
• re-using single instance of service: Duration: 44.4 sec

[nvborisenko]

geckodriver and safaridriver don't like concurrent sessions. Good news we are as a library throw human-friendly exception. Reported issue for geckodriver: mozilla/geckodriver#2209

[nvborisenko]

Let's return back try/catch pattern as it was before. Then we can improve later.

[nvborisenko]

I still don't know how to proceed and be user-friendly:

• fix incorrect old behavior, but it "breaks" all who instantiates service object explicitly
• keep old behavior, and provide an option to implicitly dispose service object (enabled by default)

The first point is an ultimate, it would be great to see it in Selenium 5. The second is more friendly, but it adds complexity.

[RenderMichael]

In exchange for potentially more complexity, we can avoid breaking changes and make behavior more clear by adding a new method on the driver service types called CreatePersistedService(…).

This way, all the disposal magic won’t have to live in the driver disposal, the code could potentially be simpler. We will end up with “single-use driver service” and “reusable driver service”, depending on how it is created.

[nvborisenko]

In exchange for potentially more complexity, we can avoid breaking changes and make behavior more clear by adding a new method on the driver service types called CreatePersistedService(…).

This way, all the disposal magic won’t have to live in the driver disposal, the code could potentially be simpler. We will end up with “single-use driver service” and “reusable driver service”, depending on how it is created.

It doesn't resolve the initial issue: "who creates those should dispose".

[RenderMichael]

If we want to fix the lifetime ownership issue without breaking anything, we can solve it with a new method.

Existing:

var driver = new ChromeDriver();
driver.Dispose(); // Owns its own service, service gets disposed here

var driverService = ChromeDriverService.CreateDefaultService();
var driver = new ChromeDriver(driverService);
driver.Dispose(); // backwards-compatible, disposes the service as well

Proposed:

var driverService = ChromeDriverService.CreatePersistedService();
var driver = new ChromeDriver(driverService); // starts new chromedriver.exe
driver.Dispose(); // does not dispose service
driverService.Dispose();

[nvborisenko]

It is a mess. Today .CreatePersistedService(), tomorrow .CreatePersistedFor10MinutesService().

[RenderMichael]

I think a new overload is justified in the face of a breaking change, we can make a DriverServiceOptions in the future if things should be made more extensible.

[nvborisenko]

The direction of this PR is very good. Now it works properly (excluding our internal some tests). In my opinion the old behavior is not right.

I propose to postpone for v5, where we can announce new "right" behavior.

[nvborisenko]

Good decision is:

new FirefoxDriver(); // it always starts new service and dispose it, current behavior;
new FirefoxDriver(service); // it takes existing service and dispose it, current behavior;

new FirefoxDriver(service, bool disposeService); // introduce new flag whether existing service should be disposed

Implementation notes: it should not be optional! Only new method overload.

Also don't refactor unit tests, keep existing behavior.

The key changes are identified, let's fix this bad design and forget. This approach is not a breaking change.