[dotnet] [bidi] Additional Event streaming (breaking change)#17349
Open
nvborisenko wants to merge 55 commits intoSeleniumHQ:trunkfrom
Open
[dotnet] [bidi] Additional Event streaming (breaking change)#17349nvborisenko wants to merge 55 commits intoSeleniumHQ:trunkfrom
nvborisenko wants to merge 55 commits intoSeleniumHQ:trunkfrom
Conversation
Contributor
Review Summary by Qodo(Agentic_describe updated until commit 42e8ef7)[dotnet] [bidi] Refactor event subscription system with generic types and async streaming support
WalkthroughsDescriptionComprehensive refactoring of the BiDi event subscription system to improve type safety, thread safety, and developer experience: * **Event Subscription Architecture**: Replaced EventDispatcher-based handler mechanism with a new per-event SubscriptionRegistry pattern, enabling more reliable and type-safe event handler management * **Generic Type-Safe API**: Transitioned from non-generic Subscription to generic Subscription<TEventArgs> throughout the API, providing better compile-time type checking and IntelliSense support * **Property-Based Event Sources**: Converted all OnXxxAsync methods to property-based EventSource<TEventArgs> properties with lazy initialization, simplifying the API surface * **Async Enumerable Support**: Introduced EventStream<TEventArgs> and IEventStream<TEventArgs> for consuming events via async LINQ queries, enabling modern async/await patterns * **Channel-Based Event Delivery**: Implemented channel-backed event delivery ensuring sequential handler execution within each subscription * **Centralized Event Descriptors**: Created EventDescriptor<TEventArgs> abstraction with static event definitions (BrowsingContextEvent, NetworkEvent, ScriptEvent, etc.) for consistent event metadata management * **Context-Filtered Events**: Added ContextEventSource<TEventArgs> wrapper for automatic context-specific event filtering in browsing context APIs * **Comprehensive Test Coverage**: Updated all existing tests and added new tests demonstrating single/multiple event subscriptions, async stream consumption, and LINQ-style event processing * **Module Updates**: Refactored all modules (BrowsingContext, Network, Script, Log, Input, Speculation) to use the new event source pattern with thread-safe lazy initialization File Changes1. dotnet/src/webdriver/BiDi/EventDispatcher.cs
|
Contributor
Code Review by Qodo
Context used 1. Added methods to IBiDi
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the .NET BiDi event subscription pipeline to support strongly-typed subscriptions and optional event streaming, replacing the prior centralized dispatcher approach with per-event subscription registries.
Changes:
- Introduces
Subscription<TEventArgs>backed byChannel<T>and implementsIAsyncEnumerable<TEventArgs>for streaming consumption. - Replaces
EventDispatcherwith a per-eventSubscriptionRegistryinsideBroker, and propagates terminal exceptions to active subscriptions on shutdown. - Updates BiDi module and
BrowsingContextevent APIs to returnSubscription<TEventArgs>and adds handlerless overloads.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/webdriver/BiDi/Subscription.cs | Adds generic, channel-backed subscriptions with streaming support and sequential handler draining. |
| dotnet/src/webdriver/BiDi/Broker.cs | Replaces dispatcher with per-event registries and direct delivery/completion to subscriptions. |
| dotnet/src/webdriver/BiDi/Module.cs | Updates subscribe helper to return Subscription<TEventArgs> and accept ValueTask handlers. |
| dotnet/src/webdriver/BiDi/EventDispatcher.cs | Removes the previous centralized queued dispatcher implementation. |
| dotnet/src/webdriver/BiDi/Speculation/SpeculationModule.cs | Updates speculation event subscription APIs to typed subscriptions + handlerless overload. |
| dotnet/src/webdriver/BiDi/Speculation/ISpeculationModule.cs | Updates public speculation interface signatures to typed subscriptions and adds handlerless overload. |
| dotnet/src/webdriver/BiDi/Script/ScriptModule.cs | Updates script event subscription APIs to typed subscriptions + handlerless overload. |
| dotnet/src/webdriver/BiDi/Script/IScriptModule.cs | Updates public script interface signatures to typed subscriptions and adds handlerless overloads. |
| dotnet/src/webdriver/BiDi/Network/NetworkModule.cs | Updates network event subscription APIs to typed subscriptions + handlerless overloads. |
| dotnet/src/webdriver/BiDi/Network/INetworkModule.cs | Updates public network interface signatures to typed subscriptions and adds handlerless overloads. |
| dotnet/src/webdriver/BiDi/Log/LogModule.cs | Updates log event subscription APIs to typed subscriptions + handlerless overload. |
| dotnet/src/webdriver/BiDi/Log/ILogModule.cs | Updates public log interface signatures to typed subscriptions and adds handlerless overload. |
| dotnet/src/webdriver/BiDi/Input/InputModule.cs | Updates input event subscription APIs to typed subscriptions + handlerless overload. |
| dotnet/src/webdriver/BiDi/Input/IInputModule.cs | Updates public input interface signatures to typed subscriptions and adds handlerless overload. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs | Updates browsing-context module event subscription APIs to typed subscriptions + handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/IBrowsingContextModule.cs | Updates public browsing-context interface signatures to typed subscriptions and adds handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs | Updates BrowsingContext facade methods to use typed subscriptions and adds handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextNetworkModule.cs | Updates context-scoped network event subscription wrappers to typed subscriptions + handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/IBrowsingContextNetworkModule.cs | Updates public context-scoped network interface signatures to typed subscriptions and adds handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextLogModule.cs | Updates context-scoped log event subscription wrappers to typed subscriptions + handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/IBrowsingContextLogModule.cs | Updates public context-scoped log interface signatures to typed subscriptions and adds handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextInputModule.cs | Updates context-scoped input event subscription wrappers to typed subscriptions + handlerless overloads. |
| dotnet/src/webdriver/BiDi/BrowsingContext/IBrowsingContextInputModule.cs | Updates public context-scoped input interface signatures to typed subscriptions and adds handlerless overloads. |
Member
Author
|
Need more design decisions.. |
Member
Author
|
Perfect, I understood how to design events. Before: // Push
await using var sub = await network.OnBeforeRequestSentAsync(e => Console.WriteLine(e));After: // Push
await using var sub = await network.BeforeRequestSent.OnAsync(e => Console.WriteLine(e));
// Pull
await using var sub = await network.BeforeRequestSent.SubscribeAsync();
await foreach (var args in sub) { ... }2 separate clean concepts. Why this way...
Thus example of usage becomes very simple (purely native C#): await using var sub = await network.BeforeRequestSent.SubscribeAsync();
await context.NavigateAsync("example.com");
await sub.Where(req => req.Url.Contains("/api")).FirstAsync();Or if somebody wants to develop extension method: var request = await network.BeforeRequestSent
.During(() => context.NavigateAsync("example.com"))
.Where(req => req.Url.Contains("/api")).FirstAsync() |
Member
Author
CheckpointRedesigns BiDi event subscription with two clean patterns — push and pull — exposed through properties on each module. // Push — subscribe with a handler
await using var sub = await context.Network.BeforeRequestSent.SubscribeAsync(async e =>
{
await bidi.Network.ContinueRequestAsync(e.Request.Request);
});
// Pull — consume as async stream + LINQ
await using var stream = await bidi.Log.EntryAdded.ReadAllAsync();
var error = await stream.Where(e => e.Level == Level.Error).FirstAsync(cts.Token);
// Filter before subscribing
await using var sub = await bidi.Network.ResponseCompleted
.Where(e => e.Response.Url.Contains("/api"))
.SubscribeAsync(e => Console.WriteLine(e.Response.Status));
// Context-scoped — auto-filters to this tab
await using var sub = await context.Log.EntryAdded.SubscribeAsync(e =>
Console.WriteLine(e.Text));
// Multiple events in a single wire subscription
await using var sub = await bidi.SubscribeAsync(
[NetworkEvent.ResponseStarted, NetworkEvent.ResponseCompleted],
(EventArgs e) =>
{
switch (e)
{
case ResponseStartedEventArgs started:
Console.WriteLine($"Started: {started.Request.Url}");
break;
case ResponseCompletedEventArgs completed:
Console.WriteLine($"Completed: {completed.Response.Status}");
break;
}
}); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
dotnet/src/webdriver/BiDi/Subscription.cs:113
SubscriptionOptionsis a public type and the previousTimeoutoption has been removed, which is a breaking API change (and conflicts with the PR description claiming non-breaking changes). If this is intended, it should be handled via deprecation (keep the old member, mark[Obsolete], and map it to the new behavior) or documented as a breaking change.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (8)
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:123
- This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. It should be disposed (e.g.,await using) after the test to ensure the wire subscription and dispatch task don’t leak into later tests.
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:175 - This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. Dispose it (e.g.,await using) to avoid leaking the subscription/background task across tests.
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:242 - This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. Dispose it to avoid leaving a subscription active beyond the test, which can lead to cross-test interference.
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:260 - This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. Dispose it (e.g.,await using) after the test to avoid leaking the subscription and its background processing task.
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:145 - This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. Dispose it (e.g., viaawait using) so the intercept handler doesn’t remain active beyond this test.
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:203 - This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. It should be disposed to avoid leaving an auth intercept handler active after the test completes.
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:223 - This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. Dispose it (e.g.,await using) to prevent the subscription/background task from leaking into later tests.
dotnet/test/webdriver/BiDi/Network/NetworkTests.cs:100 - This
SubscribeAsynccall returns anIAsyncDisposablesubscription but the result is ignored. With the new subscription implementation, it should be disposed to avoid leaking a live intercept handler/background task beyond the test. Capture it inawait using var subscription = await ...and keep it alive until after navigation/assertions.
Contributor
|
Persistent review updated to latest commit 42e8ef7 |
nvborisenko
changed the title
Contributor
|
@nvborisenko, in the latest checkpoint example there are: bidi.Log.EntryAdded...
context.Log.EntryAdded...Are |
Member
Author
|
bidi.Log.EntryAdded.... // getting all events for browser
context.Log.EntryAdded..... // only for this tabContext-aware scope is additional abstraction over specification. Raw modeling: bidi.BrowsingContext.NavigateAsync(BrowsingContext ctx, "https://example.com");We support simpler built-in variant: ctx.NavigateAsync("https://example.com"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finally the events model is more cleaner. Each
Subscriptionis backed byChannel, meaning all handlers want to be executed sequentially.New API opens doors for consuming events via async LINQ - big features with minimal maintenance.
🔗 Related Issues
Contributes to #16095
💥 What does this PR do?
This pull request refactors and extends the BiDi event subscription and dispatching system to improve flexibility, reliability, and resource management. The most notable changes are the introduction of a more generic event subscription API in
BiDi, significant reworking of event handling inBroker, and ensuring proper disposal of event subscriptions.Event Subscription and Dispatching Improvements:
SubscribeAsyncandReadAllAsyncmethods to theBiDiclass, supporting both single and multiple event descriptors, and both synchronous and asynchronous handlers. This provides a more flexible and type-safe way for clients to subscribe to and consume events.Brokerto use a publicEventDispatcherinstance, initialized with delegates for subscribing and unsubscribing via the session, and removed the previous internal event subscription logic. This centralizes and simplifies event management. [1] [2]Resource Management and Disposal:
BiDiandBrokerto ensure event subscriptions are properly completed and disposed of before shutting down the transport and processing tasks, preventing resource leaks and ensuring clean shutdowns. [1] [2] [3]Event Handling Logic:
Brokerto delegate deserialization and dispatching to theEventDispatcher, removing the need for event metadata management and manual event argument construction. [1] [2]These changes provide a more robust, extensible, and maintainable event system for BiDi clients.
🔄 Types of changes