Skip to content

Results.File and Results.Stream consume large amount of Memory, don't stream file directly anymore #45037

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

Closed
1 task done
davepermen opened this issue Nov 11, 2022 · 12 comments
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Comments

@davepermen
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

using MapGet("/", () => Results.File("bigfile.bin")) causes huge amount of memory consumption, and multiple seconds of delay till the download starts.

In .net6, download started instantly and used up no memory.

Same Issue on Results.Stream

Bug with screenshots for comparison visible here:

image
image

Expected Behavior

Results.File and Results.Stream should directly stream the file download without loading it first into ram.

Steps To Reproduce

I created a repo here, needs a big file in wwwroot/1gb.bin

https://github.com/davepermen/ResultsFileBug

to create file quickly (on windows): open cmd in wwwroot and run "fsutil file createnew 1gb.bin 1073741824" but any file works.

switch project between net6 and net7 to see the effect.

Exceptions (if any)

No response

.NET Version

7.0.100

Anything else?

No response

@mkArtakMSFT mkArtakMSFT added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Nov 11, 2022
@davidfowl
Copy link
Member

@davepermen Are you seeing this with HTTP/2? I assume you are using the browser to download the file?

@davepermen
Copy link
Author

Yes, both cases the browser responds with "h2" (i assume it's short for HTTP/2).

i the browser network tools, it states on net6: timing: it starts after 4.8ms. in net7, 3.2s. transfer itself is a bit above 3seconds in both.

@davidfowl
Copy link
Member

I can reproduce the issue, looking into it.

@davidfowl
Copy link
Member

OK this seems to be a bug in the BrowserLink middleware (something visual studio installs to do refreshing of HTML).

image

Seems like there's a large allocation happening.

@davepermen You can turn off it off with this feature in visual studio:

image

Can you also confirm that this does not repro from the command line?

cc @vijayrkn @jodavis

@davidfowl
Copy link
Member

OK it's not the browser link middleware but it seems like the BrowserRefresh middleware is also to blame here 😄. (sorry @vijayrkn and @jodavis).

@MackinnonBuck it looks like this change might be the problem. We're using a memory stream to buffer the entire response body instead of the passthrough we had before.

@davepermen
Copy link
Author

I can confirm, "dotnet run" from the commandline works as expected, and disabling CSS Hot Reload solves the issue in Visual Studio for now, till a proper fix is back 👍

@MackinnonBuck
Copy link
Member

MackinnonBuck commented Nov 14, 2022

Yep, we changed the BrowserRefresh middleware to buffer the entire response to improve the reliability of hot reload script injection. We could fix the memory usage problem by checking earlier whether we're working with an HTML response so we can switch back to a passthrough in cases where we know script injection can't be performed.

Possible servicing candidate?

cc @mkArtakMSFT. Note that this issue only affects development scenarios with hot reload enabled.

@davidfowl
Copy link
Member

@mkArtakMSFT @MackinnonBuck yes we need to service this.

@davidfowl
Copy link
Member

davidfowl commented Nov 15, 2022

Yep, we changed the BrowserRefresh middleware to buffer the entire response to improve the reliability of hot reload script injection. We could fix the memory usage problem by checking earlier whether we're working with an HTML response so we can switch back to a passthrough in cases where we know script injection can't be performed.

I think we should revert the fix and work on a more reliable solution in .NET 8. We need to get rid of the buffering. Developers doing performance testing while running visual studio are going to see huge problems with that.

@KSemenenko
Copy link

@davidfowl What is the right way to handle large files?
If I need to transfer it.

I have so far come to the solution that I open the stream from the disk and write the data into HttpResponce, but still for a 250mb file it is a problem.

I also tried to do when I receive a file from a user, first write it to disk, and then take action.

I also have a http stream in memory anyway.

Do you have any examples?

@davidfowl
Copy link
Member

The sample here is fine. The problem is our middleware that is buffering the response (as described in the replies above).

@Spirch
Copy link

Spirch commented Dec 27, 2022

I have spend multiple hours trying to figure out why this simple code wasnt working

just create a new asp net web core web app mvc, put this in the home controller, .net 7.0.1 and vs 17.4.3

       public async Task<FileStreamResult> Test()
        {
            //random big file found online
            //const string url = "http://ipv4.download.thinkbroadband.com/512MB.zip";
            const string url = "https://speed.hetzner.de/1GB.bin";
            var client = new HttpClient();
            var stream = await client.GetStreamAsync(url);
            return File(stream, "application/zip");
        }

it always load everything in memory before the browser start downloading

I tried disabling the CSS Hot Reload, it doesnt work

it only work when i do a dotnet run

@dougbu dougbu removed this from the 7.0.x milestone Jan 20, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

No branches or pull requests

8 participants