Memory leak due to key dependencies never being removed

[GrantGryczan]

Vue version

3.3.4

Link to minimal reproduction

https://play.vuejs.org/#eNqNU01v2zAM/SuCLnWQxM7QndK06Fbk0AH7wLqjL5pMx+psSZDkJEDg/z5Kqh3XDYoKMESTj3yPNH2iX7RO9y3QNd1YboR2xIJr9V1ucikarYwjJ2KAcSf2sECrXJADc7zaliVwRzpSGtWQK6xxdeOT/MOVtKEQuR1yEwkH8gQumc0CLmK4amVElckqBvwzYkiSGbm9IyfvJnhCRrpndQsB7n39XSpDkhppBZZc3eC1IZ9WeNCcz2fnIv6gvLRiNvnOXJUaJgvV9Np8uPNGd1mSkg81MNnqiTLfkZAODMpDBcjw+PI27WLSyXx+5j1T+teB6mIFDJmBomce9RDNl2uTxS8cvi1dUGdRcSl26bNVEhfg5JNyylWjRQ3mp3YCO8rpGknjmCmra3X4FnzOtLDo/bwC/u+C/9kevS+nvwxYFAc5HWKOmR24GN4+/YAj2kOwUUVbI/qd4G+wqm69xgj72soCZY9wQe1jWGIhd3/s9uhA2r4pL9Qju4DPKW7wwzutn+Vep9chDweLU4w/ybJhejLHGHhdBDmi2Mo5bddZxguJaQXUYm9SCS6TusnuEZYZXA7RwBLX8h4Z089ZIawbu1OwzfKvUQecLBYZNe7zw7jN0oAswPixfIx2kjamnoTe0A9D6f4D9x1umQ==

Steps to reproduce

Inside the callback of a reactivity hook (e.g. watchEffect, computed), read a reactive value from a key-based data structure (e.g. set.has(key), map.get(key), object[key]).

Then allow it to rerun an arbitrarily large number of times with different keys.

What is expected?

I would expect this to not result in arbitrarily large memory usage.

One solution worth considering is that a dependency should be removed as soon as the callback runs the next time and no longer uses the dependency from its previous run. But if this can be demonstrated to necessarily result in other performance drawbacks (e.g. every key-based access becoming O(n) rather than the semantically expected O(1)), then that solution may not be viable.

Ideally, it should additionally be ensured that, even if the callback never runs again (thus never expiring the unused deps), the memory for the unused deps is eventually freed by the time the callback (or computed ref) is garbage-collected, but I understand this may not be possible in JS (at least without very complex compile-time behavior).

What is actually happening?

In this minimal reproduction, one of two things eventually occurs after some time (depending on various factors):

1. The page runs out of memory and crashes.

2. The following error occurs due to the reactive value having too many deps that are never cleaned up:

RangeError: Map maximum size exceeded
    at Map.set (<anonymous>)
    at track (vue.runtime.esm-browser.js:501:15)
    at Proxy.has (vue.runtime.esm-browser.js:821:5)
    at <anonymous>:26:13
    at callWithErrorHandling (vue.runtime.esm-browser.js:1550:18)
    at callWithAsyncErrorHandling (vue.runtime.esm-browser.js:1558:17)
    at ReactiveEffect.getter [as fn] (vue.runtime.esm-browser.js:3092:16)
    at ReactiveEffect.run (vue.runtime.esm-browser.js:428:19)
    at job (vue.runtime.esm-browser.js:3134:14)
    at callWithErrorHandling (vue.runtime.esm-browser.js:1550:32)

System Info

No response

Any additional comments?

As a side note, when developing in Vue I often find that I have to think about memory leaks, which I really don't enjoy since how Vue's reactivity system manages memory should be an abstracted implementation detail. I often manually test if various things result in memory leaks, and this is one which did. This is one of the (few!) things I dislike about Vue: frequently having to reason about implementation details. In this case, I have to think about whether reactive values, callbacks, and/or dependencies can be garbage-collected after certain conditions occur. I'm not sure if Vue can be blamed for this problem (whether it's a problem inherent to Vue's design or to JS), but I figured it was worth noting that I believe it is a problem that has a significant negative impact on Vue's DX in my experience.

I ran into this because we generate new IDs client-side and read them from various reactive sets and maps. Our clients can easily trigger this by copying and pasting a large selection of items in the UI, for example, generating large amounts of new IDs for the new items, which Vue internally adds as reactive dependencies. Even if they undo their paste, the dependencies for the new IDs stay in memory. It doesn't help that our application is a SPA.

And even if this happens not to be a significant problem for our particular use case, the same cannot necessarily be said in general, and this is still an issue which fixing would reduce the mental model required to use Vue's reactivity. I don't want to constantly have to think about whether memory leaks are a problem just because I'm using a reactive key-based data structure in Vue. And not to mention, I never want to have to comment why certain code is necessary to avoid these memory leaks!

[LinusBorg]

This is not a "memory leak", you just created millions of (valid) deps for a single set's has method. And all of these deps are required, since the effect should be re-run as soon as you do set.add("an existing dep") somewhere else in the code. So as long as that effect exists, we need to keep these deps, and they can't be "cleaned up".

Vue has no way of realizing that these deps are no longer required in the context of your business logic. How would it know that 1) the user removed the list of ids that they pasted and 2) interpret that as guaranteeding that "these ids will not reappear in the set later in that effect's lifecycle"?

It's however a great demo of the overhead that fine-grained reactivity such as ours comes with, and a reminder that there are edge cases, usually involving large data sets, where the overhead is becoming a problem and switching to an immutable data approach makes sense.

I'd be interested in better understanding how this happened in an actual app. Did you in fact hit that RangeError in a production scenario or is this just a result of this demo (max size of a Map is upwards of 16-20 Million entries)?

I'd also be interested in examples for those other types of situations where you need to think about implementation details because of memory.

Maybe we need a way to manually reset an effect's deps for certain edge cases? Not sure on the spot.

[LinusBorg]

(accidentally delete my comment so here's a re-write)

Thinking further about this, I think we might have a potential for optimzation when it comes to Map keys / Set content that are objects.

My previous explanation holds true for keys that are primitive types, but if we look at objects, those could be held in memory unnecessily if the only think keeping them from being garbage collected is the fact that they serve as a key in a reactive Map's depsMap.

Technically, I think it should be feasable to track Map keys that are objects in a WeakMap instead of a normal Map?
@johnsoncodehk maybe you have thoughts on this as you went quite deep into reactivity optimizations recently.

[skirtles-code]

@LinusBorg Perhaps I've misunderstood, but I think the scenario you're describing involving object keys is the same problem I tried to fix in #7827. I will need to redo that PR once Johnson's reactivity optimizations are merged.

[LinusBorg]

Yep, that seems to be exactly it.

[johnsoncodehk]

I solved this problem by the way in 93b5beb (#5912) (have added @skirtles-code as co-author due to directly using the tests he wrote)

[GrantGryczan]

This is not a "memory leak", you just created millions of (valid) deps for a single set's has method. And all of these deps are required, since the effect should be re-run as soon as you do set.add("an existing dep") somewhere else in the code. So as long as that effect exists, we need to keep these deps, and they can't be "cleaned up".

@LinusBorg Perhaps the deps are required given the way Vue is designed currently, but in my opinion that ideally shouldn't be necessary. I'll use an analogy to attempt to justify this: Suppose an effect watches ref A on its first run. Then ref A's value changes, so it reruns, but in its second run it doesn't read the value of ref A and instead only reads the value of ref B. Then ref A's value changes again. This does not trigger the effect to rerun, and ref A has no deps (at least I hope--haven't tested).

The same is true of values in a set or keys in a map/object for example. If it watched one key in a previous run but no longer reads that key in a later run, changing the value at the key that was read in the old run does absolutely nothing--it doesn't rerun the effect. (I tested this!) So the dep is there for no reason.

Hope that makes my perspective clearer!

I'd be interested in better understanding how this happened in an actual app. Did you in fact hit that RangeError in a production scenario or is this just a result of this demo (max size of a Map is upwards of 16-20 Million entries)?

No, I didn't hit any memory or object limit in my actual app, I only ran into the existence of this memory leak. I set up the minimal reproduction to make it easy to see the memory blow up (i.e. doing 10,000 iterations on every effect rerun, and rerunning under setInterval), so incidentally that causes either limit to be reached after only a short time. Theoretically someone could reach that limit in my app, but to do it would take either a script or a lot of time and dedication, haha. I go into the details of how this relates to my app in the last 2 paragraphs of my issue.

My previous explanation holds true for keys that are primitive types, but if we look at objects, those could be held in memory unnecessily if the only think keeping them from being garbage collected is the fact that they serve as a key in a reactive Map's depsMap.

That would be great to fix! But it still wouldn't address the case of primitive keys, which hopefully I've convinced you is an issue in this comment's first section. If not then let me know why!

[LinusBorg]

Yeah, I'm actually already convinced since looking at @skirtles-code 's fix, which would do what you ask for, for object keys and normal string based ones. Seems my mental model was off.