Memory leak in useRegisterActions hook

timc1 / kbar

fast, portable, and extensible cmd+k interface for your site

https://kbar.vercel.app

MIT License

4.84k stars 185 forks source link

Memory leak in useRegisterActions hook #233

Closed brydar closed 1 year ago

brydar commented 2 years ago

Hi, I stumbled upon an apparent memory leak in the useRegisterActions hook.

Step to reproduce:

Open this codeSandbox: https://codesandbox.io/s/compassionate-mirzakhani-mg3wgc?file=/src/App.js From the view here: https://mg3wgc.csb.app/

By inspecting the browser memory we can find that the Leaker class is kept in memory, more generally the action provider component state is leaked.

With 1 action registered (ProvideStuff)

Without any action registered (Nothing)

istered

useRegisterActions hook is apparently capturing the whole application state that can be linked to the perform lambda. In my case we had to indirect the perform function to a WeakRef to let the browser garbage collector clean it when the action is not registered anymore.

We didn't identify the underlying issue in the useRegisterActions file, maybe a stale closure or something, do you have an idea?

Thanks for the great work 👏

timc1 commented 2 years ago

Hey @brydar apologies for the late response – looking into this asap. Thank you!

timc1 commented 2 years ago

Alright, so I've been playing around with this sandbox for a bit – thank you for the environment!

Between each snapshot, it looks like you have your filter set to All object, which I'd imagine it's more accurate to filter the allocated objects between runs:

When doing so, it shouldn't display the previous run's Leaker:

https://user-images.githubusercontent.com/12195101/183519978-f116fc10-71cd-425e-842f-ce5c0fc043f9.mov

I'm not sure how ya'll are using this class instance in your site, but if you were using mobx autorun, for instance, within that Leaker class, you'd need to do your own cleanup when ProvideStuff unmounts.

stale[bot] commented 2 years ago

Hey! This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

dubzzz commented 2 years ago

Totally forgot to add some more context to it. But it seems that there was (or is) a leak.

dubzzz commented 2 years ago

@timc1 In your footage we can see the following filter: "allocated between Snapshot 7 and Snapshot 8".

So it seems normal not to see Leaker at this point as it has been allocated before Snapshot 7. In other words not seeing it is expected at this point because it has not been part of this incremental change; but it should have been released so if you check what you have in Snapshot 8 you should not find it.

I implemented a workaround version to avoid the leak of the library. I'll send you a link towards a patch version of it so that you can see the difference.

dubzzz commented 2 years ago

Here is a leak-free version: https://codesandbox.io/s/strange-chihiro-83vexl?file=/src/App.js or https://83vexl.csb.app/

dubzzz commented 2 years ago

With the leaky version: https://mg3wgc.csb.app/ kbar-leak

Without the leak: https://83vexl.csb.app/ kbar-no-leak

In both cases I took a snapshot initially to confirm the Leaker was really mount and available. Then I switch to the components not asking for any Leaker, asked Chrome to GC at least 10 times (Leaker may not be garbaged if you take a snapshot immediately as Chrome may decide not to bother cleaning it, the GC forces the clean). Finally I took a second snapshot.

stale[bot] commented 1 year ago

Hey! This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.