-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
[DO NOT MERGE] Modify file upload sample to handle very large files #33809
base: main
Are you sure you want to change the base?
Conversation
@@ -24,7 +24,7 @@ public class StreamingController : Controller | |||
private readonly AppDbContext _context; | |||
private readonly long _fileSizeLimit; | |||
private readonly ILogger<StreamingController> _logger; | |||
private readonly string[] _permittedExtensions = { ".txt" }; | |||
private readonly string[] _permittedExtensions = { ".txt", ".vhdx" }; |
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.
This was just an easy format in which to make a very large file.
streamedFileContent = | ||
await FileHelpers.ProcessStreamedFile(section, contentDisposition, | ||
ModelState, _permittedExtensions, _fileSizeLimit); | ||
using (var memoryStream = new MemoryStream()) |
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.
This will still blow up over ~2GB. I only fixed up the file impl, not the EF impl.
|
||
if (!ModelState.IsValid) | ||
{ | ||
return BadRequest(ModelState); |
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.
We should probably delete the file if this happens.
@@ -20,6 +20,12 @@ public static IHostBuilder CreateHostBuilder(string[] args) => | |||
.ConfigureWebHostDefaults(webBuilder => | |||
{ | |||
webBuilder.UseStartup<Startup>(); | |||
webBuilder.ConfigureKestrel(options => | |||
{ | |||
options.Limits.MaxRequestBodySize = 6L << 30; // 6 GB |
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.
This actually matters. The next two should just help perf for HTTP/2 (always measure though).
@@ -1,11 +1,11 @@ | |||
<Project Sdk="Microsoft.NET.Sdk.Web"> | |||
|
|||
<PropertyGroup> | |||
<TargetFramework>netcoreapp3.1</TargetFramework> | |||
<TargetFramework>net9.0</TargetFramework> |
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.
This mostly just made debugging easier. It's possible my changes work in 3.1 as well.
using (var memoryStream = new MemoryStream()) | ||
{ | ||
await section.Body.CopyToAsync(memoryStream); | ||
await section.Body.CopyToAsync(destination); |
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.
Reading into a MemoryStream
was the real bottleneck, AFAICT>
@@ -187,9 +181,8 @@ public static async Task<byte[]> ProcessStreamedFile( | |||
"The upload failed. Please contact the Help Desk " + | |||
$" for support. Error: {ex.HResult}"); | |||
// Log the exception | |||
destination.SetLength(oldLength); // Reset the stream to its original length |
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.
This clean-up may be inadequate or even counter-productive.
@@ -208,7 +201,7 @@ private static bool IsValidFileExtensionAndSignature(string fileName, Stream dat | |||
|
|||
data.Position = 0; | |||
|
|||
using (var reader = new BinaryReader(data)) | |||
using (var reader = new BinaryReader(data, System.Text.Encoding.UTF8, leaveOpen: true)) |
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.
If we're going to operate on a real stream, we should probably not close it after validation.
@@ -247,12 +240,10 @@ private static bool IsValidFileExtensionAndSignature(string fileName, Stream dat | |||
// for files (when possible) for all file types you intend | |||
// to allow on the system and perform the file signature | |||
// check. | |||
/* |
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 was too lazy to work out a file signature for VHDX for a sample.
"StoredFilesPath": "c:\\files", | ||
"FileSizeLimit": 2097152 | ||
"StoredFilesPath": "D:\\tmp\\LargeFileUpload", | ||
"FileSizeLimit": 6442450944 |
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 calculated this in bytes (6 GiB) but it might actually be in MiB (i.e. 6 TiB). 🤷
Illustration for dotnet/aspnetcore#58233.
We may want to consider updating the sample properly, but this is just a proof of concept.