dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.15k stars 9.92k forks source link

Add additional logic in the InputFile component to split the upload manifest into multiple messages if it exceeds the buffer size for a single SingalR message #42993

Open Olby2000 opened 2 years ago

Olby2000 commented 2 years ago

Is there an existing issue for this?

Describe the bug

There seems to be a correlation between InputFile upload limit and SignalR MaximumReceiveMessageSize property in Blazor Server Side projects. This has not been described in the documentation for Blazor file uploads: https://docs.microsoft.com/en-us/aspnet/core/blazor/file-uploads

Run the code and select multiple files that are somewhere between 64-200 MB in total. On my system I have not found reason as to why it would sometimes work with up to 200 MB but in some cases it will disconnect with just over 64MB of files in total. Without changing the MaximumReceiveMessageSize property the connection to the server will be lost and on OnChange method is never called whenever it reconnects. The console has a lost connection message but that's it, nothing in the VS runtime logs.

Error: Connection disconnected with error 'Error: Server returned an error on close: Connection closed with an error.'. e.log @ blazor.server.js:1

However if you change the MaximumReceiveMessageSize property from 32KB to something higher it will start to accept files with a total size that goes beyond 200 MB (or whatever the limitation is for a given test).

If this is not a bug then there should some documentation or an explanation as to what is the relationship between total up-loadable file size limit and SignalR message buffer size. Is all the file info being communicated to the server in one large message and if it hits the default 32KB limit it fails?

The two issues from 2019 touch on the subject but do not have the reasoning on how the buffer size is related to file count or size: https://github.com/dotnet/aspnetcore/issues/11643 https://github.com/dotnet/aspnetcore/issues/12203

Expected Behavior

The expected behaviour is to control file count and size limits with the two parameters below. If SignalR buffer size is involved it should be clearly detailed that it also plays a role in file upload scenarios.

InputFileChangeEventArgs.GetMultipleFiles(maximumFileCount);
IBrowserFile.OpenReadStream(maxAllowedSize);

Steps To Reproduce

@page "/"

<h1>File Upload</h1>

<InputFile OnChange="SelectFiles" multiple accept=".pdf" />

@if (Files != null)
{
    foreach(var file in Files)
    {
        <p>@file</p>
    }   
}

@code {
    const int MaxUploadFiles = 1000;
    private IEnumerable<string> Files;
    protected void SelectFiles(InputFileChangeEventArgs e)
    {
        Files = e.GetMultipleFiles(MaxUploadFiles).Select(f => f.Name);
    }
}

Exceptions (if any)

Error: Connection disconnected with error 'Error: Server returned an error on close: Connection closed with an error.'. e.log @ blazor.server.js:1

.NET Version

6.0.302

Anything else?

No response

javiercn commented 2 years ago

@Olby2000 thanks for contacting us.

What is likely happening here is that your uploads are failing even before they start, when we are retrieving the data about the files, which is exceeding the maximum SignalR message size.

There is a message size limit defined by SignalR that applies to every message Blazor receives. Our InputFile works with that constraint and streams the file up to the server in messages that respect that limit. However, the first message, which indicates the set of files to upload, is sent as a unique single message, so the data needs to fit in it.

The issue is not about the size of the files, but about the number of them. It is somewhat rare that we see people hitting this limit. That said, it is definitely not impossible.

@guardrex can we add something to the docs about this?

Olby2000 commented 2 years ago

Thank you for confirming the cause of this.

The issue is not about the size of the files, but about the number of them. It is somewhat rare that we see people hitting this limit. That said, it is definitely not impossible.

I presume the limit comes from having long file names and associated info in the initial upload manifest? Unfortunately this is something we can not control. I can for sure limit file upload to 200 files for example, however there is no guarantee that a client will not try to submit file data that will exceed the message buffer size.

Is it possible to add additional logic in the InputFile component to split the upload manifest into multiple messages if it exceeds the buffer size for a single SingalR message?

ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

mkArtakMSFT commented 1 year ago

This requires significant redesign of how Blazor works. So we will need much more feedback from the community in support of this to consider it in the future.

ghost commented 8 months ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

MackinnonBuck commented 8 months ago

Might be addressed by fixing https://github.com/dotnet/aspnetcore/issues/51080