Skip to content
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

WIP [dotnet] Reusable DriverService instance by Drivers #14662

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

WIP [dotnet] Reusable DriverService instance by Drivers #14662

wants to merge 3 commits into from

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 27, 2024 •
edited
Loading

User description

DriverService instance seems to be reusable by Drivers, but in fact no. As soon as Quit command is handled by Drivers, then underlying DriverService is disposed silently.

https://github.com/SeleniumHQ/selenium/blame/215e20b139f9af0c2db76b800472daed0633c73c/dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs#L128 - this is bad design.

Description

Motivation and Context

Fixes #14624 and #14633

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

  • Introduced a disposeService parameter across various driver classes to control the disposal of driver services.
  • Updated constructors and methods to accommodate the new disposeService parameter.
  • Removed the DriverServiceCommandExecutor class and replaced its functionality with direct service management.
  • Enhanced the disposal process in the WebDriver class to ensure proper resource management.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
ChromeDriver.cs
Add disposeService parameter to ChromeDriver constructors

dotnet/src/webdriver/Chrome/ChromeDriver.cs

  • Added disposeService parameter to constructors.
  • Updated constructor calls to include disposeService.
  • Added method to handle custom Chrome commands.
    		•  +14/-3   
    ChromiumDriver.cs
    Enhance ChromiumDriver with service disposal control         

    dotnet/src/webdriver/Chromium/ChromiumDriver.cs

  • Introduced disposeDriverService field.
  • Modified command executor method to start service.
  • Updated dispose method to handle driver service disposal.
    		•  +16/-11 
    EdgeDriver.cs
    Update EdgeDriver constructor for service disposal             

    dotnet/src/webdriver/Edge/EdgeDriver.cs

    • Updated constructor to include disposeService parameter.
    		 +1/-1     
    FirefoxDriver.cs
    Update FirefoxDriver to start service in executor               

    dotnet/src/webdriver/Firefox/FirefoxDriver.cs

  • Renamed command executor method to start service.
  • Updated constructor to use new method.
    		•  +6/-3     
    InternetExplorerDriver.cs
    Modify InternetExplorerDriver to start service in executor

    dotnet/src/webdriver/IE/InternetExplorerDriver.cs

  • Renamed command executor method to start service.
  • Updated constructor to use new method.
    		•  +6/-3     
    DriverServiceCommandExecutor.cs
    Remove DriverServiceCommandExecutor class                               

    dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs

    • Removed DriverServiceCommandExecutor class.
    		 +0/-163 
    SafariDriver.cs
    Update SafariDriver to start service in executor                 

    dotnet/src/webdriver/Safari/SafariDriver.cs

  • Renamed command executor method to start service.
  • Updated constructor to use new method.
    		•  +6/-3     
    WebDriver.cs
    Ensure executor disposal in WebDriver class                           

    dotnet/src/webdriver/WebDriver.cs

    • Added disposal of executor in Dispose method.
    		 +1/-0     
    DriverFactory.cs
    Modify DriverFactory to pass service in driver instantiation

    dotnet/test/common/Environment/DriverFactory.cs

    • Updated driver instantiation to include service parameter.
    		 +1/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Potential Resource Leak
    The driverService field is not being disposed in all scenarios. Ensure proper disposal of resources.

    Code Duplication
    Multiple constructors with similar logic. Consider refactoring to reduce duplication.

    Naming Inconsistency
    Method renamed from GenerateDriverServiceCommandExecutor to StartDriverServiceCommandExecutor. Ensure consistency across all driver classes.

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null check before disposing of the executor to prevent potential exceptions

    Consider wrapping the executor disposal in a null check to prevent potential null
    reference exceptions.

    dotnet/src/webdriver/WebDriver.cs [732]

    -this.executor.Dispose();
+this.executor?.Dispose();
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a null check before disposing of the executor is a good practice to prevent potential null reference exceptions, which could lead to runtime errors. This suggestion addresses a possible issue effectively.

    		8
    Add null check for the service parameter to prevent potential null reference exceptions

    Consider adding a null check before using the service parameter in the
    CreateDriverWithOptions method to prevent potential null reference exceptions.

    dotnet/test/common/Environment/DriverFactory.cs [158]

    +if (service == null)
+{
+    throw new ArgumentNullException(nameof(service));
+}
 driver = (IWebDriver)Activator.CreateInstance(driverType, service, true);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion to add a null check for the service parameter is important to prevent null reference exceptions, which could cause runtime errors. This is a valuable addition to ensure robustness in the method.

    		8
    Enhancement
    Implement constructor chaining to reduce code duplication and improve maintainability

    Consider using a constructor chaining pattern to reduce code duplication across the
    various constructors. This can help maintain consistency and make future updates
    easier.

    dotnet/src/webdriver/Chrome/ChromeDriver.cs [138-164]

     public ChromeDriver(ChromeDriverService service, ChromeOptions options)
-    : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService: true)
+    : this(service, options, RemoteWebDriver.DefaultCommandTimeout)
 {
 }
 
 public ChromeDriver(ChromeDriverService service, ChromeOptions options, bool disposeService)
     : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService)
 {
 }
 
 public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout)
-    : base(service, options, commandTimeout, disposeService: true)
+    : this(service, options, commandTimeout, true)
 {
-    this.AddCustomChromeCommands();
 }
 
 public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout, bool disposeService)
     : base(service, options, commandTimeout, disposeService)
 {
     this.AddCustomChromeCommands();
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to implement constructor chaining is valid as it reduces code duplication and enhances maintainability. The improved code correctly applies the chaining pattern, making the constructors more concise and easier to update in the future.

    		7
    Use null-coalescing assignment operator for more concise field initialization

    Consider using a null-coalescing assignment operator ??= to simplify the
    initialization of the driverService field if it's not already set.

    dotnet/src/webdriver/Chromium/ChromiumDriver.cs [131-136]

     protected ChromiumDriver(ChromiumDriverService service, ChromiumOptions options, TimeSpan commandTimeout, bool disposeService)
     : base(StartDriverServiceCommandExecutor(service, options, commandTimeout), ConvertOptionsToCapabilities(options))
 {
-    this.driverService = service;
+    this.driverService ??= service;
     this.disposeDriverService = disposeService;
     this.optionsCapabilityName = options.CapabilityName;
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to use a null-coalescing assignment operator is correct and makes the code more concise. However, the impact is minor since the field is being set in the constructor and is unlikely to be null at this point.

    		5

    💡 Need additional feedback ? start a PR chat

    @nvborisenko
    Copy link
    Member Author

    To move further we have to decide:

    • Just fix "incorrect/old" behavior, which is not source/binary breaking change, but changes "old" behavior
    • Or keep "old" behavior and introduce an ability to specify whether ChromeDriver should dispose underlying ChromeDriverService (chromedriver.exe)

    @YevgeniyShunevych your input is appreciated here.

    @nvborisenko
    Copy link
    Member Author

    I vote for just fix "old" behavior. If user instantiates DriverService object, he is responsible to Dispose it. This is general rule to Dispose everything which is Disposable. So let's fix/break it. If new issues will be reported, then we can say "You instantiated, you should dispose it".

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @jimevans jimevans Awaiting requested review from jimevans

    @titusfortner titusfortner Awaiting requested review from titusfortner

    Assignees
    No one assigned
    Labels
    C-dotnet Review effort [1-5]: 4
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: C#: Driver process left running if DriverService is reused and Driver disposed
    1 participant
    @nvborisenko