-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Adds Disposal Logic for IJSStreamReference in PullFromJSDataStream #64426
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for your PR, @@eugeneogongo. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
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.
Pull Request Overview
This PR adds disposal logic for IJSStreamReference in PullFromJSDataStream and introduces unit tests to verify the disposal behavior. The changes refactor the stream to use a CancellationTokenSource instead of a CancellationToken directly, and implement both synchronous and asynchronous disposal patterns.
- Replaces direct
CancellationTokenusage withCancellationTokenSourcefor better lifecycle management - Adds
Dispose(bool disposing)andDisposeAsync()override methods to properly clean up resources - Introduces unit tests to verify that disposal calls propagate to the underlying
IJSStreamReference
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Components/Shared/src/PullFromJSDataStream.cs | Implements disposal pattern with CancellationTokenSource management and IJSStreamReference cleanup |
| src/Components/WebAssembly/WebAssembly/test/PullFromJSDataStreamTest.cs | Adds unit tests for synchronous and asynchronous disposal behavior, removes unused import |
| _streamCts?.Cancel(); | ||
| try | ||
| { | ||
| _ = _jsStreamReference?.DisposeAsync().Preserve(); |
Copilot
AI
Nov 18, 2025
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.
The Preserve() extension method does not exist in the codebase. This will cause a compilation error.
To properly handle the fire-and-forget disposal in a synchronous context, you should use .AsTask() or simply discard the ValueTask without calling any method on it. Consider one of these alternatives:
// Option 1: Just discard the ValueTask (allows it to be collected)
_ = _jsStreamReference?.DisposeAsync();or if you need to ensure the task runs:
// Option 2: Convert to Task (ensures the task runs)
_ = _jsStreamReference?.DisposeAsync().AsTask();| _ = _jsStreamReference?.DisposeAsync().Preserve(); | |
| _ = _jsStreamReference?.DisposeAsync(); |
| return; | ||
| } | ||
|
|
||
| _streamCts?.Cancel(); |
Copilot
AI
Nov 18, 2025
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.
The _streamCts CancellationTokenSource is never disposed, which can lead to resource leaks. You should dispose it in both Dispose and DisposeAsync methods.
Add disposal after cancellation:
_streamCts?.Cancel();
_streamCts?.Dispose();
Adds Disposal Logic for IJSStreamReference in PullFromJSDataStream
Refactor PullFromJSDataStream disposal logic and add Dispose unit tests
Description
{Detail}
Fixes #62140 (in this specific format)