[dotnet] Sending GeckoDriver output to a log file.

[diemol]

User description

🔗 Related Issues

Related to #12273

💥 What does this PR do?

This PR enables users to send GeckoDriver logs to a file.

🔧 Implementation Notes

I tried to avoid many changes and mostly touched the FirefoxDriverService class.

💡 Additional Considerations

I did run a test locally, and it works, but I did not find any section in the test suite where DriverService-related code is getting tested.

🔄 Types of changes

• Cleanup (formatting, renaming)
• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)
• Breaking change (fix or feature that would cause existing functionality to change)

---

PR Type

Enhancement

---

Description

• Add log file output capability for GeckoDriver

• Enable redirection of driver process output to file

• Implement async stream reading for log capture

• Add proper resource disposal for log writer

---

Diagram Walkthrough

flowchart LR
  A["DriverService base class"] --> B["Make event methods virtual"]
  B --> C["FirefoxDriverService"]
  C --> D["Add LogPath property"]
  D --> E["Override process events"]
  E --> F["Redirect output to log file"]
  F --> G["Async stream reading"]

Loading

File Walkthrough

Relevant files

Enhancement

DriverService.cs Make event methods virtual for inheritance                              dotnet/src/webdriver/DriverService.cs Make OnDriverProcessStarting method virtual Make OnDriverProcessStarted method virtual +2/-2      FirefoxDriverService.cs Implement GeckoDriver log file output functionality            dotnet/src/webdriver/Firefox/FirefoxDriverService.cs Add LogPath property for log file configuration Override process event handlers for log redirection Implement async stream reading for output capture Add proper disposal of log writer resources +96/-0

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Resource Leak The async ReadStreamAsync tasks are fire-and-forget operations that could continue running after disposal. There's no mechanism to cancel or wait for these tasks during disposal, potentially causing resource leaks or exceptions when writing to a disposed StreamWriter. _ = Task.Run(() => ReadStreamAsync(eventArgs.StandardOutputStreamReader)); } if (eventArgs.StandardErrorStreamReader != null) { _ = Task.Run(() => ReadStreamAsync(eventArgs.StandardErrorStreamReader)); } Thread Safety The logWriter field is accessed from multiple threads without synchronization. The main thread can dispose it while async tasks are still writing to it, leading to potential ObjectDisposedException or race conditions. private async Task ReadStreamAsync(StreamReader reader) { string? line; while ((line = await reader.ReadLineAsync()) != null) { if (logWriter != null) { logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}"); } } } Exception Handling The ReadStreamAsync method lacks exception handling. If stream reading fails or the logWriter becomes disposed during operation, unhandled exceptions could crash the application or leave tasks in a faulted state. private async Task ReadStreamAsync(StreamReader reader) { string? line; while ((line = await reader.ReadLineAsync()) != null) { if (logWriter != null) { logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}"); } } }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Add exception handling for async operations Suggestion Impact: The suggestion was directly implemented - a try-catch block was added around the async stream reading operations with exception logging using System.Diagnostics.Debug.WriteLine code diff: + try + { + string? line; + while ((line = await reader.ReadLineAsync()) != null) + { + if (logWriter != null) + { + logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}"); + } + } + } + catch (Exception ex) + { + // Log or handle the exception appropriately + System.Diagnostics.Debug.WriteLine($"Error reading stream: {ex.Message}"); } The async method lacks exception handling which could cause unhandled exceptions to crash the application. Add try-catch blocks to handle potential I/O exceptions during stream reading and log writing operations. dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [346-356] private async Task ReadStreamAsync(StreamReader reader) { - string? line; - while ((line = await reader.ReadLineAsync()) != null) + try { - if (logWriter != null) + string? line; + while ((line = await reader.ReadLineAsync()) != null) { - logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}"); + if (logWriter != null) + { + logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}"); + } } + } + catch (Exception ex) + { + // Log or handle the exception appropriately + System.Diagnostics.Debug.WriteLine($"Error reading stream: {ex.Message}"); } } [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the ReadStreamAsync method, being run as a fire-and-forget task, lacks exception handling, which could lead to unobserved exceptions and application instability.	Medium
Learned best practice	Add null parameter validation The method should validate that the reader parameter is not null before using it. Add a null check at the beginning of the method to prevent potential NullReferenceException when calling ReadLineAsync(). dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [346-356] private async Task ReadStreamAsync(StreamReader reader) { + ArgumentNullException.ThrowIfNull(reader); + string? line; while ((line = await reader.ReadLineAsync()) != null) { if (logWriter != null) { logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}"); } } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Add null checks and validation for parameters, properties, and return values before using them to prevent NullReferenceException and other runtime errors	Low
General	✅ Set disposed field to null Suggestion Impact: The suggestion was directly implemented - the line "logWriter = null;" was added after the logWriter.Dispose() call in the Dispose method code diff: @@ -256,6 +256,7 @@ if (logWriter != null && disposing) { logWriter.Dispose(); + logWriter = null; The disposal logic should set the logWriter field to null after disposing to prevent potential use-after-dispose scenarios. This ensures the field cannot be accessed after disposal. dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [254-262] protected override void Dispose(bool disposing) { if (logWriter != null && disposing) { logWriter.Dispose(); + logWriter = null; } base.Dispose(disposing); } [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: Setting the logWriter field to null after disposal is a good practice that improves code robustness by preventing potential use-after-dispose errors.	Low
Update

[diemol]

@nvborisenko is there a place where DriverService related code is tested?

[nvborisenko]

#15060 is a right direction to implement requested process flag.

[diemol]

#15060 is a right direction to implement requested process flag.

But how does it work? GeckoDriver has no flag to send the log output to a file.

[diemol]

--log <LEVEL>
     Set Gecko log level [possible values: fatal, error, warn, info,
     config, debug, trace]

That is what the flag does in GeckoDriver.

[nvborisenko]

Surprise :)

I propose to combine the unified approach for all driver services in #15785. Idea is pretty simple: capture stdout by default and redirect the messages to our Log class, which is capable to output to file or/and console (configurable). Thus we resolve many issues users complained, and even supporting log files for geckodriver.

[cgoldberg]

capture stdout by default and redirect the messages to our Log class, which is capable to output to file or/and console

What if the user wants logging to go to a file, but still wants to print to console? Hijacking stdout doesn't sound very nice.

[nvborisenko]

And my proposal covers this case. Even the case when user wants to redirect driver logs to any remote server in runtime.

Currently in chrome:

--log-path=FILE                 write server log to file instead of stderr, increases log level to INFO

[diemol]

I'd say .NET has done it correctly over time, which is using the flags provided to send output to a file. Java did the opposite which is taking all output from all drivers to a file.

I would prefer to do exceptions on a case by case basis rather than making the exception the rule. I am not sure why Java did it that way, but here we are.

When I create another PR to send logs to console (next item on #12273), we can use the Log class for it.

[nvborisenko]

I would prefer to do exceptions on a case by case basis rather than making the exception the rule.

This is good argument.

But there is but. Tomorrow you will implement LogToConsole property in DriverService class. So it will be possible to:

service.LogFile = "C://myfile.txt";
service.LogToConsole = true;

I think it is impossible.

[diemol]

We will follow the same logic Java has. If a path has been set for the log, that has priority. I will document that as well when the PR for log to console is there.

[diemol]

I added a test and also the bazel config to run it on RBE. Seems we are good so I will merge this tomorrow.

[nvborisenko]

AI reviewed your PR and concerns are absolutely correct.