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.

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

Request Streaming upload #33693

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Sep 23, 2024

Fixes #33692
Fixes #33850 ... Adds a Security considerations section with two subsections: One is on avoiding IBrowserFile.Size for file size limits. The other is on not using a client-supplied file name for saving a file to physical storage.

Notes ...

  • Tested working 🎉🍻 with ...
    • BWA (Interactive WebAssembly rendered component to server project)
    • Standalone Blazor WebAssembly app to a web API.
  • Eventually, a fully working demo for both BWA and WASM/web API can be placed into sample apps for easy demo by users. I've made a tracking note to address it later.

BIG ❓...

My understanding is that request streaming gracefully degrades when any of the following conditions fail:

  • Chromium-based browser
  • HTTP/2
  • CORS (preflight must succeed)
  • HTTPS

However ...

For a Safari/FF user and a large file upload, this current guidance is going to 💥. We could take any of the following approaches ...

  1. Do nothing outside of just letting readers know that this feature is only supported for Chromium browsers at this time.
  2. Add code: If the user agent is a non-Chromium browser, prevent the user from uploading a large file.
  3. Add code: If the user agent is a non-Chromium browser, don't post with multipart body. Take Javier's advice and set up the component to use HTTP Ranges if the browser is detected as non-Chromium. We've said in the article to use Ranges in large file upload scenarios but never actually showed how to do it. I checked the Web for good examples, and I didn't find any ... just bits of code and passing remarks.

For 2 or 3 ☝️, tell me if checking the user agent via JS interop is the right way to go. I mean there's nothing else available via API that I can check to determine if it's a non-Chromium browser, correct?

If we're going with showing an HTTP Ranges fallback, my 🦖 hacks with MultipartContent("byteranges", "..."); didn't really result in a working demo. It is best if a PU engineer provide a fully-working client-server example of the HTTP Ranges approach.

... and one more related ❓ ...

This is CSR right now for the BWA side ... Interactive WASM ... no IsBrowser check and no alternative processing required in this section/demo. Up to this point, I was just trying to get it working in the CSR case for the CSR section. I haven't delved into Auto rendering yet. Do you want to get into the weeds with the Auto scenario (i.e., alternative services for server versus client), or do you want to wait and think about that later?


Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/call-web-api.md aspnetcore/blazor/call-web-api
aspnetcore/blazor/file-uploads.md aspnetcore/blazor/file-uploads
aspnetcore/grpc/why-migrate-wcf-to-dotnet-grpc.md aspnetcore/grpc/why-migrate-wcf-to-dotnet-grpc

@guardrex guardrex self-assigned this Sep 23, 2024
@guardrex
Copy link
Collaborator Author

@pavelsavara ... Do you know if dotnet/runtime#91699 has made it into the latest SDK that I think I can get, which is 9.0.100-rc.2.24474.4?

I'm still hitting the out of memory error with large file uploads using our published example code under the public RC1 release (9.0.0-rc.1.24431.7), which I think will work (in Edge) if I get the SDK that has dotnet/runtime#91699 in it.

@pavelsavara
Copy link
Member

It should be there. Which sample code ?

@guardrex
Copy link
Collaborator Author

This ...

https://learn.microsoft.com/en-us/aspnet/core/blazor/file-uploads?view=aspnetcore-8.0#upload-files-to-a-server-with-client-side-rendering-csr

There are two spots that we'll need coverage ...

  • In the File Uploads article, which is on the PR in draft form. The question is if request streaming will ✨ Just Work!™ ✨ without code changes to the FileUpload2 component IF they're using a Chromium browser? If request streaming isn't available (Safari/FF), doesn't request streaming gracefully fall back? ... noting that we never maintained content on Javier's suggestion to developers looking for client-side large file uploads to create a custom HTTP Ranges-based file upload component. We say that's what to do, but we never showed an example of it. I wonder if we need to show how to use HTTP Ranges in case request streaming isn't available?
  • We also need coverage in the Call web API article, which I'm working on now locally (i.e., it isn't on the PR yet). There, I'll be adding a section and describing the SetBrowserRequestStreamingEnabled extension method for HttpRequestMessage.

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 24, 2024

If the FileUpload2 component IS going to change, then I'll need the updated component to place in versioned content. Can you (or someone) send me the new component to replace it for >=9.0?

UPDATE: Nevermind on that! I have the code generally working, so I'll float what I have on the PR for review.

I also have a side question WRT the LazyBrowserFileStream part that @MackinnonBuck provided ... Mackinnon ... is that to be versioned OUT 🔪 at >=9.0? IIRC, it was a temporary workaround that can go away at 9.0. That's what the comments that I left for myself in the article seem to indicate. It looks like we'll carry the LazyBrowserFileStream approach <9.0 and version it out >=9.0 in favor of just a stream.

@pavelsavara
Copy link
Member

Something like below will enable that for Chrome/Edge.

var webAssemblyEnableStreamingRequestKey = new HttpRequestOptionsKey<bool>("WebAssemblyEnableStreamingRequest");
var req = new HttpRequestMessage(HttpMethod.Post, url);
req.Options.Set(webAssemblyEnableStreamingRequestKey, true);

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 24, 2024

So, you're saying that we'll have to move away from an HttpClient.PostAsync(string, HttpContent) to a HttpClient.SendAsync(HttpRequestMessage) ... that's going to be a firm requirement for it to work?

The reason that I thought the code wouldn't need to change is that I thought the inner workings of HttpClient.PostAsync would perform the checks for HTTP/2 and IsBrowser and automatically set the streaming request key ... that it would do internal magic ✨ to make streaming requests work. I guess you're saying that's not the case.

I have a local test environment here for this. I'll refactor it into an HttpClient.SendAsync(HttpRequestMessage) approach to see if I can get it working. I'll report back in a bit how it went.

@guardrex
Copy link
Collaborator Author

Can't I just do it this way? ...

var req = new HttpRequestMessage(HttpMethod.Post, "/Filesave");
req.SetBrowserRequestStreamingEnabled(true);

@pavelsavara
Copy link
Member

My point was to use the WebAssemblyEnableStreamingRequest option, not how to set it.

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 24, 2024

Latest code is ...

var req = new HttpRequestMessage(HttpMethod.Post, "/Filesave");
req.SetBrowserRequestStreamingEnabled(true);
req.Version = HttpVersion.Version20;
req.Content = content;
var response = await Http.SendAsync(req);

Works for small files, but for a 1 GB file, I'm getting a Kestrel(?) exception ...

"type": "https://tools.ietf.org/html/rfc9110#section-15.5.1",
"title": "One or more validation errors occurred.",
"status": 400,
"errors": {
  "": [
    "Failed to read the request form. Request body too large. The max request body size is 30000000 bytes."
  ]

@pavelsavara
Copy link
Member

"Failed to read the request form. Request body too large. The max request body size is 30000000 bytes."

That's actually server side error. Try setting MaxRequestBodySize on your server

@pavelsavara
Copy link
Member

req.Version = HttpVersion.Version20; is not necessary, doesn't do anything.

@guardrex
Copy link
Collaborator Author

I hit another limit on the server ...

"Failed to read the request form. Multipart body length limit 134217728 exceeded."

I'll see about extending that one.

@guardrex
Copy link
Collaborator Author

Lick'd IT! 🍻🕺💃🎉

Ok ... I'll get this all into the PR. I'll ping for reviews Wednesday when it's all in place.

image

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 25, 2024

@danroth27 @mkArtakMSFT @pavelsavara @maraf ... See the NOTES that I just added to the opening comment ☝️.

This is ready for a look and discussion on next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants