File uploads (writing to memory/Autofac content)

[guardrex]

Fixes #33272

Notes ...

• The versioning assumes that the problems associated with multiple file uploads (and avoiding writing to a MemoryStream/byte array) and Autofac will be resolved for >=9.0.
• The <9.0 text won't dissuade writing to a MemoryStream/byte array any longer. However, I'm leaving the bits about how it can degrade performance and present a possible security risk.
• 🤔 I think the "security risk" that we were generically mentioning before is DoS, so I'm stating that explicitly now on this PR. Let me know if that's an incorrect guess. 👂
• The sample app changes were made in the Blazor samples repo for Blazor Server and Blazor Web App samples. Here's an example 👉 https://github.com/dotnet/blazor-samples/blob/main/8.0/BlazorSample_BlazorWebApp/Components/Pages/FileUpload2.razor#L80-L85

---

Internal previews

📄 File	🔗 Preview link
aspnetcore/blazor/file-uploads.md	ASP.NET Core Blazor file uploads

[MackinnonBuck]

Thanks @guardrex!

The content you wrote here looks accurate, although I imagine that customers will likely arrive at these docs after experiencing the TimeoutException. That is, I'm not sure they'll have the patience to grasp all the intricacies of what scenarios are expected to work determine whether their scenario meets those requirements, and then connect that back to the TimeoutException they're experiencing.

Furthermore, these intricacies are due to a bug in the framework (dotnet/aspnetcore#47301), and will probably disappear when that bug gets fixed.

I wonder if we could keep the docs simpler, without explaining the details of the bug, and instead add a "Troubleshooting" section where we:

• List the TimeoutException error message (so it can be discovered via web search)
• Explain the possible causes:
  1. Autofac
  2. Not reading the stream to completion
  3. On Blazor server, calling OpenReadStream on multiple files before reading them to completion (Multiple files upload hangs the process in a blazor server app aspnetcore#47301).
• Explain workarounds for each cause

Also: I wrote an example below for another workaround we can suggest other than reading the stream into memory. I would recommend we document that instead, so our guidance can be consistent that reading the stream into memory should be avoided.

The workaround is to define a custom stream type that lazily calls OpenReadStream just before the first bytes of the stream are requested. In other words, the stream won't start getting transmitted from the browser to the server until the stream starts to get read in .NET. Here's the example:

internal sealed class LazyBrowserFileStream : Stream
{
    private readonly IBrowserFile _file;
    private readonly int _maxAllowedSize;

    private Stream? _underlyingStream;
    private bool _isDisposed;

    public override bool CanRead => true;

    public override bool CanSeek => false;

    public override bool CanWrite => false;

    public override long Length => _file.Size;

    public override long Position
    {
        get => _underlyingStream?.Position ?? 0;
        set => throw new NotSupportedException();
    }

    public LazyBrowserFileStream(IBrowserFile file, int maxAllowedSize)
    {
        _file = file;
        _maxAllowedSize = maxAllowedSize;
    }

    public override void Flush()
    {
        _underlyingStream?.Flush();
    }

    public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
    {
        EnsureStreamIsOpen();
        return _underlyingStream.ReadAsync(buffer, offset, count, cancellationToken);
    }

    public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
    {
        EnsureStreamIsOpen();
        return _underlyingStream.ReadAsync(buffer, cancellationToken);
    }

    [MemberNotNull(nameof(_underlyingStream))]
    private void EnsureStreamIsOpen()
    {
        _underlyingStream ??= _file.OpenReadStream(_maxAllowedSize);
    }

    protected override void Dispose(bool disposing)
    {
        if (_isDisposed)
        {
            return;
        }

        _underlyingStream?.Dispose();
        _isDisposed = true;

        base.Dispose(disposing);
    }

    public override int Read(byte[] buffer, int offset, int count)
        => throw new NotSupportedException();

    public override long Seek(long offset, SeekOrigin origin)
        => throw new NotSupportedException();

    public override void SetLength(long value)
        => throw new NotSupportedException(); 

    public override void Write(byte[] buffer, int offset, int count)
        => throw new NotSupportedException();
}

And here's an example using it in a MultipartFormDataContent:

using var content = new MultipartFormDataContent();

foreach (var file in e.GetMultipleFiles())
{
    var stream = new LazyBrowserFileStream(file, maxAllowedSize: 15_000_000);
    var fileContent = new StreamContent(stream);
    fileContent.Headers.ContentType = new MediaTypeHeaderValue(file.ContentType);

    content.Add(fileContent);
}

So, if the cause of the TimeoutException is item (iii.) listed above, our suggested workaround options would be:

1. After calling OpenReadStream() for a file, read the stream fully to completion before calling OpenReadStream() on the next file
2. If (1.) isn't possible (e.g., when using MultipartFormDataContent), you could create a custom stream wrapper (shown above as LazyBrowserFileStream).

Let me know what you think 🙂

[guardrex]

Are you planning on back-porting the fix to earlier releases? If not, this doesn't seem like it's "troubleshooting" guidance in the <9.0 era.

Same deal with the Autofac coverage. AFAICT, it's now the base coverage for <9.0, so it doesn't seem like it would be troubleshooting after the fact.

The problem with troubleshooting guidance is that it's too easily overlooked. Devs will 💥😢, and then we 🙏 to the server gods that they find their way back to the article for help. Why not just cover the LazyBrowserFileStream approach as the base case for all <9.0 (server-side) implementations?

On Blazor server, calling OpenReadStream on multiple files before reading them to completion

I need to look at this caveat again tomorrow. I'm aware that this doesn't apply to client-side. However, the section where the code is implemented for FileUpload2 is dedicated to server-side scenarios. I'll study it further in the morning.

[MackinnonBuck]

The problem with troubleshooting guidance is that it's too easily overlooked. Devs will 💥😢, and then we 🙏 to the server gods that they find their way back to the article for help.

I definitely agree that a troubleshooting section can be easily overlooked, but isn't that kind of by design?

I can only speak for myself, but my goal when reading documentation is to spend as little time as possible doing so. Ideally, the APIs I'm using are self-evident and don't require reading docs at all, but if I do have a question, I'll skim the docs only until I've grasped just enough information to unblock me. Rarely do I read a page of docs to completion before starting to use any given feature. And if the docs go into extreme detail, it hurts my ability to skim because there's more to sift through, and I'm even less likely to read them to completion because most of the information there doesn't apply to me.

Given that workflow, I'd rather there be a troubleshooting section so that I can ctrl+F the exception I'm getting (or find it via web search) to see if I can find a workaround. Unfortunately, I don't think I'd have the patience to do a thorough pass of the docs to fully understand the intricacies and limitations of the feature, then use that information to deduce the root cause of my problem.

Obviously this is just my personal preference and I'm not sure how reflective it is of the typical reader, so it should be taken with a grain of salt. I also don't have nearly as much experience writing docs, so I'm totally willing to be overridden on this!

@danroth27, do you have any thoughts?

Why not just cover the LazyBrowserFileStream approach as the base case for all <9.0 (server-side) implementations?

The challenge is that in most cases, this approach isn't necessary. Only in a small number of scenarios do the right conditions get hit to cause the problem to occur:

1. Using server interactivity, and
2. Supporting multiple file upload, and
3. Calling OpenReadStream multiple times in a row without fully consuming the stream first, and
4. Not controlling the code that consumes the stream (e.g., the MultipartFormDataContent case)

If our blanket recommendation is to always wrap the returned stream in a custom stream implementation, then it's making the common scenario much more complicated. From that perspective, the workaround does seem more like a "troubleshooting" step to me.

[guardrex]

Two of those are built into the current coverage of the dedicated Upload files to a server with server-side rendering section ...

• Using server interactivity
• Supporting multiple file upload

... so it seems that even if the opening remarks don't cover the LazyBrowserFileStream approach use and we stick with the current guidance (+ a troubleshooting section), that section has to show the LazyBrowserFileStream approach.

I'll look at this further in the morning and revise the PR.

[guardrex]

@MackinnonBuck ...

Try it now with the LazyBrowserFileStream approach only in the section that applies to the scenario (server-side + multiple files).

I added a Troubleshoot section.

The sample code was updated on dotnet/blazor-samples#330.

To inform devs that in the single-file-upload case or in the WASM component in a BWA case they don't need to use the LazyBrowserFileStream approach, I left the bits in there for <9.0 that explain how to back it out. As of 9.0, it will ✨ automagically disappear ✨ because it will be removed from the 9.0 sample app when I put it up later and versioning in the article (in place now) will take care of the rest.

[MackinnonBuck]

I like the changes! Just did a more thorough pass and have a few notes/suggestions.

[MackinnonBuck]

LGTM!