10000 [dotnet] [bidi] Initialize internal modules without Lazy by nvborisenko · Pull Request #15979 · SeleniumHQ/selenium · GitHub
[go: up one dir, main page]
Skip to content

[dotnet] [bidi] Initialize internal modules without Lazy #15979

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

Jump to bottom
Merged
merged 2 commits into from
Jun 29, 2025
Merged

[dotnet] [bidi] Initialize internal modules without Lazy #15979

merged 2 commits into from
Jun 29, 2025

Conversation

nvborisenko
Copy link
Member
@nvborisenko nvborisenko commented Jun 29, 2025 •
edited
Loading

User description

💥 What does this PR do?

Don't use Lazy<T> because it reserves a reference to all modules, which is not AOT friendly.

💡 Additional Considerations

Probably we want to make an initialization in Property (with locking to be thread-safe)?

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Other

Description

  • Remove Lazy<T> initialization from BiDi modules for AOT compatibility

  • Initialize modules directly in constructor instead of lazy loading

  • Update property accessors to return modules directly

Changes diagram

flowchart LR
  A["Lazy<Module> fields"] --> B["Direct Module fields"]
  C["Lazy initialization"] --> D["Direct initialization"]
  E["Property.Value access"] --> F["Direct property access"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
BiDi.cs
Remove lazy initialization from BiDi modules                         

dotnet/src/webdriver/BiDi/BiDi.cs

  • Replace Lazy fields with direct module fields
  • Initialize modules directly in constructor
  • Update property accessors to return modules without .Value
    		• +24/-24 
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jun 29, 2025
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    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

    Thread Safety

    The removal of Lazy initialization may introduce thread safety concerns if the BiDi class is accessed from multiple threads concurrently. The previous Lazy implementation provided thread-safe initialization, but direct field initialization in constructor doesn't guarantee thread-safe access to the modules themselves.

     
    private readonly Session.SessionModule _sessionModule;
private readonly BrowsingContext.BrowsingContextModule _browsingContextModule;
private readonly Browser.BrowserModule _browserModule;
private readonly Network.NetworkModule _networkModule;
private readonly Input.InputModule _inputModule;
private readonly Script.ScriptModule _scriptModule;
private readonly Log.LogModule _logModule;
private readonly Storage.StorageModule _storageModule;
    Performance Impact

    All modules are now initialized eagerly in the constructor regardless of whether they will be used. This could impact startup performance and memory usage, especially if some modules are rarely used or have expensive initialization.

     
    _sessionModule = new Session.SessionModule(_broker);
_browsingContextModule = new BrowsingContext.BrowsingContextModule(_broker);
_browserModule = new Browser.BrowserModule(_broker);
_networkModule = new Network.NetworkModule(_broker);
_inputModule = new Input.InputModule(_broker);
_scriptModule = new Script.ScriptModule(_broker);
_logModule = new Log.LogModule(_broker);
_storageModule = new Storage.StorageModule(_broker);

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor
    qodo-merge-pro bot commented Jun 29, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add exception handling for initialization

    Consider adding null checks or exception handling around module initialization.
    If any module constructor throws an exception, the BiDi object will be left in
    an inconsistent state with some modules initialized and others not.

    dotnet/src/webdriver/BiDi/BiDi.cs [45-52]

    -_sessionModule = new Session.SessionModule(_broker);
-_browsingContextModule = new BrowsingContext.BrowsingContextModule(_broker);
-_browserModule = new Browser.BrowserModule(_broker);
-_networkModule = new Network.NetworkModule(_broker);
-_inputModule = new Input.InputModule(_broker);
-_scriptModule = new Script.ScriptModule(_broker);
-_logModule = new Log.LogModule(_broker);
-_storageModule = new Storage.StorageModule(_broker);
+try
+{
+    _sessionModule = new Session.SessionModule(_broker);
+    _browsingContextModule = new BrowsingContext.BrowsingContextModule(_broker);
+    _browserModule = new Browser.BrowserModule(_broker);
+    _networkModule = new Network.NetworkModule(_broker);
+    _inputModule = new Input.InputModule(_broker);
+    _scriptModule = new Script.ScriptModule(_broker);
+    _logModule = new Log.LogModule(_broker);
+    _storageModule = new Storage.StorageModule(_broker);
+}
+catch
+{
+    _broker?.Dispose();
+    throw;
+}
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that if a module constructor fails, the _broker object might not be disposed, leading to a resource leak; the proposed try-catch block is a robust solution.

    		Medium
    • Update

    RenderMichael
    RenderMichael approved these changes Jun 29, 2025
    View reviewed changes
    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 simplication!

    @nvborisenko
    Copy link
    Member Author

    Thanks Nick and Mike!

    @nvborisenko nvborisenko merged commit 83eb17e into SeleniumHQ:trunk Jun 29, 2025
    11 of 12 checks passed
    @nvborisenko nvborisenko deleted the bidi-nodule-init branch June 29, 2025 22:17
    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
    C-dotnet .NET Bindings Review effort 2/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @nvborisenko @RenderMichael @selenium-ci
    0