[dotnet] [bidi] Wait until events are dispatched when unsubscribing

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.

[nvborisenko]

🔗 Related Issues

Fix#16574

💥 What does this PR do?

Refactors the event dispatching mechanism in the EventDispatcher class to improve thread safety, reliability, and event handler management. The changes introduce a new EventItem structure, enhance handler registration/removal, and ensure that all pending events are processed before unsubscribing handlers. Error handling and concurrency controls have also been improved.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Refactors event dispatching to ensure all pending events are processed before unsubscribing handlers

• Introduces sequence-based draining mechanism to track enqueued and processed events per registration

• Improves thread safety with proper locking and atomic operations for handler management

• Enhances error handling with separate try-catch blocks for deserialization and handler invocation

• Adds handler before remote subscription to prevent race conditions on subscription failure

---

File Walkthrough

Relevant files

Bug fix

EventDispatcher.cs Event draining mechanism and thread-safe handler management dotnet/src/webdriver/BiDi/EventDispatcher.cs Replaced PendingEvent struct with EventItem record containing registration reference instead of TypeInfo Refactored EventRegistration class with sequence-based draining mechanism using _enqueueSeq and _processedSeq counters Added DrainAsync() method to wait until all pending events for a registration are processed before handler removal Introduced thread-safe handler management with AddHandler(), RemoveHandler(), and GetHandlersSnapshot() methods Simplified task creation by replacing TaskFactory with direct Task.Run() call Enhanced error handling with separate exception handling for deserialization and handler invocation Added handler registration before remote subscription with rollback on failure +111/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #16574 🟢 Prevent race conditions where events are still in-flight / queued when the subscription is disposed or the connection is closing. ⚪ Ensure BiDi log events (e.g., console logs) are reliably delivered to the registered OnEntryAddedAsync handler without requiring a time-based delay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent event drop: The result of _pendingEvents.Writer.TryWrite(...) is ignored, which can silently drop events (e.g., after Complete() during disposal) without any fallback or logging. Referred Code if(_events.TryGetValue(method,outvarregistration)){registration.IncrementEnqueued();_pendingEvents.Writer.TryWrite(newEventItem(jsonUtf8Bytes,bidi,registration));} Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Exception details logged: Error logs interpolate Exception objects (e.g., $"... {ex}"), which may include sensitive data from exception messages/stack traces depending on upstream content and logging sinks. Referred Code catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error processing BiDi event handler: {ex}");}}}}catch(Exceptionex){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Unhandled error deserializing BiDi event: {ex}");} Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Increment only on successful enqueue Suggestion Impact: EnqueueEvent was changed to call IncrementEnqueued() only when _pendingEvents.Writer.TryWrite(...) returns true, aligning the enqueue counter with successful channel writes; additionally added a warning log when TryWrite fails. code diff: @@ -85,8 +85,17 @@ { if (_events.TryGetValue(method, out var registration)) { - registration.IncrementEnqueued();- _pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+ if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+ {+ registration.IncrementEnqueued();+ }+ else+ {+ if (_logger.IsEnabled(LogEventLevel.Warn))+ {+ _logger.Warn($"Failed to enqueue BiDi event with method '{method}' for processing. Event will be ignored.");+ }+ } } In EnqueueEvent, only call registration.IncrementEnqueued() if _pendingEvents.Writer.TryWrite(...) returns true to prevent a potential deadlock in DrainAsync. dotnet/src/webdriver/BiDi/EventDispatcher.cs [88-89] -registration.IncrementEnqueued();-_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration));+if (_pendingEvents.Writer.TryWrite(new EventItem(jsonUtf8Bytes, bidi, registration)))+{+ registration.IncrementEnqueued();+} [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where DrainAsync could hang indefinitely if TryWrite fails, by correctly synchronizing the enqueue counter with the channel write operation.	High
	Prevent race condition during subscription To prevent a race condition, move the registration.AddHandler(eventHandler) call to after the _sessionProvider().SubscribeAsync call succeeds in the SubscribeAsync method. dotnet/src/webdriver/BiDi/EventDispatcher.cs [51-69] public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo, CancellationToken cancellationToken) where TEventArgs : EventArgs { var registration = _events.GetOrAdd(eventName, _ => new EventRegistration(jsonTypeInfo)); - registration.AddHandler(eventHandler);- try { var subscribeResult = await _sessionProvider().SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }, cancellationToken).ConfigureAwait(false); ++ registration.AddHandler(eventHandler); return new Subscription(subscribeResult.Subscription, this, eventHandler); } catch { - registration.RemoveHandler(eventHandler);+ // No need to remove the handler as it was never added in case of an exception. throw; } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition where an event handler could process an event before its subscription is confirmed, improving the atomicity and correctness of the subscription logic.	Medium
	✅ Propagate cancellation token Suggestion Impact: Updated the UnsubscribeAsync flow to pass cancellationToken into DrainAsync, and changed DrainAsync to accept and honor a CancellationToken by returning a cancellable task continuation. code diff: @@ -75,7 +75,7 @@ await _sessionProvider().UnsubscribeAsync([subscription.SubscriptionId], null, cancellationToken).ConfigureAwait(false); // Wait until all pending events for this method are dispatched - await registration.DrainAsync().ConfigureAwait(false);+ await registration.DrainAsync(cancellationToken).ConfigureAwait(false); registration.RemoveHandler(subscription.EventHandler); } @@ -199,7 +199,7 @@ } } - public Task DrainAsync()+ public Task DrainAsync(CancellationToken cancellationToken) { lock (_drainLock) { @@ -218,7 +218,13 @@ return Task.CompletedTask; } - return tcs.Task;+ if (!cancellationToken.CanBeCanceled) return tcs.Task;++ return tcs.Task.ContinueWith(+ static _ => { },+ cancellationToken,+ TaskContinuationOptions.None,+ TaskScheduler.Default); } Propagate the cancellationToken from UnsubscribeAsync to registration.DrainAsync to allow the draining operation to be cancelled. dotnet/src/webdriver/BiDi/EventDispatcher.cs [78] -await registration.DrainAsync().ConfigureAwait(false);+await registration.DrainAsync(cancellationToken).ConfigureAwait(false); [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes propagating the cancellationToken to DrainAsync to allow cancellation of the wait, which improves responsiveness and resource management.	Medium
High-level	Consider simplifying the custom synchronization logic The custom synchronization logic in EventRegistration for DrainAsync is complex and error-prone. It should be simplified using higher-level .NET concurrency primitives for better readability and robustness. Examples: dotnet/src/webdriver/BiDi/EventDispatcher.cs [154-224] privatesealedclassEventRegistration(JsonTypeInfotypeInfo){privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privatereadonlyList<EventHandler>_handlers=[];privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicJsonTypeInfoTypeInfo{get;}=typeInfo; ...(clipped 61lines) Solution Walkthrough: Before: privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlyobject_drainLock=new();privateList<(longTargetSeq,TaskCompletionSource<bool>Tcs)>?_drainWaiters;publicvoidIncrementProcessed(){varprocessed=Interlocked.Increment(ref_processedSeq);lock(_drainLock){// ... logic to check waiters and complete their tasks ...}}publicTaskDrainAsync(){lock(_drainLock){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)returnTask.CompletedTask;vartcs=newTaskCompletionSource<bool>(...);_drainWaiters??=[];_drainWaiters.Add((target,tcs));// ... double-check logic ...returntcs.Task;}}} After: // Using a SemaphoreSlim to signal completion could simplify the logic.privatesealedclassEventRegistration{privatelong_enqueueSeq;privatelong_processedSeq;privatereadonlySemaphoreSlim_drainSemaphore=new(0);publicvoidIncrementProcessed(){longprocessed=Interlocked.Increment(ref_processedSeq);if(processed==Volatile.Read(ref_enqueueSeq)){// Potentially release waiters_drainSemaphore.Release();}}publicasyncTaskDrainAsync(){vartarget=Volatile.Read(ref_enqueueSeq);if(Volatile.Read(ref_processedSeq)>=target)return;// Wait until the semaphore is released, indicating processing might be complete.await_drainSemaphore.WaitAsync();// Re-check if our target has been met.}} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies the most complex part of the PR—the custom concurrency logic in EventRegistration—and proposes a valid alternative to improve maintainability and robustness, which is a significant quality improvement.	Medium
Update

[Copilot]

Pull request overview

Refactors the .NET BiDi EventDispatcher to make event dispatch and handler unsubscription more reliable under concurrency, specifically ensuring pending events are processed before handlers are removed (addressing the race described in #16574).

Changes:

• Replaces the queued event payload with an EventItem that carries the EventRegistration, and simplifies the background processing loop.
• Introduces per-event-method draining via enqueue/processed sequence tracking so UnsubscribeAsync can wait for already-enqueued events to finish dispatching.
• Hardens handler management with locked add/remove and snapshotting, and improves error isolation between deserialization vs handler invocation.

[Copilot]

Pull request overview

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

[nvborisenko]

Seems overcomplicated, but it resolves simple behaviour:

awaitsubscription.DisposeAsync();// waiting until events in the queue are dispatched

Can be simplified if w3c/webdriver-bidi#1032 is resolved via subscriptionId in all events.