Skip to content
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

Fix: memory leak on listeners for elements #2244

Merged
merged 5 commits into from Apr 4, 2024

Conversation

jkelleyrtp
Copy link
Member

@jkelleyrtp jkelleyrtp commented Apr 4, 2024

We switched over to Copy types for EventHandlers on elements which is equivalent to calling Signal::new(). This is not necessary for elements, just for child components, and thus caused a memory leak for long-running components.

This PR adds a drop impl to VNode to manually drop listeners when the last VNodeInner is dropped. We can't add the drop impl anywhere else since VNode is the only struct in the chain that can't be destructured, making it semver compatible.

Fix: #2227

@jkelleyrtp jkelleyrtp requested a review from ealmloff April 4, 2024 19:38
Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

I think we should publish this fix in a minor release, but just noting that could break some code using 0.5 if anyone is reading the eventhandler we now drop properly. There isn't any way to fix the leak without making a breaking change like this

@jkelleyrtp jkelleyrtp merged commit 44fe2de into main Apr 4, 2024
6 of 9 checks passed
@jkelleyrtp jkelleyrtp deleted the jk/fix-memory-leak-listeners branch April 4, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EventHandlers on elements are owned by parent, causing long-running memory usage
2 participants