Release JS Blob after BrowserFileStream is disposed. Fixes #35540

[SteveSandersonMS]

Fixes #35540, which was reported by several customers and ultimately could lead to the browser running out of memory if uploading a lot of large files.

The underlying issue here is that:

• If you're working with IJSStreamReference directly through manual interop code, then as well as disposing any Stream instances you get by calling jsStreamReference.OpenReadStreamAsync, it's also your responsibility to dispose the IJSStreamReference itself. That's the point at which the JS-side Blob gets released, because until then, you could keep on calling OpenReadStreamAsync more times and continue to read the blob data.
• However, if you're using an InputFile and you call OpenReadStream on one of the IBrowserFile instances it gives you, then you get a Stream, but you never get to see the underlying IJSStreamReference that we're using internally. In fact, our implementation BrowserFileStream doesn't even keep a reference to the IJSStreamReference - it just transiently calls the OpenReadStreamAsync method and discards the IJSStreamReference. As such, even when you manually dispose the BrowserFileStream it doesn't dispose the underlying IJSStreamReference, and hence the browser never releases the Blob.

The fix then is pretty simple. BrowserFileStream should keep track of the IJSStreamReference that it's using for transport, and it should dispose it when itself is disposed.

One further caveat is that with Blazor Server, there's no guarantee the connection to the browser still exists at this point. And we don't want the failure to dispose the IJSStreamReference to cause the .NET circuit disposal process itself to fail. So, when we dispose the JS stream reference we just do it on a "best effort, ignore failures" basis. Strictly speaking you could still leak memory if the disposal happens while the circuit is disconnected and then the circuit reconnects later, and this happens over and over. However that's extremely unlikely, and really is a broader underlying issue with IJSObjectReference - that if you try to dispose them while the circuit is disconnected, the disposal doesn't happen and nothing will automatically retry it later. If we're speaking super strictly, then we could say we're leaving the application developer to track and replay all JS interop calls that happen while disconnected if they really want to (since the semantics would always be scenario-specific), but it's incredibly rare for anyone to want to do that.

Testing

It's not really sensible to try to E2E test this, because external systems have no visibility into the internal state of JS object reference caching. So what I have done is:

• Manually verified using the JS debugger that it does in fact now release the cachedJSObjectsById entries when the BrowserFileStream is disposed
• Added a unit test showing the .NET side code does ask for the JS object to be disposed. It's a bit unusual to unit test this, and I don't really like this kind of mocking much, but it's very localized to this one scenario so I don't think it's going to cause any real impact on long-term maintainability. I wouldn't do this if we have many more complex behaviors to test in this area.

[SteveSandersonMS]

Looking for feedback from @TanayParikh on this solution.

[TanayParikh]

Looks great, just a question for my understanding.

[TanayParikh]

What does Preserve do here?

[SteveSandersonMS]

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"?

[SteveSandersonMS]

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.

[SteveSandersonMS]

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

[pranavkm]

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

[stephentoub]

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.

[SteveSandersonMS]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1303239855