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

fix(dev): increase default session memory limit in HTTPS dev server to prevent disconnections on large projects. #6207

Merged

Conversation

davidwallacejackson
Copy link
Contributor

Description

In projects with a particularly large number of modules, some requests to the dev server will fail when the server is under heavy load. Which requests fail is nondeterministic, but some of them always will. It looks like this:

image

Here's a repro. Unfortunately, I couldn't do this in StackBlitz -- when I tried to run it there, I couldn't even get the app to start. I'm assuming that's because this involves intentionally thrashing the filesystem and StackBlitz isn't optimized for that kind of fs access, but I don't really know.

Additional context

When I had this problem with my own app, I did a bunch of research on the 502 ENHANCE_YOUR_CALM error that I was seeing in my terminal output. I eventually tracked it to nghttp2, the library Node uses to implement HTTP/2 support. This now-closed issue in Node itself gave me a clue as to what was going on. Apparently the error message is an anti-DDOS mechanism -- in the case of the now-fixed bug, the memory used by requests wasn't getting released.

The default value for this limit is 10MB. I guessed that it might just be too small for a large-ish Vite project, so after seeing how the options get passed to createSecureServer, I bumped it up in my own project by passing https: { maxSessionMemory: 1000 } in my vite.config.ts, and this seemed to solve the problem consistently. 100 also seemed to work, but it doesn't for this repro.

Admittedly, the repro is absolutely bonkers. My real project loads ~1800 modules (we're working on code-splitting, but we did some things that inadvertently made that difficult, so it'll take a while 😬 ) but pulls down nowhere near as much data as the repro does. I'm not sure what a reasonable limit is, or what the dangers of bumping the default value might be (will it affect SSR?), so I'm curious what you all think about that. It also seems like something that should probably be configurable, but I had a bit of trouble trying to add it to the allowed types -- the option in question comes from SessionOptions, which doesn't make it into the set of allowed options as considered by Vite for reasons I haven't figured out yet.

Also, if you'd like me to make my repro into a test, let me know! I wasn't sure if this sort of heavyweight thing was appropriate for the test suite, but happy to add it if it is.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • [x Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

Thanks for creating this repro. For reference, @davidwallacejackson showcased the issue and this is terrible when it happens. I think we aren't getting more issues related to this problem because of the big amount of modules that are being loaded.

Given that this option can already be configured, maybe we should document this problem and its solution instead of bumping the limit? If it was 20MB instead of 10MB, it wouldn't be a problem, but 1000MB is starting to sound that another issue may be hiding here and as you said... I would also not feel comfortable bumping it that high by default.
I will bring this PR to our next team meeting. Maybe a possible way forward until we find more of why we are using so much memory is to have a Common issues section in the docs where we can add an entry about these problems that large apps are hitting? Also recommending working on lazy loading, etc

@patak-dev patak-dev added bug p3-minor-bug An edge case that only affects very specific usage (priority) labels Dec 30, 2021
@davidwallacejackson
Copy link
Contributor Author

I'm not sure how high it really needs to be -- 1000MB was what I did in my own project; it seems like 100MB fixes it as well but I wanted to leave a lot of room because my team had hit the problem a lot and I wanted to leave a LOT of room, knowing I could change it in another PR if I really had to. Haven't tried something lower, like 20MB, but for all I know that might solve it.

Documenting it and letting users configure it also seems like a good idea -- I was actually thinking that a nice way to do it would be to listen for the 502 error specifically with https://nodejs.org/api/http2.html#event-error and emit an error message directing users to that setting, but I haven't tried wiring that up yet.

@benatshippabo
Copy link
Contributor

benatshippabo commented Jan 6, 2022

I'm not sure how high it really needs to be -- 1000MB was what I did in my own project; it seems like 100MB fixes it as well but I wanted to leave a lot of room because my team had hit the problem a lot and I wanted to leave a LOT of room, knowing I could change it in another PR if I really had to. Haven't tried something lower, like 20MB, but for all I know that might solve it.

Documenting it and letting users configure it also seems like a good idea -- I was actually thinking that a nice way to do it would be to listen for the 502 error specifically with https://nodejs.org/api/http2.html#event-error and emit an error message directing users to that setting, but I haven't tried wiring that up yet.

Yeah, it seems another user had a similar issue before and had good luck with just setting it to 100mb and created a PR allowing users to specify maxSessionMemory in server.https. But then they closed it for some unknown reason.

Relevant links: #4403 #3895

@bluwy bluwy mentioned this pull request Mar 13, 2022
6 tasks
@liuweiGL
Copy link
Contributor

This is awesome: #3895, no why it's closed.

@John60676
Copy link

What's the status?

@hybridherbst
Copy link

hybridherbst commented Apr 23, 2022

We're also running into this issue, and pretty frequently. Seems to also happen not only when many modules are loaded, but simply when large files are served along the page (e.g. a 10-20MB GLB file or so).

@patak-dev
Copy link
Member

@davidwallacejackson, would you test again that the PR is working well for you? We talked about it today, we can merge it during the v3 beta and see if someone reports issues.

@Niputi Niputi added this to the 3.0 milestone May 6, 2022
@patak-dev
Copy link
Member

Let's merge it so we can start checking how it behaves for others.

@patak-dev patak-dev merged commit f895f94 into vitejs:main May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants