[dotnet] Conditionally enable driver service process output redirection

[nvborisenko]

User description

🔗 Related Issues

Related to #16245

💥 What does this PR do?

Introduces new hidden flag whether process output should be listened to. By default we just redirect but not listen it. In case of Firefox driver, if LogPath property is set, then we start process redirection with listening to output.

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Conditionally enable driver service process output redirection

• Add EnableProcessRedirection property to control output listening

• Firefox driver enables redirection only when LogPath is set

• Remove default logging behavior from base class

---

Diagram Walkthrough

flowchart LR
  A["DriverService"] --> B["EnableProcessRedirection property"]
  B --> C["Conditional output listening"]
  C --> D["FirefoxDriverService"]
  D --> E["LogPath-based redirection"]

Loading

File Walkthrough

Relevant files

Enhancement

DriverService.cs Add conditional process output redirection control              dotnet/src/webdriver/DriverService.cs Add EnableProcessRedirection virtual property with default false value Conditionally attach/detach output event handlers based on property Remove default trace logging from OnDriverProcessDataReceived method Guard disposal cleanup with redirection check +21/-12  FirefoxDriverService.cs Enable redirection when LogPath configured                              dotnet/src/webdriver/Firefox/FirefoxDriverService.cs Override EnableProcessRedirection to return true when LogPath is set Enable output redirection only when logging is configured +5/-0

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 16245 - Partially compliant Compliant requirements: Avoid unnecessary driver service process output listening that causes contention/overhead. Preserve ability to capture driver logs when explicitly requested/configured. Ensure Firefox continues to log when a log path is provided. Non-compliant requirements: Address performance degradation in parallel test execution introduced in v4.35. Maintain backward compatibility for normal usage patterns. Requires further human verification: Address performance degradation in parallel test execution introduced in v4.35 (needs benchmarking under parallel load). Maintain backward compatibility for normal usage patterns (verify other driver services and consumers that relied on default logging).

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Behavior Change Removing default trace logging in OnDriverProcessDataReceived may impact consumers who relied on base-class logging. Validate that no downstream diagnostics regress unintentionally. /// <summary> /// Handles the output and error data received from the driver process. /// </summary> /// <param name="sender">The sender of the event.</param> /// <param name="args">The data received event arguments.</param> protected virtual void OnDriverProcessDataReceived(object sender, DataReceivedEventArgs args) { } Redirection Toggle Logic Output/error handlers and Begin*ReadLine are only attached when EnableProcessRedirection is true. Confirm that other driver services override this property when needed; otherwise, logs and event-driven behaviors might silently stop. if (this.EnableProcessRedirection) { this.driverServiceProcess.OutputDataReceived += this.OnDriverProcessDataReceived; this.driverServiceProcess.ErrorDataReceived += this.OnDriverProcessDataReceived; } DriverProcessStartingEventArgs eventArgs = new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo); this.OnDriverProcessStarting(eventArgs); this.driverServiceProcess.Start(); if (this.EnableProcessRedirection) { // Important: Start the process and immediately begin reading the output and error streams to avoid IO deadlocks. this.driverServiceProcess.BeginOutputReadLine(); this.driverServiceProcess.BeginErrorReadLine(); } LogPath Null Semantics Redirection is enabled only when LogPath is not null. Verify that empty string or whitespace paths are treated correctly and that legacy configurations still trigger logging as expected. /// <summary> /// Gets a value indicating whether process output redirection is required. /// </summary> protected internal override bool EnableProcessRedirection => LogPath is not null;

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent potential process deadlock To prevent a potential process deadlock, move the RedirectStandardOutput and RedirectStandardError property assignments inside the if (this.EnableProcessRedirection) block. dotnet/src/webdriver/DriverService.cs [257-264] -this.driverServiceProcess.StartInfo.RedirectStandardOutput = true; -this.driverServiceProcess.StartInfo.RedirectStandardError = true; - if (this.EnableProcessRedirection) { + this.driverServiceProcess.StartInfo.RedirectStandardOutput = true; + this.driverServiceProcess.StartInfo.RedirectStandardError = true; this.driverServiceProcess.OutputDataReceived += this.OnDriverProcessDataReceived; this.driverServiceProcess.ErrorDataReceived += this.OnDriverProcessDataReceived; } Apply / Chat Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical bug that can lead to a process deadlock when EnableProcessRedirection is false, as output redirection is enabled without a corresponding stream reader.	High
High-level	Apply the fix to all drivers Extend the conditional output redirection logic to all driver services, not just Firefox. This involves overriding EnableProcessRedirection in each service to enable it when a log path is configured, ensuring logging functionality is preserved across all drivers. Examples: dotnet/src/webdriver/DriverService.cs [179] protected virtual internal bool EnableProcessRedirection { get; } = false; dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [228] protected internal override bool EnableProcessRedirection => LogPath is not null; Solution Walkthrough: Before: // In DriverService.cs public abstract class DriverService { protected virtual internal bool EnableProcessRedirection { get; } = false; // ... } // In FirefoxDriverService.cs public sealed class FirefoxDriverService : DriverService { public string? LogPath { get; set; } protected internal override bool EnableProcessRedirection => LogPath is not null; } // Other services like ChromeDriverService implicitly use the base implementation, // so EnableProcessRedirection is always false, breaking logging. After: // In DriverService.cs public abstract class DriverService { protected virtual internal bool EnableProcessRedirection { get; } = false; // ... } // In FirefoxDriverService.cs (as in PR) public sealed class FirefoxDriverService : DriverService { public string? LogPath { get; set; } protected internal override bool EnableProcessRedirection => LogPath is not null; } // In other services like ChromeDriverService (suggested change) public sealed class ChromeDriverService : DriverService { public string? LogPath { get; set; } protected internal override bool EnableProcessRedirection => LogPath is not null; } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant flaw where the PR fixes a performance issue but breaks logging for all drivers except Firefox, making the fix incomplete and introducing a regression.	High
Learned best practice	Always detach event handlers Unsubscribe from event handlers unconditionally when a process exists to avoid leaks if redirection was toggled during runtime or partially attached. dotnet/src/webdriver/DriverService.cs [307-311] -if (EnableProcessRedirection && this.driverServiceProcess is not null) +if (this.driverServiceProcess is not null) { this.driverServiceProcess.OutputDataReceived -= this.OnDriverProcessDataReceived; this.driverServiceProcess.ErrorDataReceived -= this.OnDriverProcessDataReceived; } Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Always dispose and unsubscribe resources/listeners deterministically to prevent leaks.	Low
General	Correct inaccurate property documentation Correct the XML documentation for the EnableProcessRedirection property to state it is get-only, not "Gets or sets", to match its implementation. dotnet/src/webdriver/DriverService.cs [179] +protected virtual internal bool EnableProcessRedirection { get; } = false; - Apply / Chat Suggestion importance[1-10]: 3 __ Why: The suggestion correctly points out an inconsistency between the code and its documentation, which is a minor but valid code quality issue that improves maintainability.	Low
Update

[nvborisenko]

Merging this important update because nobody will review it. Thanks Nikolay.