woutdp / live_svelte

Svelte inside Phoenix LiveView with seamless end-to-end reactivity
https://hexdocs.pm/live_svelte
MIT License
1.2k stars 46 forks source link

Not destroying Svelte components #108

Closed JOldak closed 8 months ago

JOldak commented 8 months ago

We had a bug in our LiveView app where Svelte components were creating big memory leaks, and after spending some time following it through it seems to be because LiveSvelte never calls $destroy() on components that it creates.

Looking in hooks.js I found this code/comment:

        destroyed() {
            // We don't want to destroy the component
            // If we do a page navigation, this would remove the component in the DOM,
            // and then it would to the transition, causing a flicker of unrendered content
            // Since we're doing a page transition anyway, the component will be remove automatically
        }

The comment shows that this behaviour is intentional. However, our experience is that components that have globals never get to clean up after themselves, and so we get big memory leaks. When doing live navigation between pages the components are never destroyed, but are recreated next time the page is viewed. So every view leaks all the globals from all the components in that view.

I confirmed this by adding an onMount and onDestroy callback in a custom component. onMount is called as expected, but onDestroy is never called.

We are using components from Carbon Components Svelte and a few in particular we noticed have big memory leaks when not destroyed. For example the DatePicker leaks flatpickr instances, and TooltipIcon leaks an onkeydown handler.

To fix this for our project I have hacked in a call to $destroy() after getting the hooks from LiveSvelte. A bit like this:

import { getHooks } from "live_svelte"
import * as SvelteComponents from "../svelte/**/*.svelte"

let Hooks = {
    ...getHooks(SvelteComponents),
}

Hooks.SvelteHook.destroyed = function () {
    this._instance.$destroy();
}

It would seem sensible to me to include the call to $destroy() in the destroyed() method as standard?

Thanks

Joe

GitUser0001 commented 8 months ago

I also faced the problem of memory leakage, but I was not able to find the cause. What I noticed is that before adding livesvelte the memory did not go above 200mb, but now it increases to 5GB in a week.

Joe, thanks for posting the solution, I'll try to use the fix for myself and let you know if it solved the problem

woutdp commented 8 months ago

This is a great catch. I've added your change and made a new version, can you try again with version 0.13.0 and see if the issue is solved?

JOldak commented 8 months ago

0.13.0 does solve the problem, thanks! :-)

JOldak commented 8 months ago

@GitUser0001 unrelated to this specific issue, but in terms of memory use I also found that if you have console debug on then it uses loads of memory, because the log messages from phoenix contain references to the objects being changed, which means they can't ever be garbage collected. I found that adding liveSocket.disableDebug() to app.js meant that memory use was much reduced!

GitUser0001 commented 8 months ago

Thank you, Joe! I will try this change as well.