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

Potential memory leak in computed #9233

Closed
matej-svejda opened this issue Sep 16, 2023 · 8 comments · Fixed by #5912
Closed

Potential memory leak in computed #9233

matej-svejda opened this issue Sep 16, 2023 · 8 comments · Fixed by #5912

Comments

@matej-svejda
Copy link

matej-svejda commented Sep 16, 2023

Vue version

3.3.4 (all 3.* versions I tested this with were affected)

Link to minimal reproduction

https://play.vuejs.org/#eNqdVMFu2zAM/RXBpxRI7EN3ypKg29AB7aEd2l0G+KLYdKJWlgyJTpoF/vdRkp0oSTsM66Wx+PT4+ERyn3xpmnTTQjJNZrYwokFmAdtmkStRN9og27NC102LUI6ZgWrMthyL9W1VQYGsY5XRNcsTosiTXOWq0MoSh67hcflCkG8SuIGSzd3lUcWlhasBVVBMtQ3FFGzZd6G4FL85Cq2eYCUsmt3oFXZsvmB7okZRsfA9n1PGY4o8uQqAHB2vlpBKvRrFEJ+KZBA0AC8EphsuWyApaKgUwnS56hxYSYjreYKqr0W1Uh5Kifh6twg1GDcaXbkiTkhCPp8gMPCiAGvpVgDHldw/Pz6kZIdQK1HtRpe5erKIzdd7IPPuvJPeOU9VuOhHzrl6t9yeGEjWDHmEEvh8tPkkYZbFvdKbMET+u0C63F19HiTHzlP2PUN4wym1xxqk1GyrjSTNzCn+wICojxxnaMnU+AYEE2kZnzedN+LUgBEdzrIwSDRC9IFQN5Ij0NfMTVWOdxXb6ZZVwpD6xrhHz5Pw+qSUq5LhGhSdecvzZByXKCzNygbM8B6pY/xFdGu+AYa6J+ybia+4UO40tIPAdLY0TknmpPR6ZssWUSt2U0hRvM6PWhbhxywLgPfBvciF/38C3e/pOS4XQce6jrGDhuxgUDJO0NKbVmKVvlitaCVRsxA0cXMkJJjHxu0GUjZ1bcToj8TSM2/v/Zmb3PFwXqyheH3n/MW+ubM8+UFGgdnQ2jrEkJsV0Mu68O3zA3VSFKx12UpC/yX4BNTRrdMYYF9bVZLsCOfV3vnFSr3+096+ISg7FBVWD2Odx/ul6gbgo9KPcq/Ta3+POpJcDIt7UvPmzMcQOCXxi9sPDGJjp1lWlIqulSDFxqQKMFNNnd0QLDOtQlHDpNT1DWVMP2UlzUh8nIKtJ0ujt+QskUSFu/vebjMxoEowzpZ/S3t2LU59FrpIfzCl+wNg0WvC

Steps to reproduce

Click on "access" and then on "clear". Even if you then manually trigger garbage collection in the browser, "someObject" doesn't get removed from memory. You have to click "access" again for this to happen.

What is expected?

That after "clear" is called "someObject" gets garbage collected because there shouldn't be anything referencing it.

What is actually happening?

"someObject" stays in memory until the computed property is accessed again.

System Info

No response

Any additional comments?

This was the cause of a huge memory leak in my application that took me very long to discover.

@Alfred-Skyblue
Copy link
Member

This is not a memory leak. computed properties are cached, so when you modify the variables that a computed property depends on, it doesn't immediately change the computed property's value. In fact, it continues to cache the value from the last time you accessed it until the next get call, at which point it gets recalculated and cached again. During this recalculation, it releases the previously cached value.

if (self._dirty || !self._cacheable) {

@edison1105
Copy link
Member

like @Alfred-Skyblue said above. this is not a memory leak.
as far as I know, vue-devtools will lead to memory leaks.
@matej-svejda Could you test in your application with vue-devtools disabled?

@matej-svejda
Copy link
Author

@Alfred-Skyblue I see, I guess that makes sense from a technical perspective. I'm not sure it's great from an API usage point of view, but I also don't see how to easily improve that.

@edison1105 I had vue-devtools turned off.

@Alfred-Skyblue
Copy link
Member

@johnsoncodehk If we want to solve this problem, we must run the effect every time set is executed, even if get is not triggered. Perhaps we should add a parameter to it to indicate whether it needs lazy loading, like lazy: false.

@johnsoncodehk
Copy link
Member

@Alfred-Skyblue I don't think the author's original problem needs fixing, it's expected behavior, but my playground link shows a different problem, someObject still doesn't get recycled even though computed itself is set to undefined.

@LinusBorg LinusBorg linked a pull request Oct 20, 2023 that will close this issue
@yyx990803
Copy link
Member

I dug a bit more and it seems the case @johnsoncodehk discovered is an edge case.

  • It's true that simply setting the computed to undefined doesn't cause its cached value to be garbage collected.

  • However, if we put the same test case inside a child component and unmount that child component, the computed value is cleanly garbage collected. See this case: https://gist.github.com/yyx990803/5ed0bdcb3ff74dee34de1326acf143e7

    • I wasn't able to cleanly reproduce this in the SFC playground, as there might be other variables at play. A simple HTML file also allows us to more easily capture and analyze memory snapshots.
    • click access first, then click toggle, then force GC, you will see the object is collected. You can also use Memory snapshots to check for the presence of HeavyObject.
    • Note this is using the current released 3.3.7. I've verified it's the same behavior using this PR, but with WeakRef and FinalizationRegistry removed.

I'd argue the 2nd case is far more common (and probably 99.9% of the case) in real world apps. I've never seen anyone manually setting computed to undefined - they are always declared and used as a constant inside components, and expected to be garbage collected when the component is unmounted. If that works as expected, then I don't think we need to resort to FinalizationRegistry for the unlikely usage of computed.

@yyx990803
Copy link
Member

Closing as the original issue is not considered a leak: a leak means the memory has a chance of infinitely growing over time - in this case, there will always only be at most 1 copy of a cached value held, and it is properly released on next access / component unmount.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants