-
Notifications
You must be signed in to change notification settings - Fork 10.3k
.NET to JS Streaming Interop #34817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
.NET to JS Streaming Interop #34817
Conversation
@@ -20,6 +21,7 @@ internal class RemoteJSRuntime : JSRuntime | |||
private readonly CircuitOptions _options; | |||
private readonly ILogger<RemoteJSRuntime> _logger; | |||
private CircuitClientProxy _clientProxy; | |||
private readonly ConcurrentDictionary<long, DotNetStreamReference> _pendingDotNetToJSStreams = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the concurrent dictionary here? Shouldn't this already be scoped to the circuit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cancellationToken.Register
which is used to evict streams which aren't read from after the timeout, does not capture the Synchronization context.. Hence, without a ConcurrentDictionary
we could run into potential issues when the cancellation token expires (and attempts to evict the stream) at the exact time we get a call into TryClaimPendingStreamForSending
to evict the stream for consumption.
Code looks good, waiting for the E2E tests |
Thank you for your API proposal. I'm removing the |
This PR should be good to go now using the |
I might be missing something, but I don't see the E2E tests, can you point me to them? |
Hmm, they should be there under |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Usage:
Based on @SteveSandersonMS's proof of concept: #32848 (which also has a great writeup on how the pending stream logic works for Blazor Server).
Fixes: #30289
API Review: #35009