[dotnet] allow asynchronous command execution #13952
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PR Description updated to latest commit (eca81ec)
|
PR Review 🔍(Review updated until commit c405e23)
Code feedback:
|
PR Code Suggestions ✨No code suggestions found for PR. |
CI Failure Feedback 🧐(Checks updated until commit 0c8c691)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
nvborisenko
requested changes
May 16, 2024
nvborisenko
reviewed
May 16, 2024
|
/help |
PR Agent Walkthrough 🤖Welcome to the PR Agent, an AI-powered tool for automated pull request analysis, feedback, suggestions and more. Here is a list of tools you can use to interact with the PR Agent:
(1) Note that each tool be triggered automatically when a new PR is opened, or called manually by commenting on a PR. (2) Tools marked with [*] require additional parameters to be passed. For example, to invoke the |
codiumai-pr-agent-pro
bot
added
Review effort [1-5]: 4
and removed
Review effort [1-5]: 3
labels
|
Persistent review updated to latest commit c405e23 |
nvborisenko
reviewed
May 17, 2024
nvborisenko
reviewed
May 17, 2024
nvborisenko
reviewed
May 17, 2024
titusfortner
force-pushed
the
dotnet_async
branch
2 times, most recently
from
59d6a17
to
01b6f29
Compare
titusfortner
force-pushed
the
dotnet_async
branch
from
01b6f29
to
49741eb
Compare
|
@nvborisenko & @YevgeniyShunevych is this good enough to merge for now? |
| /// <param name="driverCommandToExecute">Command that needs executing</param> | ||
| /// <param name="parameters">Parameters needed for the command</param> | ||
| /// <returns>A task object representing the asynchronous operation</returns> | ||
| internal Task<Response> InternalExecuteAsync(string driverCommandToExecute, |
There was a problem hiding this comment.
If I got the idea of InternalExecuteAsync right, it is just a
copy of ExecuteAsync but with internal access modifier. If so, let's maybe change the access modifier of ExecuteAsync from protected to protected internal, and then InternalExecuteAsync can be removed.
A
protected internalmember is accessible from the current assembly or from types that are derived from the containing class.
There was a problem hiding this comment.
I'm not sure why it's there. If we change it, it should be part of a different PR, though.
|
|
||
|
|
||
| /// <summary> | ||
| /// Executes a command Asynchronously |
There was a problem hiding this comment.
Suggested change
| /// Executes a command Asynchronously | |
| /// Executes a command asynchronously. |
The capital letter fix. Additionally, I would recommend to end all sentences in XML docs (<summary>, <param>, etc.) with a period.
| } | ||
|
|
||
| /// <summary> | ||
| /// Executes a command Asynchronously |
There was a problem hiding this comment.
Suggested change
| /// Executes a command Asynchronously | |
| /// Executes a command asynchronously. |
| @@ -184,7 +194,7 @@ public virtual Response Execute(Command commandToExecute) | |||
| HttpResponseInfo responseInfo = null; | |||
| try | |||
| { | |||
| responseInfo = Task.Run(async () => await this.MakeHttpRequest(requestInfo)).GetAwaiter().GetResult(); | |||
| responseInfo = await this.MakeHttpRequest(requestInfo); | |||
There was a problem hiding this comment.
I think it also makes sense to rename existing method MakeHttpRequest to MakeHttpRequestAsync.
| /// <summary> | ||
| /// Encapsulates methods for working with asynchronous tasks. | ||
| /// </summary> | ||
| public static class AsyncHelper |
There was a problem hiding this comment.
The AsyncHelper implementation is similar to https://github.com/aspnet/AspNetIdentity/blob/main/src/Microsoft.AspNet.Identity.Core/AsyncHelper.cs. If you check the differences, this implementation lacks cultures set for async functions. Maybe worth adding as well, or cultures skipped intentionally for some reason? Also I would recommend making AsyncHelper class internal.
There was a problem hiding this comment.
We either need to duplicate the helper class in both webdriver and support packages or make it public in webdriver to access it from support. This approach appears to be what other classes in the repo do.
There was a problem hiding this comment.
Duplicate, it is not a criminal.
There was a problem hiding this comment.
Agreed, in this case I'm following what appears to be done elsewhere
nvborisenko
reviewed
May 29, 2024
| public static class AsyncHelper | ||
| { | ||
| private static readonly TaskFactory _myTaskFactory = new TaskFactory(CancellationToken.None, | ||
| TaskCreationOptions.None, TaskContinuationOptions.None, TaskScheduler.Default); |
There was a problem hiding this comment.
Also wandering should we deny attaching children tasks via TaskCreationOptions?
|
Ok, I've redone this code a bit, and it's going to be easier to just create a new PR, so closing this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
Description
This surfaces the ability to allow the driver to execute commands asynchronously by returning a task object representing the async operation in a way that can be awaited in user code.
Motivation and Context
This is in preparation for implementing an asynchronous solution in .NET.
This PR is to demonstrate the template for the naming conventions we want to use.
The next step would be to change the async method implementation to be able to use WebDriverBiDi if it is enabled, with the current sync operation as the fallback
Types of changes
PR Type
Enhancement
Description
ExecuteAsyncinICommandExecutor,DriverServiceCommandExecutor,HttpCommandExecutor, andWebDriverto support async operations.BackAsyncinINavigationandNavigatorfor asynchronous navigation.Changes walkthrough 📝
ICommandExecutor.cs
Add asynchronous command execution interfacedotnet/src/webdriver/ICommandExecutor.cs
ExecuteAsyncmethod to allow asynchronous command execution.INavigation.cs
Introduce asynchronous navigation methodsdotnet/src/webdriver/INavigation.cs
BackAsyncmethod to navigate back asynchronously.Navigator.cs
Implement async back navigation in Navigatordotnet/src/webdriver/Navigator.cs
BackAsyncto perform asynchronous back navigation.Backto useBackAsync.DriverServiceCommandExecutor.cs
Support async command execution in DriverServiceCommandExecutordotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs
ExecuteAsyncto handle command execution asynchronously.Executeto useExecuteAsync.HttpCommandExecutor.cs
Enhance HttpCommandExecutor with async execution supportdotnet/src/webdriver/Remote/HttpCommandExecutor.cs
ExecuteAsyncfor asynchronous HTTP command execution.Executeto useExecuteAsync.WebDriver.cs
Extend WebDriver with asynchronous execution capabilitiesdotnet/src/webdriver/WebDriver.cs
InternalExecuteAsyncandExecuteAsyncfor async commandexecution.
Executemethods to use their async counterparts.