Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Feb 7, 2025

User description

Motivation and Context

Now we are dependent on System.Threading.Channels package for netstandard2.0 target only. net8 is still free of dependencies.

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


Description

  • Replaced BlockingCollection with Channel for event handling.

  • Updated Broker class to use System.Threading.Channels.

  • Added System.Threading.Channels as a dependency in multiple files.

  • Improved task management and event processing loops.


Changes walkthrough 📝

Relevant files
Enhancement
Broker.cs
Refactored `Broker` to use `Channel` for events                   

dotnet/src/webdriver/BiDi/Communication/Broker.cs

  • Replaced BlockingCollection with Channel for event handling.
  • Updated methods to use Channel for writing and reading events.
  • Removed TaskFactory and improved task management.
  • Renamed methods for better clarity and consistency.
  • +14/-13 
    Dependencies
    BUILD.bazel
    Added `System.Threading.Channels` to Bazel dependencies   

    dotnet/src/webdriver/BUILD.bazel

    • Added System.Threading.Channels as a dependency for Bazel build.
    +2/-0     
    WebDriver.StrongNamed.nuspec
    Added `System.Threading.Channels` to StrongNamed nuspec   

    dotnet/src/webdriver/WebDriver.StrongNamed.nuspec

    • Added System.Threading.Channels dependency for netstandard2.0.
    +1/-0     
    WebDriver.nuspec
    Added `System.Threading.Channels` to WebDriver nuspec       

    dotnet/src/webdriver/WebDriver.nuspec

    • Added System.Threading.Channels dependency for netstandard2.0.
    +1/-0     
    WebDriver.csproj
    Added `System.Threading.Channels` to csproj dependencies 

    dotnet/src/webdriver/WebDriver.csproj

    • Included System.Threading.Channels package for netstandard2.0.
    +1/-0     
    paket.dependencies
    Added `System.Threading.Channels` to Paket dependencies   

    dotnet/paket.dependencies

  • Added System.Threading.Channels version 8.0.0 to Paket dependencies.
  • +1/-0     
    Additional files
    paket.nuget.bzl +10/-9   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @nvborisenko nvborisenko marked this pull request as ready for review February 7, 2025 15:53
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The new event processing loop using channels should include proper error handling and graceful shutdown mechanisms when the channel is completed or errors occur

    private async Task ProcessEventsLoopAsync()
    {
        while (await _pendingEvents.Reader.WaitToReadAsync().ConfigureAwait(false))
        {
            try
            {
                var result = await _pendingEvents.Reader.ReadAsync().ConfigureAwait(false);
    
                if (_eventHandlers.TryGetValue(result.Method, out var eventHandlers))
                {
                    if (eventHandlers is not null)
    Task Management

    The removal of TaskCreationOptions.LongRunning might impact performance for long-running operations. Verify that the new implementation handles long-running tasks efficiently

    _receivingMessageTask = Task.Run(async () => await ReceiveMessagesLoopAsync(_receiveMessagesCancellationTokenSource.Token));
    _eventEmitterTask = Task.Run(ProcessEventsLoopAsync);

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add channel write error handling

    Add error handling for when writing to the channel fails. The WriteAsync
    operation could throw if the channel is completed or disposed.

    dotnet/src/webdriver/BiDi/Communication/Broker.cs [130]

    -await _pendingEvents.Writer.WriteAsync(messageEvent).ConfigureAwait(false);
    +try {
    +    await _pendingEvents.Writer.WriteAsync(messageEvent).ConfigureAwait(false);
    +} catch (ChannelClosedException) {
    +    // Handle or log channel closed error
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Adding error handling for channel write operations is critical for preventing unhandled exceptions that could crash the application when the channel is completed or disposed.

    Medium
    General
    Add cancellation support for channels

    Add cancellation token to channel operations to allow proper cancellation and
    cleanup during shutdown.

    dotnet/src/webdriver/BiDi/Communication/Broker.cs [142-146]

    -while (await _pendingEvents.Reader.WaitToReadAsync().ConfigureAwait(false))
    +while (await _pendingEvents.Reader.WaitToReadAsync(_receiveMessagesCancellationTokenSource.Token).ConfigureAwait(false))
     {
         try
         {
    -        var result = await _pendingEvents.Reader.ReadAsync().ConfigureAwait(false);
    +        var result = await _pendingEvents.Reader.ReadAsync(_receiveMessagesCancellationTokenSource.Token).ConfigureAwait(false);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Adding cancellation token support to channel operations is important for proper cleanup and resource management during shutdown, preventing potential resource leaks.

    Medium
    Learned
    best practice
    Add null validation checks for method parameters to prevent potential NullReferenceExceptions

    The ExecuteCommandAsync<TCommand, TResult> and ExecuteCommandAsync methods
    accept a nullable options parameter but don't validate it before use. While it
    may be optional, explicitly validating nullable parameters helps prevent
    potential NullReferenceExceptions and makes the code more robust.

    dotnet/src/webdriver/BiDi/Communication/Broker.cs [182-185]

     public async Task<TResult> ExecuteCommandAsync<TCommand, TResult>(TCommand command, CommandOptions? options)
         where TCommand : Command
     {
    +    ArgumentNullException.ThrowIfNull(command, nameof(command));
         var jsonElement = await ExecuteCommandCoreAsync(command, options).ConfigureAwait(false);
    • Apply this suggestion
    Low

    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

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

    Nice changes! How does this affect performance/memory?

    @nvborisenko
    Copy link
    Member Author

    Nice changes! How does this affect performance/memory?

    Didn't test it accurately without remote-end party. But I don't expect performance improvement here.

    @nvborisenko
    Copy link
    Member Author

    Hm, now I am struggled: do we really want to introduce new dependency instead of BlockingCollection<> which just works? Given that a good library is a library without dependencies, then we should provide strong arguments why we need Channels.

    @nvborisenko nvborisenko added the A-needs decision TLC needs to discuss and agree label Feb 7, 2025
    @nvborisenko nvborisenko marked this pull request as draft February 7, 2025 19:24
    @RenderMichael
    Copy link
    Contributor

    RenderMichael commented Feb 7, 2025

    BlockingCollection<> is bounded to a maximum size, and is blocking (probably why we needed to mess with TaskFactory in the existing code).

    Channel<T> is also much more low-level and is faster (performance improvements shown in the recent Nick Chapsas video and the older Toub blog post).

    Additionally, this only adds a new dependency on older TFMs. I don't know the stats about how many people use the .NET 8 version vs. the .NET Standard version, but the kinds of blockers which prevent upgrades to modern .NET are not usually present in code that uses Selenium. For this reason, a dependency that only exists on older .NET versions seems less critical.

    @nvborisenko
    Copy link
    Member Author

    Thanks @nvborisenko for contribution. I decided (8 Feb) to not introduce new dependency to fix mimic issue/improvement. In case of any issues, please came in here.

    @nvborisenko nvborisenko closed this Feb 7, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    A-needs decision TLC needs to discuss and agree Review effort [1-5]: 2

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants