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

[DO NOT MERGE] Modify file upload sample to handle very large files #33809

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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" };
Copy link
Member Author

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.

private readonly string _targetFilePath;

// Get the default form options so that we can use them to set the default
Expand Down Expand Up @@ -100,9 +100,12 @@ public async Task<IActionResult> UploadDatabase()
trustedFileNameForDisplay = WebUtility.HtmlEncode(
contentDisposition.FileName.Value);

streamedFileContent =
await FileHelpers.ProcessStreamedFile(section, contentDisposition,
ModelState, _permittedExtensions, _fileSizeLimit);
using (var memoryStream = new MemoryStream())
Copy link
Member Author

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.

{
await FileHelpers.ProcessStreamedFile(section, contentDisposition,
ModelState, _permittedExtensions, _fileSizeLimit, memoryStream);
streamedFileContent = memoryStream.ToArray();
}

if (!ModelState.IsValid)
{
Expand Down Expand Up @@ -269,19 +272,18 @@ public async Task<IActionResult> UploadPhysical()
// For more information, see the topic that accompanies
// this sample.

var streamedFileContent = await FileHelpers.ProcessStreamedFile(
section, contentDisposition, ModelState,
_permittedExtensions, _fileSizeLimit);

if (!ModelState.IsValid)
{
return BadRequest(ModelState);
}

using (var targetStream = System.IO.File.Create(
Path.Combine(_targetFilePath, trustedFileNameForFileStorage)))
{
await targetStream.WriteAsync(streamedFileContent);

await FileHelpers.ProcessStreamedFile(
section, contentDisposition, ModelState,
_permittedExtensions, _fileSizeLimit, targetStream);

if (!ModelState.IsValid)
{
return BadRequest(ModelState);
Copy link
Member Author

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.

}

_logger.LogInformation(
"Uploaded file '{TrustedFileNameForDisplay}' saved to " +
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.Extensions.DependencyInjection;
Expand All @@ -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
options.Limits.Http2.InitialStreamWindowSize = 16 << 20; // 16 MB
Copy link
Member Author

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).

options.Limits.Http2.MaxFrameSize = (1 << 24) - 1;
});
});
}
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>
<TargetFramework>net9.0</TargetFramework>
Copy link
Member Author

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.

</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.EntityFrameworkCore.InMemory" Version="3.1.0" />
<PackageReference Include="Microsoft.EntityFrameworkCore.InMemory" Version="9.0.0-rc.1.24451.1" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -146,39 +146,33 @@ public static async Task<byte[]> ProcessFormFile<T>(IFormFile formFile,
return Array.Empty<byte>();
}

public static async Task<byte[]> ProcessStreamedFile(
public static async Task ProcessStreamedFile(
MultipartSection section, ContentDispositionHeaderValue contentDisposition,
ModelStateDictionary modelState, string[] permittedExtensions, long sizeLimit)
ModelStateDictionary modelState, string[] permittedExtensions, long sizeLimit, Stream destination)
{
var oldLength = destination.Length;
try
{
using (var memoryStream = new MemoryStream())
{
await section.Body.CopyToAsync(memoryStream);
await section.Body.CopyToAsync(destination);
Copy link
Member Author

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>


// Check if the file is empty or exceeds the size limit.
if (memoryStream.Length == 0)
{
modelState.AddModelError("File", "The file is empty.");
}
else if (memoryStream.Length > sizeLimit)
{
var megabyteSizeLimit = sizeLimit / 1048576;
modelState.AddModelError("File",
$"The file exceeds {megabyteSizeLimit:N1} MB.");
}
else if (!IsValidFileExtensionAndSignature(
contentDisposition.FileName.Value, memoryStream,
permittedExtensions))
{
modelState.AddModelError("File",
"The file type isn't permitted or the file's " +
"signature doesn't match the file's extension.");
}
else
{
return memoryStream.ToArray();
}
// Check if the file is empty or exceeds the size limit.
if (destination.Length == 0)
{
modelState.AddModelError("File", "The file is empty.");
}
else if (destination.Length > sizeLimit)
{
var megabyteSizeLimit = sizeLimit / 1048576;
modelState.AddModelError("File",
$"The file exceeds {megabyteSizeLimit:N1} MB.");
}
else if (!IsValidFileExtensionAndSignature(
contentDisposition.FileName.Value, destination,
permittedExtensions))
{
modelState.AddModelError("File",
"The file type isn't permitted or the file's " +
"signature doesn't match the file's extension.");
}
}
catch (Exception ex)
Expand All @@ -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
Copy link
Member Author

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.

}

return Array.Empty<byte>();
}

private static bool IsValidFileExtensionAndSignature(string fileName, Stream data, string[] permittedExtensions)
Expand All @@ -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))
Copy link
Member Author

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.

{
if (ext.Equals(".txt") || ext.Equals(".csv") || ext.Equals(".prn"))
{
Expand Down Expand Up @@ -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.
/*
Copy link
Member Author

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.

if (!_fileSignature.ContainsKey(ext))
{
return true;
}
*/

// File signature check
// --------------------
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
{
"Logging": {
"LogLevel": {
"Default": "Debug",
"System": "Information",
"Microsoft": "Information"
"Default": "Warning",
"Microsoft": "Debug"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
}
},
"AllowedHosts": "*",
"StoredFilesPath": "c:\\files",
"FileSizeLimit": 2097152
"StoredFilesPath": "D:\\tmp\\LargeFileUpload",
"FileSizeLimit": 6442450944
Copy link
Member Author

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). 🤷

}
Loading