unjs / unhead

Unhead is the any-framework document head manager built for performance and delightful developer experience.
https://unhead.unjs.io
MIT License
589 stars 39 forks source link

Memory Leak #281

Open agracia-foticos opened 9 months ago

agracia-foticos commented 9 months ago

Environment

Reproduction

image

When i crawled our web, crashes by memory leak

Describe the bug

image

When i crawled our web, crashes by memory leak

Additional context

No response

Logs

No response

harlan-zw commented 9 months ago

Will need some reproduction details

agracia-foticos commented 9 months ago

Will need some reproduction details

We have a lot of code generated, it is impossible to have a minimal playback environment

harlan-zw commented 9 months ago

Without a reproduction, there isn't much I can do.

This screenshot just shows it hit a memory limit while running this line, which doesn't mean that's where a memory leak is.

There's no way to confirm if Unhead is the problem or if downstream code implementing it, which seems more likely given this is the first time a memory leak issue has been opened and knowing how the API is designed.

dargmuesli commented 8 months ago

There could be some truth to this report. As you know, @harlan-zw, I'm new to debugging memory overflows, but poking around led me to the function

https://github.com/unjs/unhead/blob/b2bc2531aacf06c85b38bbe283570ff7d4a2e3f5/packages/unhead/src/createHead.ts#L48

in which there are two suspects: const updated and const head. Both refer to each other and it seems that could create a loop which might prevent associated resources from being garbage collected. Would that make sense?

BobbieGoede commented 8 months ago

While testing for memory leaks in @nuxtjs/i18n it did seem like something was leaky and I suspected unhead, but I haven't been able to consistently reproduce it.

Whatever seemed to be leaking did so at a low rate, unless someone manages to reproduce it, I wouldn't worry too much about it. 😅

harlan-zw commented 8 months ago

Thank you for your investigation @BobbieGoede! Some great work on @nuxtjs/i18n :clap:

dargmuesli commented 8 months ago

I investigated too :sob: :joy: I have one or two other theories on suspect code, maybe we can have a look together in the coming days @harlan-zw or @BobbieGoede? If you have time next week, we could meet on Discord to stop the leak :broom:

dargmuesli commented 8 months ago

Just saw and tested that https://github.com/nuxt-modules/i18n/pull/2621 is also a great fix :fire: :rocket: From the looks it seems that the only directly noticeable leak left is on me in my project. So no need for a call rn, but I may get back to that if I find anything else. Thank you all for the good work!

MosheL commented 7 months ago

I can explain better than the opener of the bug. He is right. I was wasted a day to debug a similar bug in my code, so I can explain better.

The leak is in the $vm._ of vue, persisted in node memory by the global __unhead_injection_handler__ and its friends, so after the SSR page go down, the memory is not freed and stayed in memory again and again.

image

harlan-zw commented 7 months ago

There should only be a single instance of Vue kept in memory, it's not freed as you correctly identified.

It shouldn't be stacking though, your screenshot doesn't show that it is afaik?

BobbieGoede commented 7 months ago

BTW, while I was searching for what could be leaking memory in the i18n module I often saw this pattern (Symbol(__DEV__ ? 'LIB_KEY_HERE' : '') which obscures/removes the identifying global key in a production build.

I guess the primary reason for doing this is to reduce the build size, but a side effect is that we don't see these keys when debugging memory (which I have only done on a production build). Since unhead does not use this pattern, and it uses some memory, it will be one of the first things you'll see when debugging which doesn't necessarily mean that it is leaking.

obulat commented 7 months ago

This seems to be one of the largest memory leaks that's preventing us at Openverse from migrating to Nuxt 3. Even after I removed all of the useHead calls, only leaving the head properties in the nuxt.config.ts, I can see a lot of retained dependencies of $head:

Screenshot 2024-01-25 at 6 08 59 PM

Here's the PR for migrating to Nuxt 3 that is, unfortunately, unusable, due to memory leaks.

harlan-zw commented 7 months ago

Looks like a vue-router stack? $head is a singleton on the vue VM (like globalProperties), so not sure what you're showing here exactly.

Not to say there isn't an issue, but this doesn't help me identify anything from Unhead's side.

Also, see comment above.

This is something I do want to take a deeper look into in any case but I'm currently limited with time so unless more people are reporting memory leak issues with Nuxt / Unhead then it's a low priority as it's going to be a lengthy investigation.

BobbieGoede commented 7 months ago

@obulat Also I see you're using @nuxtjs/i18n, I recommend installing the edge version with pnpm i -D @nuxtjs/i18n@npm:@nuxtjs/i18n-edge, the latest changes include a fix for a memory leak.

This won't fix the memory leak in your PR (I checked) but likely reduces it, it could still be the i18n module or unhead or both, I'll look into it some more when I have the time.

obulat commented 7 months ago

@obulat Also I see you're using @nuxtjs/i18n, I recommend installing the edge version with pnpm i -D @nuxtjs/i18n@npm:@nuxtjs/i18n-edge, the latest changes include a fix for a memory leak.

This won't fix the memory leak in your PR (I checked) but likely reduces it, it could still be the i18n module or unhead or both, I'll look into it some more when I have the time.

Thank you for looking into our repo, @BobbieGoede ! I'll try the edge version.

harlan-zw commented 6 months ago

I did some testing with clinic.js and was not able to replicate any sort of memory leak.

image

This is with ~500 connections doing ~5k requests x2 on a Nuxt app.

Will leave this issue open in case anyone has any further research they can share. (I tried testing with other clinic.js tools but it was failing).