-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[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
base: trunk
Are you sure you want to change the base?
[dotnet] Propagate service asynchronicity to the command executor. #15246
Conversation
PR Reviewer Guide 🔍(Review updated until commit 3d085be)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 3d085be
Previous suggestionsSuggestions up to commit 8fc36d2
|
Mass test failures. I think this is failing because we are @nvborisenko Is it OK if we make Since it is public, can we add an |
DriverService.Start()
thread-safe
PR has been updated, my previous initiative has been scrapped in favor of an incremental improvement: true async |
|
if (this.driverServiceProcess is null)
{
var driverServiceProcess = new Process(); Should be |
I simplified the thread-safety so now I can do this. The PR now waits for initialization even if the process has been set. |
The general strategy was laid out in this older issue #14067 However, the new async method was just for the 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. |
@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? |
There was a problem hiding this 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.
@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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Reopening, still valid. We just need verify whether it is breaking. |
PR Code Suggestions ✨Explore these optional code suggestions:
|
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 DriverStart
asynchronicity up to that method.Motivation and Context
The
NewSession
command does sync-over-async, which is bad practice. Now that we haveasync ExecuteAsync
, that can be minimized.Types of changes
Checklist
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
File Walkthrough
DriverService.cs
Convert DriverService to async pattern
dotnet/src/webdriver/DriverService.cs
DriverServiceCommandExecutor.cs
Use async StartAsync method
dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs
service.Start()