Prevent reactive variables from retaining otherwise unreachable InMemoryCache objects. #7661
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The scripts/memory tests added in PR #7661 fail in Node.js 12, because the FinalizationRegistry (a relatively new ECMAScript feature) is not defined.
benjamn
force-pushed
the
7622-reactive-var-cache-leak
branch
from
a4d6810 to
668711a
Compare
benjamn
commented
Feb 8, 2021
Comment on lines
+41
to
+44
| // If the registry object itself gets garbage collected before the | ||
| // callback fires, the callback might never be called: | ||
| // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry#notes_on_cleanup_callbacks | ||
| registries.push(registry); |
There was a problem hiding this comment.
If you're ever tempted to use a FinalizationRegistry, make sure you don't accidentally let the registry itself get garbage collected, because then any unfired callbacks you've registered will never fire!
benjamn
added a commit
that referenced
this pull request
Feb 12, 2021
Thanks to @francisu for pushing back on the reasoning behind #7276: #7276 (comment) Thanks to #7661, we now have a system for programmatically testing garbage collection of discarded objects, so we can actually enforce the expectation that the whole ObservableQuery is garbage collected after the last subscriber is removed (which tears down the ObservableQuery and removes it from the QueryManager).
This was referenced Mar 14, 2021
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In my investigation of #7622, I found two problems that caused
InMemoryCacheobjects to be retained after they were discarded by application code:If
client.stop()is called while a request is in flight,queryInfo.stop()will be called for eachQueryInfoobject, which successfully cancels any active cache watchers, causing the reactive variable system to release all non-weak references toclient.cache. However, when the request later comes back from the server,queryInfo.markResultis called, which needlessly revives the forgotten cache (see Reactivate reactive variables when InMemoryCache acquires first watcher. #7657) by writing the network result to the cache. This PR fixes that problem by not writing toclient.cacheinmarkResultafterstophas been called.The
varDepdependency function used by the reactive variable system was indirectly retaining references toInMemoryCacheinstances, which was a problem because there was only onevarDepfunction, defined in the module scope ofreactiveVars.ts(and thus never garbage collected). This PR fixes that problem by associating a separate dependency function with each cache object, using aWeakMap(keyed bycache) to hold that information weakly. In other words, the dependency function can now be garbage collected along with the cache object, whenever the cache object becomes unreachable.Although the usage of
WeakMapin this part of the code is not new, and we haven't heard any recent complaints aboutWeakMapbeing unavailable/unusable in older JS environments, it's worth mentioning thatWeakMapis not the only option we have at our disposal. We could also associate metadata with cache objects simply by attaching the metadata as a property of the cache object, which would make the cache the owner of the data, allowing both to be garbage collected at the same time. I don't love that strategy, because it involves modifying the cache object, and potentially exposes the metadata to outside tampering, but it's an option.