[dotnet] [bidi] Thread safe events registration

[nvborisenko]

This pull request refactors the event handler management in the EventDispatcher class to improve thread safety and performance. The main change is replacing the use of a List<EventHandler> with a copy-on-write array and introducing locking for handler modifications.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Implement thread-safe event handler management with copy-on-write array

🐞 Bug fix

Walkthroughs

Description

• Replace List<EventHandler> with thread-safe copy-on-write array
• Add locking mechanism for handler modifications in EventRegistration
• Encapsulate handler access through AddHandler, RemoveHandler, GetHandlers methods
• Improve thread safety during concurrent event subscription and processing

File Changes

1. dotnet/src/webdriver/BiDi/EventDispatcher.cs 🐞 Bug fix +18/-4

Thread-safe copy-on-write event handler array implementation

• Replaced List<EventHandler> Handlers with private EventHandler[] _handlers array in
 EventRegistration class
• Added lock object _lock for synchronizing handler modifications
• Introduced three new methods: GetHandlers() returns current handlers array, AddHandler()
 appends handler with lock protection, RemoveHandler() removes handler with lock protection
• Updated SubscribeAsync() to call registration.AddHandler() instead of direct list add
• Updated UnsubscribeAsync() to call registration.RemoveHandler() instead of direct list remove
• Updated ProcessEventsAwaiterAsync() to call registration.GetHandlers() instead of
 registration.Handlers.ToArray()

dotnet/src/webdriver/BiDi/EventDispatcher.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (2) 📎 Requirement gaps (0)

1. GetHandlers lacks volatile read ☑ 📘 Rule violation ⛯ Reliability

Description

_handlers is written under a lock but read without any memory-barrier/volatile semantics, so other
threads may observe stale handler arrays and miss recent add/remove operations. This undermines the
intended thread-safety of event handler registration and dispatch.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R132-147]

+        private readonly object _lock = new();
+        private EventHandler[] _handlers = [];
+
       public JsonTypeInfo TypeInfo { get; } = typeInfo;
-        public List<EventHandler> Handlers { get; } = [];
+
+        public EventHandler[] GetHandlers() => _handlers;
+
+        public void AddHandler(EventHandler handler)
+        {
+            lock (_lock) _handlers = [.. _handlers, handler];
+        }
+
+        public void RemoveHandler(EventHandler handler)
+        {
+            lock (_lock) _handlers = Array.FindAll(_handlers, h => h != handler);
+        }

Evidence

PR Compliance ID 11 requires atomic, thread-safe updates for shared async/event state. The new
copy-on-write implementation updates _handlers under lock, but GetHandlers() returns
_handlers directly (no volatile/Volatile.Read/Interlocked), while the event loop
concurrently iterates over it.

dotnet/src/webdriver/BiDi/EventDispatcher.cs[132-147]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[98-105]
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
`EventRegistration._handlers` is updated under `lock` but read via `GetHandlers()` without `volatile`/`Volatile.Read`/`Interlocked` semantics. This can lead to stale reads across threads and undermines the intended thread-safety improvements.
## Issue Context
This PR introduces a copy-on-write array for event handlers and claims it is "safe to iterate". For correctness under the .NET memory model, publishing the new array reference should include appropriate visibility guarantees for lock-free readers.
## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[132-147]
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[98-105]

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

---

2. No tests for EventDispatcher 📘 Rule violation ⛯ Reliability

Description

This PR changes event handler registration/removal behavior for thread safety but does not add or
adjust any automated tests in the provided changes. The refactor risks regressions (missed handlers,
concurrency issues) without targeted coverage.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R139-147]

+        public void AddHandler(EventHandler handler)
+        {
+            lock (_lock) _handlers = [.. _handlers, handler];
+        }
+
+        public void RemoveHandler(EventHandler handler)
+        {
+            lock (_lock) _handlers = Array.FindAll(_handlers, h => h != handler);
+        }

Evidence

PR Compliance ID 4 requires tests for fixes/behavior changes, preferably small/unit tests. The PR is
described as a bug fix refactoring handler management for thread safety, but the provided diff only
contains production changes and no test updates.

AGENTS.md
dotnet/src/webdriver/BiDi/EventDispatcher.cs[139-147]

Agent prompt

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

## Issue description
The PR changes `EventDispatcher` handler storage and concurrency behavior but does not include tests to validate the new thread-safety and prevent regressions.
## Issue Context
Existing BiDi tests appear to be largely integration/browser-oriented; this change is a good candidate for a focused unit test around handler registration/removal and dispatch iteration.
## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[87-105]
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[130-147]

ⓘ 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

Refactors .NET BiDi EventDispatcher handler storage to reduce per-event allocations and improve thread safety during event dispatch by switching from a mutable List<EventHandler> to a copy-on-write array with locked updates.

Changes:

• Replace EventRegistration.Handlers (List<EventHandler>) with a copy-on-write EventHandler[].
• Add AddHandler/RemoveHandler APIs guarded by a private lock.
• Iterate event handlers via GetHandlers() without allocating a per-dispatch snapshot.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

dotnet/test/common/BiDi/Session/SessionTests.cs:54

• Same issue here: the lambda returns a Task and is not awaited, so Throws.InstanceOf<TaskCanceledException>() won’t reliably observe the cancellation. Prefer an async delegate (async () => await bidi.StatusAsync(...)) or Assert.ThrowsAsync when asserting exceptions from async APIs.

    public void ShouldRespectCancellationToken()
    {
        using var cts = new CancellationTokenSource(TimeSpan.FromMicroseconds(1));

        Assert.That(
            () => bidi.StatusAsync(cancellationToken: cts.Token),
            Throws.InstanceOf<TaskCanceledException>());
    }