Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle Leak of CancellationTokenSource in RemoteJSRuntime class #59263

Open
Anchels opened this issue Dec 2, 2024 · 4 comments · May be fixed by #59652
Open

Handle Leak of CancellationTokenSource in RemoteJSRuntime class #59263

Anchels opened this issue Dec 2, 2024 · 4 comments · May be fixed by #59652
Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-jsinterop This issue is related to JSInterop in Blazor help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone
Backlog

Comments

@Anchels
Copy link

Anchels commented Dec 2, 2024

I analyzed the ASP .NET Core code using the Svace static analyzer. It has found a HANDLE_LEAK category error with the following message:

new CancellationTokenSource(TimeSpan.FromSeconds(10)) is not disposed at the end of the function

in method TransmitStreamAsync(). Here's the source code:

aspnetcore/src/Components/Server/src/Circuits/RemoteJSRuntime.cs

Lines 153 to 174 in d088530

protected override async Task TransmitStreamAsync(long streamId, DotNetStreamReference dotNetStreamReference)
{
if (!_pendingDotNetToJSStreams.TryAdd(streamId, dotNetStreamReference))
{
throw new ArgumentException($"The stream {streamId} is already pending.");
}
// SignalR only supports streaming being initiated from the JS side, so we have to ask it to
// start the stream. We'll give it a maximum of 10 seconds to do so, after which we give up
// and discard it.
var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(10)).Token;
cancellationToken.Register(() =>
{
// If by now the stream hasn't been claimed for sending, stop tracking it
if (_pendingDotNetToJSStreams.TryRemove(streamId, out var timedOutStream) && !timedOutStream.LeaveOpen)
{
timedOutStream.Stream.Dispose();
}
});
await _clientProxy.SendAsync("JS.BeginTransmitStream", streamId);
}

An instance of the CancellationTokenSource class is created and can be disposed of.

When using CancellationTokenSource and CancellationToken objects, it is necessary to correctly organize the process of releasing them from memory.

What about rewriting the declaration of the cancellationToken by applying the using statement to the CancellationTokenSource object as follows:

 using var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));

 var cancellationToken = cancellationTokenSource.Token;

Found by Linux Verification Center (linuxtesting.org) with SVACE.
Reporter: Aleksey Kolosov ([email protected]).
Organization: [email protected]
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Dec 2, 2024
@martincostello martincostello added area-blazor Includes: Blazor, Razor Components and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Dec 2, 2024
@mkArtakMSFT mkArtakMSFT added feature-blazor-jsinterop This issue is related to JSInterop in Blazor help wanted Up for grabs. We would accept a PR to help resolve this issue labels Dec 2, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Looks like this issue has been identified as a candidate for community contribution. If you're considering sending a PR for this issue, look for the Summary Comment link in the issue description. That comment has been left by an engineer on our team to help you get started with handling this issue. You can learn more about our Help Wanted process here

@mkArtakMSFT
Copy link
Member

Thanks for reporting this. Will you be interested in sending a PR with the fix? We'd be happy to take it.

@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Dec 2, 2024
@BrennanConroy
Copy link
Member

What about rewriting the declaration of the cancellationToken by applying the using statement to the CancellationTokenSource object as follows:

using var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));

var cancellationToken = cancellationTokenSource.Token;

That would be an incorrect fix. The CancellationTokenSource is intentionally supposed to exist until TryClaimPendingStreamForSending(...) is called (or the timeout occurs). Really you would likely store the CTS in _pendingDotNetToJSStreams and dispose of it when removing the stream.

It's not really a leak either in this case. CancellationTokenSource with a timer will clean up itself and use finalizers for the internal handles. That doesn't mean it's not a good idea to still clean it up, but it does mean it does not leak.

@Anchels
Copy link
Author

Anchels commented Dec 4, 2024

That would be an incorrect fix. The CancellationTokenSource is intentionally supposed to exist until TryClaimPendingStreamForSending(...) is called (or the timeout occurs). Really you would likely store the CTS in _pendingDotNetToJSStreams and dispose of it when removing the stream.

It's not really a leak either in this case. CancellationTokenSource with a timer will clean up itself and use finalizers for the internal handles. That doesn't mean it's not a good idea to still clean it up, but it does mean it does not leak.

Thank you for your answer! Do I understand correctly that I should close the issue, since it has been resolved?

@clegoz clegoz linked a pull request Dec 28, 2024 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-jsinterop This issue is related to JSInterop in Blazor help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Dispose CTS when the Stream is claimed or timeout occurs clegoz/aspnetcore
4 participants
@martincostello @BrennanConroy @mkArtakMSFT @Anchels