Skip to content

[dotnet] allow asynchronous command execution #13952

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.

Sign up for GitHub

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

Closed
titusfortner wants to merge 10 commits into from
Closed

[dotnet] allow asynchronous command execution #13952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
36b49d8
[dotnet] allow driver to execute commands asynchronously
titusfortner May 16, 2024
3f30c8d
[dotnet] implement Navigation BackAsync with async execution
titusfortner May 16, 2024
1197b0f
[dotnet] wrap async executions inside task run
titusfortner May 16, 2024
173447c
[dotnet] set configure away to false when synchronizing tasks
titusfortner May 17, 2024
2472a8a
fix compile error
titusfortner May 17, 2024
ab31f5f
implement navigation method on event firing webdriver
titusfortner May 17, 2024
103f8d5
implement helper for running async commands synchronously
titusfortner May 17, 2024
7101abe
make async helper class public to use in support module
titusfortner May 17, 2024
49741eb
BackAsync needs to configure away false
titusfortner May 18, 2024
0c8c691
do not configure await the task
titusfortner May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion dotnet/src/support/Events/EventFiringWebDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Drawing;
using System.Threading.Tasks;
using OpenQA.Selenium.Internal;

namespace OpenQA.Selenium.Support.Events
{
Expand Down Expand Up @@ -845,12 +847,21 @@ public EventFiringNavigation(EventFiringWebDriver driver)
/// Move the browser back
/// </summary>
public void Back()
{
AsyncHelper.RunSync(this.BackAsync);
}

/// <summary>
/// Move the browser back asynchronously
/// </summary>
/// <returns>A task object representing the asynchronous operation</returns>
public async Task BackAsync()
{
try
{
WebDriverNavigationEventArgs e = new WebDriverNavigationEventArgs(this.parentDriver);
this.parentDriver.OnNavigatingBack(e);
this.wrappedNavigation.Back();
await this.wrappedNavigation.BackAsync().ConfigureAwait(false);
this.parentDriver.OnNavigatedBack(e);
}
catch (Exception ex)
Expand Down
9 changes: 9 additions & 0 deletions dotnet/src/webdriver/ICommandExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// </copyright>

using System;
using System.Threading.Tasks;

namespace OpenQA.Selenium
{
Expand All @@ -39,5 +40,13 @@ public interface ICommandExecutor : IDisposable
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A response from the browser</returns>
Response Execute(Command commandToExecute);


/// <summary>
/// Executes a command Asynchronously
Copy link
Contributor

@YevgeniyShunevych YevgeniyShunevych May 23, 2024

Choose a reason for hiding this comment

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

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>
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A task object representing the asynchronous operation</returns>
Task<Response> ExecuteAsync(Command commandToExecute);
}
}
7 changes: 7 additions & 0 deletions dotnet/src/webdriver/INavigation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// </copyright>

using System;
using System.Threading.Tasks;

namespace OpenQA.Selenium
{
Expand All @@ -31,6 +32,12 @@ public interface INavigation
/// </summary>
void Back();

/// <summary>
/// Move back a single entry in the browser's history.
/// </summary>
/// <returns>A task object representing the asynchronous operation</returns>
Task BackAsync();

/// <summary>
/// Move a single "item" forward in the browser's history.
/// </summary>
Expand Down
49 changes: 49 additions & 0 deletions dotnet/src/webdriver/Internal/AsyncHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// <copyright file="AsyncHelper.cs" company="WebDriver Committers">
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The SFC licenses this file
// to you under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Threading;
using System.Threading.Tasks;

namespace OpenQA.Selenium.Internal
{
/// <summary>
/// Encapsulates methods for working with asynchronous tasks.
/// </summary>
public static class AsyncHelper
Copy link
Contributor

@YevgeniyShunevych YevgeniyShunevych May 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@titusfortner titusfortner May 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@nvborisenko nvborisenko May 23, 2024

Choose a reason for hiding this comment

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

Duplicate, it is not a criminal.

Copy link
Member Author

@titusfortner titusfortner May 23, 2024

Choose a reason for hiding this comment

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

Agreed, in this case I'm following what appears to be done elsewhere

{
private static readonly TaskFactory _myTaskFactory = new TaskFactory(CancellationToken.None,
TaskCreationOptions.None, TaskContinuationOptions.None, TaskScheduler.Default);
Copy link
Member

@nvborisenko nvborisenko May 29, 2024

Choose a reason for hiding this comment

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

Also wandering should we deny attaching children tasks via TaskCreationOptions?


public static TResult RunSync<TResult>(Func<Task<TResult>> func)
{
return _myTaskFactory.StartNew(() =>
{
return func();
}).Unwrap().GetAwaiter().GetResult();
}

public static void RunSync(Func<Task> func)
{
_myTaskFactory.StartNew(() =>
{
return func();
}).Unwrap().GetAwaiter().GetResult();
}
}
}
13 changes: 12 additions & 1 deletion dotnet/src/webdriver/Navigator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
// </copyright>

using System;
using System.Threading.Tasks;
using OpenQA.Selenium.Internal;
using System.Collections.Generic;

namespace OpenQA.Selenium
Expand All @@ -42,7 +44,16 @@ public Navigator(WebDriver driver)
/// </summary>
public void Back()
{
this.driver.InternalExecute(DriverCommand.GoBack, null);
AsyncHelper.RunSync(this.BackAsync);
}

/// <summary>
/// Move back a single entry in the browser's history asynchronously.
/// </summary>
/// <returns>A task object representing the asynchronous operation</returns>
public async Task BackAsync()
{
await this.driver.InternalExecuteAsync(DriverCommand.GoBack, null);
}

/// <summary>
Expand Down
14 changes: 13 additions & 1 deletion dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
// </copyright>

using System;
using System.Threading.Tasks;
using OpenQA.Selenium.Internal;

namespace OpenQA.Selenium.Remote
{
Expand Down Expand Up @@ -92,6 +94,16 @@ public HttpCommandExecutor HttpExecutor
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A response from the browser</returns>
public Response Execute(Command commandToExecute)
{
return AsyncHelper.RunSync(() => this.ExecuteAsync(commandToExecute));
}

/// <summary>
/// Executes a command Asynchronously
/// </summary>
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A task object representing the asynchronous operation</returns>
public async Task<Response> ExecuteAsync(Command commandToExecute)
{
if (commandToExecute == null)
{
Expand All @@ -108,7 +120,7 @@ public Response Execute(Command commandToExecute)
// command, so that we can get the finally block.
try
{
toReturn = this.internalExecutor.Execute(commandToExecute);
toReturn = await this.internalExecutor.ExecuteAsync(commandToExecute);
}
finally
{
Expand Down
12 changes: 11 additions & 1 deletion dotnet/src/webdriver/Remote/HttpCommandExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,16 @@ public bool TryAddCommand(string commandName, CommandInfo info)
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A response from the browser</returns>
public virtual Response Execute(Command commandToExecute)
{
return AsyncHelper.RunSync(() => this.ExecuteAsync(commandToExecute));
}

/// <summary>
/// Executes a command Asynchronously
Copy link
Contributor

@YevgeniyShunevych YevgeniyShunevych May 23, 2024

Choose a reason for hiding this comment

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

Suggested change
/// Executes a command Asynchronously
/// Executes a command asynchronously.

/// </summary>
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A task object representing the asynchronous operation</returns>
public virtual async Task<Response> ExecuteAsync(Command commandToExecute)
{
if (commandToExecute == null)
{
Expand All @@ -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);
Copy link
Contributor

@YevgeniyShunevych YevgeniyShunevych May 23, 2024

Choose a reason for hiding this comment

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

I think it also makes sense to rename existing method MakeHttpRequest to MakeHttpRequestAsync.

}
catch (HttpRequestException ex)
{
Expand Down
25 changes: 22 additions & 3 deletions dotnet/src/webdriver/WebDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Globalization;
using System.Threading.Tasks;

namespace OpenQA.Selenium
{
Expand Down Expand Up @@ -556,7 +557,25 @@ internal ReadOnlyCollection<IWebElement> GetElementsFromResponse(Response respon
/// <returns>WebDriver Response</returns>
internal Response InternalExecute(string driverCommandToExecute, Dictionary<string, object> parameters)
{
return this.Execute(driverCommandToExecute, parameters);
return AsyncHelper.RunSync(() => this.InternalExecuteAsync(driverCommandToExecute, parameters));
}

/// <summary>
/// Executes commands with the driver asynchronously
/// </summary>
/// <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,
Copy link
Contributor

@YevgeniyShunevych YevgeniyShunevych May 23, 2024

Choose a reason for hiding this comment

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

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 internal member is accessible from the current assembly or from types that are derived from the containing class.

Copy link
Member Author

@titusfortner titusfortner May 23, 2024

Choose a reason for hiding this comment

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

I'm not sure why it's there. If we change it, it should be part of a different PR, though.

Dictionary<string, object> parameters)
{
return this.ExecuteAsync(driverCommandToExecute, parameters);
}

internal Response Execute(string driverCommandToExecute,
Dictionary<string, object> parameters)
{
return AsyncHelper.RunSync(() => this.ExecuteAsync(driverCommandToExecute, parameters));
}

/// <summary>
Expand All @@ -565,15 +584,15 @@ internal Response InternalExecute(string driverCommandToExecute, Dictionary<stri
/// <param name="driverCommandToExecute">A <see cref="DriverCommand"/> value representing the command to execute.</param>
/// <param name="parameters">A <see cref="Dictionary{K, V}"/> containing the names and values of the parameters of the command.</param>
/// <returns>A <see cref="Response"/> containing information about the success or failure of the command and any data returned by the command.</returns>
protected virtual Response Execute(string driverCommandToExecute, Dictionary<string, object> parameters)
protected virtual async Task<Response> ExecuteAsync(string driverCommandToExecute, Dictionary<string, object> parameters)
{
Command commandToExecute = new Command(this.sessionId, driverCommandToExecute, parameters);

Response commandResponse;

try
{
commandResponse = this.executor.Execute(commandToExecute);
commandResponse = await this.executor.ExecuteAsync(commandToExecute);
}
catch (System.Net.Http.HttpRequestException e)
{
Expand Down