[dotnet] Safe modifications of internal log handlers

[nvborisenko]

Make is safe to modify log handlers while iterating.

💥 What does this PR do?

This pull request refactors the LogHandlerList class to improve its internal implementation and thread safety. The class no longer inherits from List<ILogHandler>, and now manages its own internal array of handlers. This change also updates how handlers are added, removed, and enumerated, ensuring a more robust and maintainable design.

LogHandlerList refactoring and improvements:

• Changed LogHandlerList to no longer inherit from List<ILogHandler>, instead implementing its own internal storage with a volatile array for thread safety.
• Updated Add, Remove, and Clear methods to operate on the internal _handlers array, removing use of base class methods and ensuring modifications are thread-safe.
• Implemented IEnumerable<ILogHandler> explicitly to allow enumeration over the internal array of handlers.
• Added using System.Collections; to support the new enumeration implementation.

Minor code style improvement:

• Added a blank line in LogContext.cs for improved readability.

🔧 Implementation Notes

Copy-on-write approach to achieve immutability internally.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Refactor LogHandlerList with copy-on-write for thread safety

🐞 Bug fix

Walkthroughs

Description

• Refactored LogHandlerList to use copy-on-write pattern for thread safety
• Removed inheritance from List<ILogHandler> for safer handler modifications
• Implemented explicit IEnumerable<ILogHandler> to support safe iteration
• Added blank line in LogContext.cs for improved readability

File Changes

1. dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs 🐞 Bug fix +16/-8

Implement copy-on-write pattern for thread-safe handler management

• Changed class to no longer inherit from List<ILogHandler>, now manages internal _handlers
 array
• Implemented copy-on-write pattern in Add, Remove, and Clear methods for thread safety
• Added explicit IEnumerable<ILogHandler> implementation with GetEnumerator methods
• Added using System.Collections; namespace import

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs

---

2. dotnet/src/webdriver/Internal/Logging/LogContext.cs Formatting +1/-0

Add blank line for readability

• Added blank line after validation check for improved code readability

dotnet/src/webdriver/Internal/Logging/LogContext.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (2)   📎 Requirement gaps (0)   🎨 UX Issues (0)

🐞\ ➹ Performance (1)

📘\ ≡ Correctness (1) ⚙ Maintainability (1)

1. _handlers loses concurrent updates ☑ 📘 ☼

Description

Add, Remove, and Clear update the shared _handlers array via unsynchronized
read-modify-write assignments, so concurrent calls can overwrite each other and silently drop
handlers. This violates the requirement to make concurrent code race-safe with correct
synchronization/atomicity.

Code

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[R31-62]

+    private volatile ILogHandler[] _handlers;

    public LogHandlerList(ILogContext logContext)
    {
        _logContext = logContext;
+        _handlers = [];
    }

    public LogHandlerList(ILogContext logContext, IEnumerable<ILogHandler> handlers)
-        : base(handlers)
    {
        _logContext = logContext;
+        _handlers = [.. handlers];
    }

-    public new ILogContext Add(ILogHandler handler)
+    public ILogContext Add(ILogHandler handler)
    {
-        base.Add(handler);
+        _handlers = [.. _handlers, handler];

        return _logContext;
    }

-    public new ILogContext Remove(ILogHandler handler)
+    public ILogContext Remove(ILogHandler handler)
    {
-        base.Remove(handler);
+        _handlers = [.. _handlers.Where(h => h != handler)];

        return _logContext;
    }

-    public new ILogContext Clear()
+    public ILogContext Clear()
    {
-        base.Clear();
+        _handlers = [];

Evidence

PR Compliance ID 10 requires race-safe shared state updates; volatile only provides visibility,
but the updates here are not atomic. The code assigns _handlers = [.. _handlers, handler] /
_handlers = [.. _handlers.Where(...)] / _handlers = [] without a lock or CAS loop, so two
threads can compute different new arrays from the same old snapshot and the last write wins, losing
the other update.

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[31-62]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LogHandlerList` uses a copy-on-write array but updates it with non-atomic read-modify-write assignments, which can lose updates under concurrent `Add`/`Remove`/`Clear`.

## Issue Context
`volatile` guarantees visibility of the array reference but does not make composite updates atomic; concurrent writers can overwrite each other.

## Fix Focus Areas
- dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[31-62]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Remove() deletes all duplicates 📘 ≡

Description

Remove(ILogHandler handler) now filters out every matching handler instance, changing behavior
from the typical single-removal semantics and potentially breaking callers that add the same handler
more than once. This is a user-visible behavior change on a public interface method without an
established compatibility mechanism.

Code

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[R52-55]

+    public ILogContext Remove(ILogHandler handler)
    {
-        base.Remove(handler);
+        _handlers = [.. _handlers.Where(h => h != handler)];

Evidence

PR Compliance ID 1 requires preserving backward-compatible behavior for public interfaces. The new
implementation uses _handlers.Where(h => h != handler) which removes all occurrences of handler,
rather than removing a single occurrence, which is a potentially breaking behavior change for
ILogHandlerList.Remove.

AGENTS.md
dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[52-55]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ILogHandlerList.Remove` currently removes all duplicate instances of the same handler in one call, which may break callers relying on single-removal behavior.

## Issue Context
The method is part of a public interface (`ILogHandlerList`), so observable semantics changes should be minimized or explicitly documented and tested.

## Fix Focus Areas
- dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[52-55]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. No tests for safe iteration 📘 ⚙

Description

This PR changes observable handler iteration/modification behavior (copy-on-write enumeration) but
does not add/update unit tests to lock in the new guarantees (e.g., modifying handlers during
emission/iteration). Lack of tests risks regressions and violates the requirement to adjust unit
tests for observable behavior changes.

Code

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[R45-68]

+    public ILogContext Add(ILogHandler handler)
    {
-        base.Add(handler);
+        _handlers = [.. _handlers, handler];

        return _logContext;
    }

-    public new ILogContext Remove(ILogHandler handler)
+    public ILogContext Remove(ILogHandler handler)
    {
-        base.Remove(handler);
+        _handlers = [.. _handlers.Where(h => h != handler)];

        return _logContext;
    }

-    public new ILogContext Clear()
+    public ILogContext Clear()
    {
-        base.Clear();
+        _handlers = [];

        return _logContext;
    }
+
+    public IEnumerator<ILogHandler> GetEnumerator() => ((IEnumerable<ILogHandler>)_handlers).GetEnumerator();
+
+    IEnumerator IEnumerable.GetEnumerator() => _handlers.GetEnumerator();

Evidence

PR Compliance ID 11 requires adding/adjusting unit tests when observable behavior changes. The PR
modifies how handlers are stored and enumerated (new array-backed enumerators and copy-on-write
mutations), but existing logging tests do not cover modifying handlers during enumeration/emission,
leaving the new behavior unverified.

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[45-68]
dotnet/test/webdriver/Internal/Logging/LogTests.cs[24-301]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Observable behavior around handler storage/enumeration was changed, but there are no unit tests asserting the new guarantees (e.g., that iterating handlers while adding/removing does not throw and behaves as intended).

## Issue Context
The logging subsystem is user-facing via `Log.Handlers` and is prone to subtle regressions without targeted unit tests.

## Fix Focus Areas
- dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[45-68]
- dotnet/test/webdriver/Internal/Logging/LogTests.cs[24-301]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Per-call array allocation 🐞 ➹

Description

Add and Remove allocate a new array on every call (and Remove also uses LINQ), which is a
performance regression compared to List<T>’s amortized Add and in-place Remove. If handlers
are configured frequently (e.g., per-test or per-scope), this will add avoidable allocations/GC
pressure.

Code

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[R45-55]

+    public ILogContext Add(ILogHandler handler)
    {
-        base.Add(handler);
+        _handlers = [.. _handlers, handler];

        return _logContext;
    }

-    public new ILogContext Remove(ILogHandler handler)
+    public ILogContext Remove(ILogHandler handler)
    {
-        base.Remove(handler);
+        _handlers = [.. _handlers.Where(h => h != handler)];

Evidence

The implementation rebuilds the entire _handlers array for both Add ([.. _handlers, handler])
and Remove ([.. _handlers.Where(...)]), which necessarily allocates a new array each time; Remove
also constructs LINQ iterators during filtering.

dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[45-55]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Add`/`Remove` rebuild arrays via collection expressions and LINQ. This is allocation-heavy compared to list-based approaches.

### Issue Context
Copy-on-write is fine, but you can still implement it with fewer allocations and without LINQ iterator overhead.

### Fix Focus Areas
- dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs[45-57]

### Implementation direction
- Replace LINQ `Where` with a manual loop to count/remove and allocate exactly once.
- Consider using `ImmutableArray<ILogHandler>` or `List<ILogHandler>` internally with a lock, depending on desired tradeoffs.
- If keeping arrays, use `Array.Copy` for Add and a single-pass removal builder for Remove.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[Copilot]

Pull request overview

This PR refactors the .NET internal logging handler collection to avoid InvalidOperationException-style issues when handlers are modified during enumeration (copy-on-write snapshotting), and includes a minor formatting tweak.

Changes:

• Refactor LogHandlerList to stop inheriting from List<ILogHandler> and use an internal handler array with custom add/remove/clear + enumeration.
• Update enumeration to iterate over a stable snapshot of handlers.
• Minor whitespace-only change in LogContext.cs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs	Replaces List<T> inheritance with snapshot-array storage and custom mutation/enumeration methods.
dotnet/src/webdriver/Internal/Logging/LogContext.cs	Adds a blank line in the constructor for readability (but currently includes trailing whitespace).