Skip to content

Fixed: Memory leak #521

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
Remcoman opened this issue Feb 18, 2020 · 9 comments
Closed

Fixed: Memory leak #521

Remcoman opened this issue Feb 18, 2020 · 9 comments
Labels

Comments

@Remcoman
Copy link

Thanks for making this nuxt.js plugin! We use it in all our projects and it saves us a lot of hassle updating the meta tags for our sites.

I'm currently tasked with fixing some issues related to crashes and slowdowns on one of our latest projects. After spending some time figuring out what causes this issue i think i have discovered the culprit.

It seems that vue-meta is keeping references to earlier route components in memory. In the Chrome dev-tools you can clearly see that the number of DOM nodes keeps increasing (and never goes down):

Screenshot 2020-02-18 at 12 35 19

Here is reproduce-able example:
https://codesandbox.io/s/awesome-lalande-hrc8t

If you open the "Performance monitor" and repeatedly switch between the homepage and about page you will see the amount of DOM nodes growing.

What happens:

  • A user visits the site
  • Nuxt triggers the route
  • The beforeCreate for vue-meta is called in the context of the component
  • vue-meta adds callbacks for the created, beforeMount, mounted, activated, deactivated lifecycle hooks to the corresponding array in the $options object. So something like this: $options.beforeMount.push(callback)

Normally the list of callbacks gets cleared on every route change (a new instance of a route component will have no callbacks for the lifecycle hooks). However, if the developer has added their own lifecycle hooks with custom behaviour (like shown in the example) the list of callbacks will be on the prototype. So instead of adding callbacks to the component instance, vue-meta will add the callbacks to the component class.

The result is that these callbacks and all connected scopes (including the previous route component and element) will not be cleared.

I've created a pr were i have replaced the ensurePush($options, 'lifecycle', callback) statements by this.$on('hook:lifecycle', callback). This works because events on components are correctly cleaned up by Vuejs. Here is an updated example:

https://codesandbox.io/s/async-wildflower-5bttb

(Note how the amount of DOM nodes stays around 4000)

@pimlie
Copy link
Collaborator

pimlie commented Feb 23, 2020

Please check my bad news in the PR. Although the PR seems to fix the problem, I havent been able to reproduce the problem its trying to resolve independently in vanilla Vue.js. As using hook events is an internal Vue API, we shouldnt/cant use it unfortunately. But I will checkin with the Vue.js core team to see if they've changed their mind now with v3 coming up.

Small but important note, vue-meta is not really a Nuxt.js plugin. Its an independent Vue.js library which is just maintained by the Nuxt.js core team. There are also many other frameworks that use this package: https://vue-meta.nuxtjs.org/guide/frameworks.html :)

--edit--
Have added the needs repro label because we need proof that adding those programmatic life-cycle hooks are also acting up in a vanilla Vue.js repro. Unfortunately I have been unable to do that, which means the issue is likely somewhere else.
Any help would be appreciated!

@Remcoman
Copy link
Author

Remcoman commented Feb 24, 2020

Thanks for looking at my pr. I've created a simple reproducible example in vanilla Vue. You can find it here:

https://codesandbox.io/s/memory-leak-in-vanilla-vue-cf4qe

Note how the length of:
window.app._vnode.componentOptions.Ctor.sealedOptions.components.Page2._Ctor[0].options.created
keeps increasing with every page switch.

I hope this will be enough proof that there is indeed an issue with the programmatic lifecycle hooks.

Concerning the hook events. They are documented here: https://vuejs.org/v2/guide/components-edge-cases.html#Programmatic-Event-Listeners. Nowhere on that page is mentioned that it is an internal Vue api, so i find it really odd that a Vue Core developer would say such a thing.

@pimlie
Copy link
Collaborator

pimlie commented Feb 24, 2020

Thanks. Sorry for being unclear, but I meant a reproduction without vue-meta and just programmatic lifecycle hooks. If you look at the jsfiddle in the vue issue I linked in the PR, it doesnt seem to be experiencing any issue with retaining components / a mem leak: https://jsfiddle.net/h86atdq4/

So although using hook events seems to be a solution, at the same time it doesnt appear to be the real issue. Which means we might fix a symptom but not the issue itself. It could very well be related how we use those hooks in vue-meta, but as mentioned already I've spent days debugging this and couldnt find anything strange (maybe a fresh set of eyes will have more luck?). We really need to fully understand why/what is happening here before using hook events.

so i find it really odd that a Vue Core developer would say such a thing

You can find a public resource here if you dont believe me: vuejs/v2.vuejs.org#2247

@Remcoman
Copy link
Author

I've made a small modification to your jsfiddle:

https://jsfiddle.net/c61xL8ar/2/

As you can see, the length of the created & mounted array keeps increasing and is never cleared.

@pimlie
Copy link
Collaborator

pimlie commented Feb 24, 2020

Interesting find, thanks. So its a Vue.js issue after all I suppose. Would you be willing to share your findings in the github issue I linked in the PR?

@Remcoman
Copy link
Author

I don't think it is a Vue js issue. In your linked issue it is stated(vuejs/vue#10458 (comment)) that you are modifying the components options before the components instance is created. I quite agree with this. You are changing the frameworks implementation, which is perfectly fine, but I do think that by doing that you make yourself responsible for that part.

I think by using the documented internal vue hooks we can fix it. I'm willing to help. Anyway, I just want this ti be fixed. What do you suggest the next step would be?

@pimlie
Copy link
Collaborator

pimlie commented Feb 25, 2020

The next step is to ask the Vue-team what we should do, see my post in the vue issue. If using hook events turns out fine (either because v3 is coming out soon or because of new insights by the Vue team) I will reopen your PR and merge it.

But we cant just merge something when we were told before to not use it. Hope you will at least understand that we have to walk the proper ways and discuss your findings first with the Vue-team.

--edit--
@Remcoman Also dont cherrypick comments that suits your narrative, even in the vue issue a Vue-core team member explains it should be fine to use this.$options

@gerasimvol
Copy link

Same here.
After few client route changes - amount of vue components are growing and memory doesn't clear.
Moreover, node server memory also drowns while users visit site until node dies (RAM exceeded)

Initial state:
initial state

After some navigation:
after navigation

@pimlie
Copy link
Collaborator

pimlie commented Feb 26, 2020

Received the confirmation from the Vue core-team that using hook events is the workaround for now. Its still being debated though whether the previous approach is a bug or not

The fix is shipped in v2.3.3

@pimlie pimlie closed this as completed Feb 26, 2020
@pimlie pimlie changed the title Possible memory leak Fixed: Possible memory leak Feb 26, 2020
@pimlie pimlie changed the title Fixed: Possible memory leak Fixed: Memory leak Feb 26, 2020
@pimlie pimlie pinned this issue Feb 26, 2020
@atinux atinux unpinned this issue Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants