-
Notifications
You must be signed in to change notification settings - Fork 25.1k
Request Streaming upload #33693
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
Conversation
|
@pavelsavara ... Do you know if dotnet/runtime#91699 has made it into the latest SDK that I think I can get, which is I'm still hitting the out of memory error with large file uploads using our published example code under the public RC1 release ( |
|
It should be there. Which sample code ? |
|
This ... There are two spots that we'll need coverage ...
|
|
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 |
|
Something like below will enable that for Chrome/Edge. |
|
So, you're saying that we'll have to move away from an The reason that I thought the code wouldn't need to change is that I thought the inner workings of I have a local test environment here for this. I'll refactor it into an |
|
Can't I just do it this way? ... var req = new HttpRequestMessage(HttpMethod.Post, "/Filesave");
req.SetBrowserRequestStreamingEnabled(true); |
|
My point was to use the |
|
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 ... |
That's actually server side error. Try setting MaxRequestBodySize on your server |
|
|
|
I hit another limit on the server ...
I'll see about extending that one. |
|
@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. |
Co-authored-by: Daniel Roth <[email protected]>
Co-authored-by: Daniel Roth <[email protected]>
Co-authored-by: Daniel Roth <[email protected]>
Co-authored-by: Daniel Roth <[email protected]>
Co-authored-by: Daniel Roth <[email protected]>
danroth27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going make some executive decisions to get this PR unblocked:
- For non-Chromium browsers let's go with option 1, which is to do nothing outside of just letting readers know that this feature is only supported for Chromium browsers at this time.
- Let's drop the recommendation to use HTTP range requests as a fallback behavior given that we haven't actually proven that approach with a sample.
- Let's not bother with trying to document how to deal specifically with the auto render mode.
@lewing @pavelsavara @javiercn If you disagree with any of these decisions, then please open a separate doc issue with how you'd like them handled instead.
|
@danroth27 ... I'll react to your decisions in the morning. @lewing @pavelsavara @javiercn ... If you need to open an issue, please open it from the bottom of this article ... |
|
@danroth27 ... Based on your remarks earlier about not cross-linking the PU discussions on this, I simplified the File size read and upload limits section down to just ...
Otherwise, there are no changes for the other two items ...
|
|
|

Fixes #33692
Fixes #33850 ... Adds a Security considerations section with two subsections: One is on avoiding
IBrowserFile.Sizefor file size limits. The other is on not using a client-supplied file name for saving a file to physical storage.Notes ...
BIG ❓...
My understanding is that request streaming gracefully degrades when any of the following conditions fail:
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 ...
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
IsBrowsercheck 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