[dotnet] Add dispose on constructor failure to ensure driver closes

[MASACR99]

User description

Description

First of all I just changed the dotnet version first since is the one my project uses so I could test it out, the idea behind this change is just that if the driver fails to start a browser version (for example if there's a missmatch between the version and driver) the driver closes instead of staying open in the background. For that I just added a try catch on the WebDriver constructor and in case there's an exception I just force the disposal.

Motivation and Context

The issue came from my work where we use selenium on a server, but the IT guy managing the server refuses to force a version for the browser so from time to time when any of the browsers updates and the driver stops working it leaves a ton of drivers open in task manager which must be closed before upgrading the driver, with this fix these rogue driver sessions should stop.

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.

I don't think this change requires updated tests, 5 tests failed locally but don't seem to be related to my change

---

Type

Bug fix

---

Description

• Added error handling in the WebDriver constructor to ensure resources are properly disposed of in case of an exception during driver initialization.
• This change aims to address issues where driver instances remain running in the background if an exception occurs, particularly useful in environments where browser versions frequently change.

---

Changes walkthrough

	Relevant files
Error handling	WebDriver.cs Add Exception Handling to WebDriver Constructor                    dotnet/src/webdriver/WebDriver.cs Wrapped StartSession and subsequent initializations in a try-catch block. In the catch block, added a call to Dispose before rethrowing the exception. +18/-9

---

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[CLAassistant]

All committers have signed the CLA.

[codiumai-pr-agent-pro]

PR Description updated to latest commit (d7e3d74)

• Copy walkthrough table to "Files Changed" Tab

[codiumai-pr-agent-pro]

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are localized to a specific constructor in a single file, and the logic is straightforward. The addition of a try-catch block to handle exceptions during the WebDriver initialization is a common pattern, making the review process less complex.
🧪 Relevant tests	No
🔍 Possible issues	Exception Handling: The use of throw ex; in the catch block will reset the stack trace of the exception, which can hinder debugging. It's generally recommended to use throw; without the exception variable to rethrow the current exception while preserving its original stack trace.
🔒 Security concerns	No

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

Utilizing extra instructions The review tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize. Examples for extra instructions: [pr_reviewer] # /review # extra_instructions=""" In the 'possible issues' section, emphasize the following: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

How to enable\disable automation When you first install PR-Agent app, the default mode for the review tool is: pr_commands = ["/review", ...] meaning the review tool will run automatically on every PR, with the default configuration. Edit this field to enable/disable the tool, or to change the used configurations

Auto-labels The review tool can auto-generate two specific types of labels for a PR: a possible security issue label, that detects possible security issues (enable_review_labels_security flag) a Review effort [1-5]: x label, where x is the estimated effort to review the PR (enable_review_labels_effort flag)

Extra sub-tools The review tool provides a collection of possible feedbacks about a PR. It is recommended to review the possible options, and choose the ones relevant for your use case. Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example: require_score_review, require_soc2_ticket, and more.

Auto-approve PRs By invoking: /review auto_approve The tool will automatically approve the PR, and add a comment with the approval. To ensure safety, the auto-approval feature is disabled by default. To enable auto-approval, you need to actively set in a pre-defined configuration file the following: [pr_reviewer] enable_auto_approval = true (this specific flag cannot be set with a command line argument, only in the configuration file, committed to the repository) You can also enable auto-approval only if the PR meets certain requirements, such as that the estimated_review_effort is equal or below a certain threshold, by adjusting the flag: [pr_reviewer] maximal_review_effort = 5

More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

See the review usage page for a comprehensive guide on using this tool.

[codiumai-pr-agent-pro]

PR Code Suggestions

Category	Suggestions
Best practice	Use throw; instead of throw ex; to preserve the exception's original stack trace. Instead of using throw ex; which resets the stack trace of the exception, use throw; to preserve the original stack trace. This will help in debugging by keeping the original call stack intact. dotnet/src/webdriver/WebDriver.cs [73-76] -catch(Exception ex) +catch(Exception) { this.Dispose(); - throw ex; + throw; }

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.
When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

Enabling\disabling automation When you first install the app, the default mode for the improve tool is: pr_commands = ["/improve --pr_code_suggestions.summarize=true", ...] meaning the improve tool will run automatically on every PR, with summarization enabled. Delete this line to disable the tool from running automatically.

Utilizing extra instructions Extra instructions are very important for the improve tool, since they enable to guide the model to suggestions that are more relevant to the specific needs of the project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify relevant aspects that you want the model to focus on. Examples for extra instructions: [pr_code_suggestions] # /improve # extra_instructions=""" Emphasize the following aspects: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

A note on code suggestions quality While the current AI for code is getting better and better (GPT-4), it's not flawless. Not all the suggestions will be perfect, and a user should not accept all of them automatically. Suggestions are not meant to be simplistic. Instead, they aim to give deep feedback and raise questions, ideas and thoughts to the user, who can then use his judgment, experience, and understanding of the code base. Recommended to use the 'extra_instructions' field to guide the model to suggestions that are more relevant to the specific needs of the project, or use the custom suggestions 💎 tool With large PRs, best quality will be obtained by using 'improve --extended' mode.

More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

See the improve usage page for a more comprehensive guide on using this tool.

[diemol]

This is not the correct solution for your issue. Selenium Grid has a way to stop processes when a session fails to start, and the server is responsible for cleaning up when something fails during the creation.

In addition, this won't work in all cases because there is no guarantee a session was created, which causes the user to see exceptions will be thrown for no reason.

If you still want that code, please put it in your framework. On the other hand, we can help with the Grid configuration. Join us in our Slack channel: https://www.selenium.dev/support/#ChatRoom

[MASACR99]

Hi, thanks diemol for the feedback but I still think there should be a way the Webdriver closes the driver if it's left open after trying to start a session, it takes a lot less effort to just check if the driver was created and dispose of it rather than having the server have to check its processes and kill the zombie drivers.

I can add a check in the exception to check if the driver was created to avoid the dispose generating errors but, as I see the current implementation of the dispose, it already checks some exceptions and supresses them.

Thanks for your time and your work on these projects!

[diemol]

I just think issues should get fixed where they happen and avoid workarounds. But I am open to hearing other opinions... @nvborisenko @titusfortner

[nvborisenko]

Ideally WebDriver should not perform any operations in the constructor. Ideally we want to see something like (in Selenium v5?):

using var driver = new ChromeDriver();
await driver.StartAsync(); // or any other method name to initiate new session

In this way ctor is just an initialization of the object state, what is good. There are no any hidden activities. But we cannot just break existing logic.

This issue is about if this.StartSession(capabilities); throws an exception, then user has no ways to clean up all underlying disposable stuff.

For this particular case I can accept:

try
{
  this.StartSession(capabilities);
}
catch (Exception)
{
  // !!! dispose specific resources, don't call this.Dispose()
  throw;
}

[MASACR99]

Sorry for the delay had a few things to manage. This afternoon I'm gonna implement this changes, I just have 1 question, if we try to dispose the driver and it fails to receive the Quit command should the exception be thrown (there'd be an exception thrown inside the catch of the StartSession command) or managed and suppressed?

[nvborisenko]

I haven't looked at code yet, and I expect Quit() method should raise an exception. If the object is constructed successfully, then we suppose Dispose/Quit also should be successful, we should not be silent and ignore exceptions - raise it to end user.

Looked at code: we are silent if Quit() throws an exception, the same in Java. Let's don't touch this behavior in scope of your pull request.

[nvborisenko]

@MASACR99 thank you. Before moving further can we see exact exception (or problem) you met, which is motivated you to create this PR. I am asking because of while I was reviewing your PR I saw other issues related to disposing. And now I am not sure which case we are trying to resolve here. Am I right that if this.StartSession() throws an exception, then we meet resources leaking?

[MASACR99]

Yes so the current issue I'm trying to fix happens when creating a new WebDriver (in my case I managed to test with Chrome and Firefox), if I specify a driver version that isn't compatible with the currently installed driver (I used in this test the chromedriver for version 114 and have chrome version 122.0.6261.112 installed) and try to create a new driver I get an output error with the message

"Unhandled exception. System.InvalidOperationException: session not created: This version of ChromeDriver only supports Chrome version 114"

With the stack trace being:

Current browser version is 122.0.6261.112 with binary path C:\Program Files\Google\Chrome\Application\chrome.exe (SessionNotCreated)
at OpenQA.Selenium.WebDriver.UnpackAndThrowOnError(Response errorResponse, String commandToExecute)
at OpenQA.Selenium.WebDriver.Execute(String driverCommandToExecute, Dictionary`2 parameters)
at OpenQA.Selenium.WebDriver.StartSession(ICapabilities capabilities)
at OpenQA.Selenium.WebDriver..ctor(ICommandExecutor executor, ICapabilities capabilities)
at OpenQA.Selenium.Chromium.ChromiumDriver..ctor(ChromiumDriverService service, ChromiumOptions options, TimeSpan commandTimeout)
at OpenQA.Selenium.Chrome.ChromeDriver..ctor(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout)
at OpenQA.Selenium.Chrome.ChromeDriver..ctor(String chromeDriverDirectory, ChromeOptions options, TimeSpan commandTimeout)
at OpenQA.Selenium.Chrome.ChromeDriver..ctor(String chromeDriverDirectory, ChromeOptions options)
at OpenQA.Selenium.Chrome.ChromeDriver..ctor(String chromeDriverDirectory)
at Program.

$(String[] args) in C:\Users\username\source\repos\test\test\Program.cs:line 4

And after the program has crashed there will be a driver.exe kept open in the task manager (I've been able to replicate on different windows machines not sure if Linux or Mac would detect te process and close it)

If you want some steps to reproduce the error I'll list them here:

1. Get a driver version not matching currently installed target browser
2. Create new driver for browser forcing incorrect driver
3. An error will happen and a "browsername"driver.exe will be kept open in the process list

I'll also add the code I used for this test

(code in C# .NET8 with latest version of selenium nugget installed):

using OpenQA.Selenium.Chrome;

var driver = new ChromeDriver("C:\\repos\\test\\test\\chromedriver.exe");

driver.Navigate().GoToUrl("https://www.google.com");

Keep in mind in this specific test, after the console application closes, any processes created by it will close BUT if a windows service has this issue the chromedriver will stay open on the background.

And I'll also attach some images showing proof of this happening (i've censored the user name folder in the screenshots)

[nvborisenko]

And does Quit() method close the chromedriver.exe process?

[nvborisenko]

Thank you @MASACR99 !