-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Possible memory leak #3668
Comments
Please share more information about your setup. We cannot see your screen and don't know what your code looks like, so it's pretty much impossible for us to provide help with the issue you're seeing. Specifically the virtual list implementation that's used in your project and how you're using it might be of interest. |
From the linked issue this is a chrome bug and not a Preact one, there's not much we can do about the implementation of a browser event handler as this would occur in any library. The screenshot attached also does not seem to relate to the bug as this is a VDOM structure. The event-handler in question is a native one as we aren't creating our own events so the only things interacting here should be your callback and the browser event-handler. If this is a Preact bug could you show us how this related to the chrome bug or how the screenshot is relevant to what you are saying - currently you say that the event handler holds the ref of an unmounted vnode, do you mean that the vnode holds a reference to the dom-node which in turn has event-handlers? Theoretically this makes sense as a virtualization library usually won't unmount dom-nodes rather than just hiding them so I don't think it's a memory leak as much as an implementation detail in the virtualization library. Do note that the above hinges on assumption that I made, if these are incorrect please provide more information |
I managed to create a demo to reproduce it https://codesandbox.io/s/preact-vlist-demo-ehphz9 same code using react has no such issue https://codesandbox.io/s/react-vlist-demo-f3egjs |
@Z2Y this happens because In the meantime, here's a workaround that adds some additional unmount cleanup that likely addresses the issue: import { options } from 'preact';
let oldUnmount = options.unmount;
options.unmount = (vnode) => {
if (oldUnmount) oldUnmount(vnode);
if (vnode.__e) vnode.__e.l = undefined;
if (vnode.__c) vnode.__c.__H = undefined;
vnode.__ = vnode.__k = vnode.props = undefined;
}; |
@developit Dose the workaround has other side effects? Since there are some work after options.unmount. Lines 499 to 527 in 1cb4b38
Currently I add these vnode to a set then do the cleanup later. options.unmount = (vnode) => {
if (oldUnmount) oldUnmount(vnode);
unmountedVnodes.add(vnode); // then do cleanup in a setInterval
}; |
@Z2Y the workaround won't break anything, I only have it clearing properties that Preact ignores during unmounting. Your Set solution is good though, you could iterate over the properties of each and set them to undefined. |
@developit that workaround might be dangerous in suspense scenario's no? |
I upgrade to latest preact v10.11.0, but the problem still exist. |
@developit @JoviDeCroock @marvinhagemeister - 👋🏻 I tried the solution mentioned above: import { options } from 'preact';
let oldUnmount = options.unmount;
options.unmount = (vnode) => {
if (oldUnmount) oldUnmount(vnode);
if (vnode.__e) vnode.__e.l = undefined;
if (vnode.__c) vnode.__c.__H = undefined;
vnode.__ = vnode.__k = vnode.props = undefined;
}; But this does not prevent the DOM nodes from being retained - and so memory usage continues to grow. I have an application that has very long-lived sessions so this is an important issue for me. Is there anything around the use of signals that could be causing this also? |
@shakyShane this might be a totally different issue, mind reproducing it for us so we can see what exactly is holding onto DOM-nodes. With the patch you do you achieve an empty Vnode so nothing should be able to hold onto it anymore |
@JoviDeCroock I realised this pattern causes the same problem in React too. I often use the pattern of waiting for an element to finish transitioning, before unmounting. Here's a vid showing the detached dom nodes: pre-mem.mp4And the code/site. code: https://github.com/shakyShane/preact-memory/blob/main/src/preact.js#L20 Love to know if you have any thoughts/ideas - it's clearly something related to event handlers. |
@shakyShane the events are never detached so the closure stays alive. The closure has a pointer to a property and so on |
I've tried all sorts of variations on this, the only thing that works is the second half the vid, where you unmount immediately. This is a pretty big problem in general, like if the sidebar in question has a deep DOM tree - every unmount causes more and more detached nodes with an ever increasing memory footprint. If anyone knows how to reproduce my example above without a memory leak, it'd make a great blog post! |
Describe the bug
When i use preact with a virtualized list, I found memroy not gabage collected. this may cased by a chrome issue:
The MouseEventManager keeps removed DOM nodes alive until subsequent click https://bugs.chromium.org/p/chromium/issues/detail?id=1177010
if there is no click event for a while , (such as scrolling infinite card list ), the memory usage grows until next click.
The text was updated successfully, but these errors were encountered: