Skip to content

[dotnet][bidi] Remove json serialization from transport layer - #15250

Merged
nvborisenko merged 1 commit into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-bidi-json-transport
Feb 7, 2025
Merged

[dotnet][bidi] Remove json serialization from transport layer#15250
nvborisenko merged 1 commit into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-bidi-json-transport

Conversation

@nvborisenko

@nvborisenko nvborisenko commented Feb 7, 2025 •
edited by qodo-code-review Bot
Loading

Copy link
Copy Markdown
Member

User description

Motivation and Context

Simplify transport layer to know nothing about json serialization context
.

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

  • Removed JSON serialization from the transport layer.

  • Introduced raw byte data handling for sending/receiving.

  • Simplified ITransport interface by removing JSON-specific methods.

  • Updated Broker and WebSocketTransport to use new transport methods.

Changes walkthrough 📝

Relevant files
Enhancement
Broker.cs
Refactored Broker to remove JSON serialization dependency

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

  • Replaced JSON-specific transport methods with raw byte handling.
  • Updated message deserialization to use JsonSerializer.Deserialize.
  • Adjusted command execution to serialize/deserialize manually.
  • Fixed formatting in generic constraints.
    		• +11/-7   
    ITransport.cs
    Simplified ITransport interface for raw byte handling       

    dotnet/src/webdriver/BiDi/Communication/Transport/ITransport.cs

  • Removed JSON-specific methods from ITransport interface.
  • Added methods for raw byte data handling.
    		• +2/-4     
    WebSocketTransport.cs
    Updated WebSocketTransport to handle raw byte data             

    dotnet/src/webdriver/BiDi/Communication/Transport/WebSocketTransport.cs

  • Replaced JSON serialization with raw byte handling.
  • Updated logging to reflect raw byte data.
  • Adjusted WebSocket send/receive methods for new interface.
    		• +8/-13   
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review

    qodo-code-review Bot commented Feb 7, 2025

    Copy link
    Copy Markdown
    Contributor

    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 deserialization of messages using JsonSerializer.Deserialize lacks error handling for malformed JSON data. Consider adding try-catch block to handle potential JsonException.

    var message = JsonSerializer.Deserialize(new ReadOnlySpan<byte>(data), _jsonSerializerContext.Message);
    Resource Leak

    The MemoryStream is disposed but the received data is stored in a byte array that persists. Consider using ArrayPool for better memory management of large messages.

    byte[] data = ms.ToArray();

    @qodo-code-review

    qodo-code-review Bot commented Feb 7, 2025

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Restore long-running task option

    Add TaskCreationOptions.LongRunning back to the task factory calls since these
    are long-running background operations that shouldn't use the thread pool.

    dotnet/src/webdriver/BiDi/Communication/Broker.cs [114-115]

    -_receivingMessageTask = _myTaskFactory.StartNew(async () => await ReceiveMessagesAsync(_receiveMessagesCancellationTokenSource.Token)).Unwrap();
-_eventEmitterTask = _myTaskFactory.StartNew(ProcessEventsAwaiterAsync).Unwrap();
+_receivingMessageTask = _myTaskFactory.StartNew(async () => await ReceiveMessagesAsync(_receiveMessagesCancellationTokenSource.Token), TaskCreationOptions.LongRunning).Unwrap();
+_eventEmitterTask = _myTaskFactory.StartNew(ProcessEventsAwaiterAsync, TaskCreationOptions.LongRunning).Unwrap();
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The PR removed TaskCreationOptions.LongRunning which is important for these background tasks as they are continuous operations. Without this option, the tasks will use thread pool threads which could impact performance and scalability.

    		Medium
    Add null check after deserialization

    Add null check for the deserialized message before the switch statement to
    prevent potential NullReferenceException.

    dotnet/src/webdriver/BiDi/Communication/Broker.cs [124-126]

     var message = JsonSerializer.Deserialize(new ReadOnlySpan<byte>(data), _jsonSerializerContext.Message);
+if (message == null)
+{
+    throw new InvalidOperationException("Failed to deserialize message");
+}
 
 switch (message)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Adding a null check with proper error handling is important to prevent potential NullReferenceException and provide a clear error message when deserialization fails.

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

    The ExecuteCommandAsync and ExecuteCommandCoreAsync methods accept a nullable
    command parameter but don't validate it for null. Add null validation at the
    start of these methods to prevent potential NullReferenceException when
    accessing command properties.

    dotnet/src/webdriver/BiDi/Communication/Broker.cs [183-186]

     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

    RenderMichael
    RenderMichael approved these changes Feb 7, 2025
    View reviewed changes

    @RenderMichael RenderMichael left a comment

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    I like this simplification!

    @nvborisenko
    nvborisenko merged commit 57f541a into SeleniumHQ:trunk Feb 7, 2025
    @nvborisenko
    nvborisenko deleted the dotnet-bidi-json-transport branch February 8, 2025 16:01
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @nvborisenko @sandeepsuryaprasad
    [dotnet][bidi] Remove json serialization from transport layer (Seleni…
    ad1ca46 
    …umHQ#15250)
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    @nvborisenko Grzegorz Ryznar
    [dotnet][bidi] Remove json serialization from transport layer (Seleni…
    eac33b1 
    …umHQ#15250)
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @RenderMichael RenderMichael RenderMichael approved these changes

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @nvborisenko @RenderMichael