Skip to content

Release JS Blob after BrowserFileStream is disposed. Fixes #35540 #37004

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

Merged
SteveSandersonMS merged 1 commit into from
Oct 4, 2021
Merged

Release JS Blob after BrowserFileStream is disposed. Fixes #35540 #37004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Components/Web/src/Forms/InputFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class InputFile : ComponentBase, IInputFileJsCallbacks, IDisposable
private InputFileJsCallbacksRelay? _jsCallbacksRelay;

[Inject]
private IJSRuntime JSRuntime { get; set; } = default!;
internal IJSRuntime JSRuntime { get; set; } = default!; // Internal for testing

/// <summary>
/// Gets or sets the event callback that will be invoked when the collection of selected files changes.
Expand Down
18 changes: 16 additions & 2 deletions src/Components/Web/src/Forms/InputFile/BrowserFileStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ internal sealed class BrowserFileStream : Stream
private readonly long _maxAllowedSize;
private readonly CancellationTokenSource _openReadStreamCts;
private readonly Task<Stream> OpenReadStreamTask;
private IJSStreamReference? _jsStreamReference;

private bool _isDisposed;
private CancellationTokenSource? _copyFileDataCts;
Expand Down Expand Up @@ -88,13 +89,15 @@ public override async ValueTask<int> ReadAsync(Memory<byte> buffer, Cancellation

private async Task<Stream> OpenReadStreamAsync(CancellationToken cancellationToken)
{
var dataReference = await _jsRuntime.InvokeAsync<IJSStreamReference>(
// This method only gets called once, from the constructor, so we're never overwriting an
// existing _jsStreamReference value
_jsStreamReference = await _jsRuntime.InvokeAsync<IJSStreamReference>(
InputFileInterop.ReadFileData,
cancellationToken,
_inputFileElement,
_file.Id);

return await dataReference.OpenReadStreamAsync(
return await _jsStreamReference.OpenReadStreamAsync(
_maxAllowedSize,
cancellationToken: cancellationToken);
}
Expand All @@ -116,6 +119,17 @@ protected override void Dispose(bool disposing)
_openReadStreamCts.Cancel();
_copyFileDataCts?.Cancel();

// If the browser connection is still live, notify the JS side that it's free to release the Blob
// and reclaim the memory. If the browser connection is already gone, there's no way for the
// notification to get through, but we don't want to fail the .NET-side disposal process for this.
try
{
_ = _jsStreamReference?.DisposeAsync().Preserve();
Copy link
Contributor

@TanayParikh TanayParikh Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Preserve do here?

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use this pattern in a lot of places across ASP.NET Core and Blazor to avoid the analyzer warning https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2012 when doing fire-and-forget on a ValueTask. Unfortunately the explanation given in the CA2012 docs only says:

Ignoring such a ValueTask is likely an indication of a functional bug and may degrade performance

... but doesn't give deeper justification or concrete examples of why fire-and-forget may degrade performance. Knowing the internals of JS interop I don't see any obvious reason why it would be problematic for perf to fire-and-forget a disposeasync call, which we need to do and have a good justification for in this case (see comment in this code).

TBH I'm not really convinced that calling .Preserve() is the right thing to do instead of actually honestly suppressing the warning for real. @pranavkm I think I copied the pattern from you - do you know if .Preserve() actually avoids some real problem, or is this more a case of "Tell me you're suppressing CA2012 without telling me you're suppressing CA2012"?

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think I know the reason why it could be a perf concern. ValueTask instances that wrap values from a reusable pool of task source instances can't return them to the pool until your await is done. So not awaiting could be leaking.

In this case, there's no IValueTaskSource or pooling so the concern is not applicable.

I suppose Preserve might release the original pooled object back to the pool and create a new one that external code can hold onto as long as it likes, so would genuinely be a valid way of not having the issue that CA2012 complains about (even though it's not applicable in our case anyway). But I don't really know for sure if my interpretation is correct, because the Preserve docs don't say anything about the implementation, and the sources are pretty nontrivial to reason about.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway thanks for raising this question @TanayParikh - you forced me to learn more about this!

Copy link
Contributor

@pranavkm pranavkm Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the explanation given in the CA2012 docs only says: ... but doesn't give deeper justification or concrete examples of why fire-and-forget may degrade performance.

@stephentoub was an advocate for that analyzer (and enabled it in our repo). Perhaps some feedback for the description of the analyzer

Copy link
Member

@stephentoub stephentoub Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A ValueTask<T> can wrap a T, a Task<T>, or an IValueTaskSource<T>. Given an arbitrary ValueTask<T> (e.g. a function returns one to you), you should "consume" it once and only once, where "consume" here means awaiting it, calling .Result, calling .GetAwaiter().GetResult(), calling .AsTask(), etc.: basically you need to assume that the moment you've retrieved its result, it's no longer yours, because it may have been wrapping a pooled resource in the form of an IValueTaskSource<T>, and that implementation used its GetResult() implementation as a signal that you're now done with it and the object can be reused. https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/ provides a lot more details. The flip side of that, as is surmised earlier in the comment chain, is that if you get such an IValueTaskSource<T>-backed ValueTask<T> and never await it or otherwise call GetResult(), you're not sending that signal to the implementation, so for example if it's pooled, you just lost that object from the pool. Preserve() returns a new ValueTask<T> that is identical if it's backed by a T or Task<T>, but is the equivalent of doing new ValueTask<T>(oldVT.AsTask()) if it was backed by an IValueTaskSource<T>, and that AsTask() will properly register with the IValueTaskSource<T> to know when it's completed and call GetResult when it is. As a result, Preserve() can be used in a situation where a) you want fire-and-forget and might be dealing with a pooled resource, or b) you want to be able to consume the ValueTask<T> multiple times... both are advanced.

}
catch
{
}

_isDisposed = true;

base.Dispose(disposing);
Expand Down
45 changes: 45 additions & 0 deletions src/Components/Web/test/Forms/BrowserFileTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

using System;
using System.IO;
using Microsoft.JSInterop;
using Moq;
using Xunit;

namespace Microsoft.AspNetCore.Components.Forms
Expand All @@ -29,5 +31,48 @@ public void OpenReadStream_ThrowsIfFileSizeIsLargerThanAllowedSize()
var ex = Assert.Throws<IOException>(() => file.OpenReadStream(80));
Assert.Equal("Supplied file with size 100 bytes exceeds the maximum of 80 bytes.", ex.Message);
}

[Fact]
public void OpenReadStream_ReturnsStreamWhoseDisposalReleasesTheJSObject()
{
// Arrange: JS runtime that always returns a specific mock IJSStreamReference
var jsRuntime = new Mock<IJSRuntime>(MockBehavior.Strict);
var jsStreamReference = new Mock<IJSStreamReference>();
jsRuntime.Setup(x => x.InvokeAsync<IJSStreamReference>(It.IsAny<string>(), It.IsAny<CancellationToken>(), It.IsAny<object[]>()))
.Returns(ValueTask.FromResult(jsStreamReference.Object));

// Arrange: InputFile
var inputFile = new InputFile { JSRuntime = jsRuntime.Object };
var file = new BrowserFile { Owner = inputFile, Size = 5 };
var stream = file.OpenReadStream();

// Assert 1: IJSStreamReference isn't disposed yet
jsStreamReference.Verify(x => x.DisposeAsync(), Times.Never);

// Act
_ = stream.DisposeAsync();

// Assert: IJSStreamReference is disposed now
jsStreamReference.Verify(x => x.DisposeAsync());
}

[Fact]
public async Task OpenReadStream_ReturnsStreamWhoseDisposalReleasesTheJSObject_ToleratesDisposalException()
{
// Arrange: JS runtime that always returns a specific mock IJSStreamReference whose disposal throws
var jsRuntime = new Mock<IJSRuntime>(MockBehavior.Strict);
var jsStreamReference = new Mock<IJSStreamReference>();
jsRuntime.Setup(x => x.InvokeAsync<IJSStreamReference>(It.IsAny<string>(), It.IsAny<CancellationToken>(), It.IsAny<object[]>()))
.Returns(ValueTask.FromResult(jsStreamReference.Object));
jsStreamReference.Setup(x => x.DisposeAsync()).Throws(new InvalidTimeZoneException());

// Arrange: InputFile
var inputFile = new InputFile { JSRuntime = jsRuntime.Object };
var file = new BrowserFile { Owner = inputFile, Size = 5 };
var stream = file.OpenReadStream();

// Act/Assert. Not throwing is success here.
await stream.DisposeAsync();
}
}
}