[dotnet] Propagate service asynchronicity to the command executor.

[RenderMichael]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Now that the driver command execution is asynchronous (async ExecuteAsync), we can propagate the Driver Start asynchronicity up to that method.

Motivation and Context

The NewSession command does sync-over-async, which is bad practice. Now that we have async ExecuteAsync, that can be minimized.

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, Other

---

Description

• Introduced asynchronous methods for DriverService operations.

  • Added StartAsync and DisposeAsync methods for asynchronous service handling.
  • Deprecated synchronous methods like Start and Dispose.

• Enhanced thread safety and error handling in DriverService.

  • Improved initialization checks with IsInitializedAsync.
  • Refactored WaitForServiceInitialization to WaitForServiceInitializationAsync.

• Updated ICommandServer interface to support asynchronous operations.

  • Added StartAsync method and implemented IAsyncDisposable.

• Adjusted DriverServiceCommandExecutor to use asynchronous service start.

---

Changes walkthrough 📝

Relevant files

Enhancement

DriverService.cs Added asynchronous methods and improved thread safety in DriverService dotnet/src/webdriver/DriverService.cs Added asynchronous methods like StartAsync, DisposeAsync, and IsInitializedAsync. Deprecated synchronous methods and marked them as obsolete. Enhanced thread safety and error handling during service initialization and disposal. Refactored service initialization logic to use asynchronous patterns. +95/-50  DriverServiceCommandExecutor.cs Updated command executor to use asynchronous service start dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs Updated ExecuteAsync to use StartAsync for starting the service. Ensured compatibility with asynchronous service operations. +1/-1      ICommandServer.cs Enhanced ICommandServer with asynchronous operations          dotnet/src/webdriver/Remote/ICommandServer.cs Added StartAsync method to support asynchronous server start. Implemented IAsyncDisposable for asynchronous resource disposal. Deprecated synchronous Start method. +9/-1

---

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 🔍

(Review updated until commit 3d085be)

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 Error Handling The error handling in StartAsync could leave the service in an inconsistent state if initialization fails after process start but before service is available this.driverServiceProcess.Start(); } catch { this.driverServiceProcess.Dispose(); throw; } } bool serviceAvailable = await this.WaitForServiceInitializationAsync().ConfigureAwait(false); DriverProcessStartedEventArgs processStartedEventArgs = new DriverProcessStartedEventArgs(this.driverServiceProcess); Race Condition The IsInitializedAsync method creates a new HttpClient for each check, which could lead to socket exhaustion under high load or repeated calls using (var httpClient = new HttpClient()) { httpClient.DefaultRequestHeaders.ConnectionClose = true; httpClient.Timeout = TimeSpan.FromSeconds(5); Uri serviceHealthUri = new Uri(this.ServiceUrl, new Uri(DriverCommand.Status, UriKind.Relative)); using (var response = await httpClient.GetAsync(serviceHealthUri)) { // Checking the response from the 'status' end point. Note that we are simply checking // that the HTTP status returned is a 200 status, and that the response has the correct // Content-Type header. A more sophisticated check would parse the JSON response and // validate its values. At the moment we do not do this more sophisticated check. isInitialized = response.StatusCode == HttpStatusCode.OK && response.Content.Headers.ContentType is { MediaType: string mediaType } && mediaType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase); } } Resource Cleanup The DisposeAsync implementation calls both async and sync dispose methods which could lead to duplicate cleanup attempts public async ValueTask DisposeAsync() { await DisposeAsyncCore(); Dispose(false); GC.SuppressFinalize(this); }

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 3d085be
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent potential deadlock in synchronous operation The IsInitialized property's implementation creates a new Task and blocks on it synchronously using GetAwaiter().GetResult(), which can lead to deadlocks. Consider using a cached task or implementing a different synchronous check. dotnet/src/webdriver/DriverService.cs [175-182] [Obsolete("Use the asynchronous method IsInitializedAsync")] protected virtual bool IsInitialized { get { - return Task.Run(async () => await IsInitializedAsync()).GetAwaiter().GetResult(); + return IsInitializedAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } } Apply this suggestion Suggestion importance[1-10]: 9 __ Why: The suggestion addresses a critical issue where using Task.Run with GetAwaiter().GetResult() can cause deadlocks in synchronous operations. The proposed fix using ConfigureAwait(false) is the correct pattern for sync-over-async scenarios.	High
	Fix deadlock risk in disposal The Dispose method synchronously blocks on an async operation using Task.Run and GetAwaiter().GetResult(), which can cause deadlocks. Consider implementing proper synchronous disposal. dotnet/src/webdriver/DriverService.cs [297-308] protected virtual void Dispose(bool disposing) { if (!this.isDisposed) { if (disposing) { - Task.Run(() => this.StopAsync()).GetAwaiter().GetResult(); + this.StopAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } this.isDisposed = true; } } Apply this suggestion Suggestion importance[1-10]: 9 __ Why: The suggestion identifies and fixes a serious issue where the Dispose method could deadlock due to improper sync-over-async pattern. The proposed solution correctly implements synchronous disposal using ConfigureAwait(false).	High
Learned best practice	Add null validation check for required service field to prevent potential NullReferenceException The service field should be validated for null before being used in ExecuteAsync(). Add a null check at the start of the method to prevent potential NullReferenceException when calling service.StartAsync(). dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs [117-121] public async Task<Response> ExecuteAsync(Command commandToExecute) { if (commandToExecute == null) { throw new ArgumentNullException(nameof(commandToExecute), "Command to execute cannot be null"); } + + ArgumentNullException.ThrowIfNull(service, nameof(service)); Response toReturn; if (commandToExecute.Name == DriverCommand.NewSession) { await this.service.StartAsync().ConfigureAwait(false); } [To ensure code accuracy, apply this suggestion manually]	Low

---

Previous suggestions

Suggestions up to commit 8fc36d2

Category	Suggestion	Impact
Possible issue	Add disposed state validation Add a check for disposed state before starting the service to prevent potential resource leaks and invalid operations. dotnet/src/webdriver/DriverService.cs [222-226] public void Start() { + if (this.isDisposed) + { + throw new ObjectDisposedException(nameof(DriverService)); + } + if (this.driverServiceProcess is null) { lock (this.driverServiceProcessLock) Suggestion importance[1-10]: 9 __ Why: Adding a disposed state check is crucial for preventing resource leaks and invalid operations. This is a critical safety measure that should be implemented in any disposable class to maintain proper object lifecycle management.	High
	Ensure process cleanup on failure Move the process disposal to a finally block to ensure cleanup even if WaitForServiceInitialization() throws an exception. dotnet/src/webdriver/DriverService.cs [264-268] +try +{ + // ... existing initialization code ... +} catch { - driverServiceProcess.Dispose(); throw; } +finally +{ + if (!serviceAvailable) + { + driverServiceProcess.Dispose(); + } +} Suggestion importance[1-10]: 4 __ Why: While the suggestion aims to improve cleanup handling, the current implementation already properly disposes of the process on failure. Moving it to a finally block wouldn't provide significant benefits as the process needs to be kept alive if initialization succeeds.	Low
Learned best practice	Move parameter validation checks before lock acquisition to fail fast and avoid unnecessary lock contention Add null validation check for DriverServicePath at the start of the method, before entering the lock block. This will fail fast and avoid unnecessary lock acquisition if the path is null but executable name is provided. dotnet/src/webdriver/DriverService.cs [224-238] +if (this.DriverServicePath != null && this.DriverServiceExecutableName is null) +{ + throw new InvalidOperationException("If the driver service path is specified, the driver service executable name must be as well"); +} + if (this.driverServiceProcess is null) { lock (this.driverServiceProcessLock) { if (this.driverServiceProcess is null) { var driverServiceProcess = new Process(); try { if (this.DriverServicePath != null) { - if (this.DriverServiceExecutableName is null) - { - throw new InvalidOperationException("If the driver service path is specified, the driver service executable name must be as well"); - }	Low

[RenderMichael]

Mass test failures.

I think this is failing because we are locking over async code, and doing sync-over-async.

@nvborisenko Is it OK if we make DriverService.IsInitialized into an async method? I see it is virtual. Looking through the history, it used to be called IsAvailable and before that IgnoreMissingStatusEndpoint. This used to only affect FirefoxDriverService. It does not seem like the kind of property which needs to be overridden (or even accessed) by users directly.

Since it is public, can we add an async alternative and obsolete the sync version? That will put us on the road to success for the future.

[RenderMichael]

PR has been updated, my previous initiative has been scrapped in favor of an incremental improvement: true async NewSession command and async driver service disposal. Please see new description for a full explanation.

[nvborisenko]

1. I think it is just a bug in code. Tests in my that PR passed.
2. About introducing Async: it is good idea, @titusfortner already started it partially. Your current approach of doing it is not very good. Let's discuss it offline before moving further.

[nvborisenko]

if (this.driverServiceProcess is null)
{
    var driverServiceProcess = new Process();

Should be this.driverServiceProcess = new Process();

[RenderMichael]

Should be this.driverServiceProcess = new Process();

I simplified the thread-safety so now I can do this. The PR now waits for initialization even if the process has been set.

[RenderMichael]

We should define a strategy how we want to introduce async methods. As soon as we have clear vision, then will proceed.

@nvborisenko

The general strategy was laid out in this older issue #14067

However, the new async method was just for the DriverService type, which 99% of users do not need to know about (and the <=v4 behavior of disposing the service inside the driver makes it pointless as a standalone class anyway)

As part of making the driver service reusable in preparation for v5, I wanted to make the start/stop methods async. If we introduce those changes after v5, then users may start using the driver service type and the async methods will become a migration for them.

[RenderMichael]

@nvborisenko The changes in this PR are useful, even if not exposed to users.

What if I remove the public-facing changes, and just make this an internal optimization? Would that be good to go without deliberation?

[nvborisenko]

We should have a global strategy. Your proposal is a breaking change.

[RenderMichael]

@nvborisenko I removed all breaking changes and obsoletion messages. I believe the changes in this PR are still worthwhile.

Diffs look best with whitespace off https://github.com/SeleniumHQ/selenium/pull/15246/files?w=1