Skip to content

[dotnet] Propagate service asynchronicity to the command executor. #15246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 23 commits into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 6, 2025

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


Description

  • Convert synchronous driver service operations to async/await pattern

  • Add async versions of Start, Stop, and IsInitialized methods

  • Implement IAsyncDisposable for proper async resource cleanup

  • Replace sync-over-async patterns with proper async execution


Diagram Walkthrough

flowchart LR
  A["Sync DriverService"] --> B["Async DriverService"]
  B --> C["StartAsync()"]
  B --> D["IsInitializedAsync()"]
  B --> E["StopAsync()"]
  B --> F["IAsyncDisposable"]
  G["DriverServiceCommandExecutor"] --> H["Uses StartAsync()"]
Loading

File Walkthrough

Relevant files
Enhancement
DriverService.cs
Convert DriverService to async pattern                                     

dotnet/src/webdriver/DriverService.cs

  • Add async versions of Start, IsInitialized, and Stop methods
  • Implement IAsyncDisposable interface for proper async cleanup
  • Replace Task.Run().GetAwaiter().GetResult() with proper async/await
  • Add better exception handling in StartAsync method
+94/-50 
DriverServiceCommandExecutor.cs
Use async StartAsync method                                                           

dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs

  • Update ExecuteAsync to use service.StartAsync() instead of
    service.Start()
  • Remove sync-over-async pattern in NewSession command execution
+1/-1     

Copy link
Contributor

qodo-merge-pro bot commented Feb 6, 2025

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);
}

Copy link
Contributor

qodo-merge-pro bot commented Feb 6, 2025

PR Code Suggestions ✨

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

CategorySuggestion                                                                                                                                    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
CategorySuggestion                                                                                                                                    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
Copy link
Contributor Author

RenderMichael commented Feb 6, 2025

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 RenderMichael marked this pull request as draft February 6, 2025 21:24
@RenderMichael RenderMichael changed the title [dotnet] Make DriverService.Start() thread-safe [dotnet] Propagate service asynchronicity to the command executor. Feb 6, 2025
@RenderMichael
Copy link
Contributor Author

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
Copy link
Member

  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
Copy link
Member

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

Should be this.driverServiceProcess = new Process();

@RenderMichael
Copy link
Contributor Author

RenderMichael commented Feb 7, 2025

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 RenderMichael marked this pull request as ready for review February 7, 2025 17:12
@RenderMichael
Copy link
Contributor Author

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
Copy link
Contributor Author

@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?

Copy link
Member

@nvborisenko nvborisenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RenderMichael
Copy link
Contributor Author

@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

@github-actions github-actions bot added the J-stale Applied to issues that become stale, and eventually closed. label Aug 8, 2025
@github-actions github-actions bot closed this Aug 22, 2025
@nvborisenko nvborisenko reopened this Aug 22, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Sync-over-async

New async APIs are invoked via Task.Run(...).GetAwaiter().GetResult() in synchronous wrappers (e.g., IsInitialized getter, Start, Dispose), which can deadlock in certain synchronization contexts and defeats the goal of eliminating sync-over-async. Consider preserving synchronous implementations or providing purely async usage paths without blocking.

        return Task.Run(this.IsInitializedAsync).GetAwaiter().GetResult();
    }
}

/// <summary>
/// Gets a value indicating whether the service is responding to HTTP requests.
/// </summary>
/// <returns>A task that represents the asynchronous initialization check operation.</returns>
protected async virtual Task<bool> IsInitializedAsync()
{
    bool isInitialized = false;
    try
    {
        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);
            }
        }
    }
    catch (Exception ex) when (ex is HttpRequestException || ex is TaskCanceledException)
    {
        // Do nothing. The exception is expected, meaning driver service is not initialized.
    }

    return isInitialized;
}

/// <summary>
/// Releases all resources associated with this <see cref="DriverService"/>.
/// </summary>
public void Dispose()
{
    this.Dispose(true);
    GC.SuppressFinalize(this);
}

/// <summary>
/// Starts the DriverService if it is not already running.
/// </summary>
public void Start()
{
    Task.Run(this.StartAsync).GetAwaiter().GetResult();
}
Event timing

OnDriverProcessStarted is raised after awaiting service initialization; previously it fired immediately after process start. Consumers may rely on the event to trigger actions before the service is fully initialized. Validate intended behavior and update event naming/docs or fire an additional event for "process started" vs "service ready".

bool serviceAvailable = await this.WaitForServiceInitializationAsync().ConfigureAwait(false);
DriverProcessStartedEventArgs processStartedEventArgs = new DriverProcessStartedEventArgs(this.driverServiceProcess);
this.OnDriverProcessStarted(processStartedEventArgs);

if (!serviceAvailable)
{
Visibility/contract change

Stop() changed from private void to private async Task StopAsync(), and Dispose(bool) now blocks on Task.Run(StopAsync). This alters disposal semantics and can lead to long synchronous disposal. Consider exposing a guarded synchronous Stop for IDisposable path and keep async for IAsyncDisposable to avoid blocking.

protected virtual void Dispose(bool disposing)
{
    if (!this.isDisposed)
    {
        if (disposing)
        {
            Task.Run(this.StopAsync).GetAwaiter().GetResult();
        }

        this.isDisposed = true;
    }
}

/// <summary>
/// Releases all resources associated with this <see cref="DriverService"/>.
/// </summary>
/// <returns>A task that represents the asynchronous dispose operation.</returns>
public async ValueTask DisposeAsync()
{
    await DisposeAsyncCore();
    Dispose(false);
    GC.SuppressFinalize(this);
}

/// <summary>
/// Releases all resources associated with this type in the instance's type chain. Override to dispose more resources.
/// </summary>
/// <returns>A task that represents the asynchronous dispose operation.</returns>
protected virtual async ValueTask DisposeAsyncCore()
{
    await this.StopAsync().ConfigureAwait(false);
}

/// <summary>
/// Raises the <see cref="DriverProcessStarting"/> event.
/// </summary>
/// <param name="eventArgs">A <see cref="DriverProcessStartingEventArgs"/> that contains the event data.</param>
protected void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs)
{
    if (eventArgs == null)
    {
        throw new ArgumentNullException(nameof(eventArgs), "eventArgs must not be null");
    }

    this.DriverProcessStarting?.Invoke(this, eventArgs);
}

/// <summary>
/// Raises the <see cref="DriverProcessStarted"/> event.
/// </summary>
/// <param name="eventArgs">A <see cref="DriverProcessStartedEventArgs"/> that contains the event data.</param>
protected void OnDriverProcessStarted(DriverProcessStartedEventArgs eventArgs)
{
    if (eventArgs == null)
    {
        throw new ArgumentNullException(nameof(eventArgs), "eventArgs must not be null");
    }

    this.DriverProcessStarted?.Invoke(this, eventArgs);
}

/// <summary>
/// Stops the DriverService.
/// </summary>
private async Task StopAsync()
{

@nvborisenko
Copy link
Member

Reopening, still valid. We just need verify whether it is breaking.

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent orphaned process and misfired event

If initialization fails, the process remains running and event
'DriverProcessStarted' still fires. Ensure events fire after successful
initialization and stop/cleanup the process on failure to avoid orphaned
processes. Also null-check process before event usage to guard against races.

dotnet/src/webdriver/DriverService.cs [237-284]

 public async Task StartAsync()
 {
     if (this.driverServiceProcess == null)
     {
         this.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");
                 }
 
                 this.driverServiceProcess.StartInfo.FileName = Path.Combine(this.DriverServicePath, this.DriverServiceExecutableName);
             }
             else
             {
                 this.driverServiceProcess.StartInfo.FileName = new DriverFinder(this.GetDefaultDriverOptions()).GetDriverPath();
             }
 
             this.driverServiceProcess.StartInfo.Arguments = this.CommandLineArguments;
             this.driverServiceProcess.StartInfo.UseShellExecute = false;
             this.driverServiceProcess.StartInfo.CreateNoWindow = this.HideCommandPromptWindow;
 
-            DriverProcessStartingEventArgs eventArgs = new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo);
+            var eventArgs = new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo);
             this.OnDriverProcessStarting(eventArgs);
 
             this.driverServiceProcess.Start();
         }
         catch
         {
             this.driverServiceProcess.Dispose();
             this.driverServiceProcess = null;
             throw;
         }
     }
 
     bool serviceAvailable = await this.WaitForServiceInitializationAsync().ConfigureAwait(false);
-    DriverProcessStartedEventArgs processStartedEventArgs = new DriverProcessStartedEventArgs(this.driverServiceProcess);
-    this.OnDriverProcessStarted(processStartedEventArgs);
 
     if (!serviceAvailable)
     {
+        // Attempt graceful stop, then force kill to avoid orphan.
+        try { await this.StopAsync().ConfigureAwait(false); } catch { /* swallow */ }
         throw new WebDriverException($"Cannot start the driver service on {this.ServiceUrl}");
+    }
+
+    if (this.driverServiceProcess != null)
+    {
+        var processStartedEventArgs = new DriverProcessStartedEventArgs(this.driverServiceProcess);
+        this.OnDriverProcessStarted(processStartedEventArgs);
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical bug where a failed service initialization would leave an orphaned process running, and it provides a robust fix to ensure proper cleanup.

High
Avoid deadlock-prone sync-over-async

Calling async code via Task.Run().GetAwaiter().GetResult() risks deadlocks in
synchronization-context environments and masks exceptions. Provide a truly
synchronous path without Task.Run, or document and use .ConfigureAwait(false)
within StartAsync and block via GetAwaiter().GetResult() directly to avoid
context capture issues.

dotnet/src/webdriver/DriverService.cs [228-231]

 public void Start()
 {
-    Task.Run(this.StartAsync).GetAwaiter().GetResult();
+    this.StartAsync().ConfigureAwait(false).GetAwaiter().GetResult();
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that using Task.Run is unnecessary overhead and proposes a more direct and efficient, yet still safe, sync-over-async implementation.

Low
Learned
best practice
Remove sync-over-async wrapper

Remove the synchronous wrapper calling an async method via
Task.Run().GetAwaiter().GetResult(), and keep the API purely async. Expose only
the async IsInitializedAsync() and migrate callers to use it. This prevents
potential deadlocks and improves scalability.

dotnet/src/webdriver/DriverService.cs [175-181]

-protected virtual bool IsInitialized
+// Remove the synchronous IsInitialized property entirely.
+// Consumers should use the async method:
+protected async virtual Task<bool> IsInitializedAsync()
 {
-    get
+    bool isInitialized = false;
+    try
     {
-        return Task.Run(this.IsInitializedAsync).GetAwaiter().GetResult();
+        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).ConfigureAwait(false))
+            {
+                isInitialized = response.StatusCode == HttpStatusCode.OK
+                    && response.Content.Headers.ContentType is { MediaType: string mediaType }
+                    && mediaType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase);
+            }
+        }
     }
+    catch (Exception ex) when (ex is HttpRequestException || ex is TaskCanceledException)
+    {
+    }
+    return isInitialized;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Avoid sync-over-async patterns; prefer async all the way to prevent deadlocks and thread-pool starvation.

Low
  • More

@github-actions github-actions bot removed the J-stale Applied to issues that become stale, and eventually closed. label Aug 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants