Uh oh!
There was an error while loading. Please reload this page.
[dotnet] [bidi] Split event stream backed subscription and enumeration - #17705
Conversation
PR Summary by Qodo[dotnet][bidi] Use IAsyncEnumerable for event streams with implicit unsubscribe Description
Diagram
High-Level Assessment
Files changed (9) |
Code Review by Qodo
Context used✅ Compliance rules (platform): 15 rules 1. No unsubscribe on enumeration end |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
I really like this simplification too! It makes things more understandable for consumers by using only a well-known interface and reduces casting by avoiding the The main drawback I see is that if the Another small drawback is that if enumeration is attempted after the first is disposed (i.e. Thanks for exploring it @nvborisenko, I really like it! |
nvborisenko
commented
Jun 22, 2026
This is the same issue as before. All data, got from remote end, is buffered. This is "intentional". Users might forgot to call
Good call, we should throw with self-explainable exception. Thanks for your review, appreciate earlier feedback. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Now the following case doesn't work, event collection pattern: varstream=awaitbidi...StreamAsync();// await stream.DisposeAsync();stream.ToListAsync();// hang because the stream never completesMight be fixed via: |
kevinoid
commented
Jun 23, 2026
Good catch! I would have thought that enumerating a stream after disposing it would throw. Having a way to unsubscribe from future events while still enumerating buffered events seems like a great idea. Relatedly: It would be great to have a way to enumerate only buffered events (i.e. via |
nvborisenko
commented
Jun 23, 2026
This is good observation. Enumeration after disposal does look strange. At the same time, honestly, I prefer explicit
Current state in my head: awaitusingvarstream=awaitbidi....StreamAsync();// disposable, because remote-end subscriptionDoSomething();awaitstream.CompleteAsync();// signal to stop receiving events (should we unsubscribe on remote-end?)stream.Reader// -> ChannelReader<>, new api surface, providing some great capabilitiesIf we will introduce |
kevinoid
commented
Jun 23, 2026
I agree; the redundancy would be confusing and having explicit What if |
nvborisenko
commented
Jun 23, 2026
True, this is exactly what is in my head. Previously I thought throght how to make OK, sold! Next step is to understand what we are loosing and what we are bringing for end users. |
nvborisenko
commented
Jun 23, 2026
Concern: exposing |
nvborisenko
commented
Jun 24, 2026
I don't see use cases why If we really want to bring |
kevinoid
commented
Jun 25, 2026
I agree; it's a trade-off that's important to weigh and understand. The big features that
Features that might be useful for other users:
The big ones for me are checking for buffered events and cancelling individual reads. Providing an interface with just these methods would be sufficient for most of the use cases I see and would keep the API a bit smaller and easier to change in the future, with the slight understandability cost of using a custom interface instead of the well-known |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Code review by qodo was updated up to the latest commit 8cd2fee |
nvborisenko
commented
Jun 26, 2026
@kevinoid , I am looking at: awaitusingvarstream=awaitbidi....StreamAsync();// disposable resource, starts bufferingvarenumerable=awaitstream.ReadAllAsync();// IAsyncEnumerableDoSomething();enumerable.ToList();// still hang, not yet completed
Anyway puzzle is incomplete:
I don't see complete picture how it should work. |
kevinoid
commented
Jun 26, 2026
I like separating I'm disappointed that it won't have either of the features I mentioned previously (a way to read buffered items without waiting ( Dispose vs Complete is tricky. It feels unsafe to continue enumerating after disposal, but as long as it's clearly documented that stream disposal doesn't invalidate the read enumerable, I think it would be ok. I'd probably lean toward separate methods with Complete unsubscribing and Dispose both unsubscribing and releasing all buffered events and invalidating readers, but I see it as subjective and wouldn't object to either approach. Calling |
@RenderMichael@YevgeniyShunevych your opinion is highly appreciated here. Recently we added streaming of events. This feature should open the world for This is not easy. please be careful. UPD: seems allowing only one single enumerator might resolve all concerns. Enumerator finishes - we are stopping buffering following events (unsubscribe on remote-end). This contract also implicitly resolves the issue with memory consumption: user forgot to unsubscribe/dispose. But how to require user to consume buffered events?.. it is his responsibility? - yes. UPD: if enumerator finish means to unssubcribe, then it should mean starting of enumeration is to subscribe. UPD: personally I wish to create extension method like |
Uh oh!
There was an error while loading. Please reload this page.
Code review by qodo was updated up to the latest commit 616a694 |
Remove now-unnecessary casts and call `.ReadAllAsync()` due to API changes from <SeleniumHQ/selenium#17705>. --- updated-dependencies: - dependency-name: Selenium.WebDriver dependency-version: 4.46.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Remove now-unnecessary casts and call `.ReadAllAsync()` due to API changes from <SeleniumHQ/selenium#17705>. --- updated-dependencies: - dependency-name: Selenium.WebDriver dependency-version: 4.46.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
StreamAsync()is disposable only, meaning explicit subscription lifecycle management. To get enumeration usestream.ReadAllAsync().🔗 Related Issues
Implicitly resolves#17696
💥 What does this PR do?
This pull request refactors the event stream handling in the BiDi WebDriver .NET implementation to simplify cancellation token management. It introduces a new
ReadAllAsyncmethod for consuming event streams, updates the interface and all usage sites. The changes also include corresponding updates to tests to reflect the new usage pattern.Event stream API changes:
IAsyncEnumerable<TEventArgs>inIEventStream<TEventArgs>with a newReadAllAsyncmethod, and removed theGetAsyncEnumeratormethod fromEventStream<TEventArgs>. This simplifies cancellation token handling and standardizes stream consumption. [1][2][3]ReadAllAsync, ensuring cancellation tokens are passed explicitly where needed. [1][2][3][4][5][6][7][8]Code cleanup and improvements:
_cancellationTokenfield and related logic fromEventStream<TEventArgs>, streamlining resource management.System.Runtime.CompilerServicesimport to support[EnumeratorCancellation]attribute usage.🔧 Implementation Notes
Good to simplify user-facing API.
🤖 AI assistance
💡 Additional Considerations
Low level API, amazing simplification.
🔄 Types of changes