-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
AC3: Memory leak in watchQuery
retains context
object in memory even after unsubscribe
#7149
Comments
Updated the reproduction with the heap snapshot file (https://github.com/dotansimha/apollo-very-pessimism#heap-snapshot) |
We managed to fix the leak by patching In diff --git a/node_modules/optimism/lib/bundle.esm.js b/node_modules/optimism/lib/bundle.esm.js
index 99c5962..546fa35 100644
--- a/node_modules/optimism/lib/bundle.esm.js
+++ b/node_modules/optimism/lib/bundle.esm.js
@@ -510,6 +510,10 @@ function wrap(originalFunction, options) {
return entry.peek();
}
};
+ optimistic.delete = function (key) {
+ const k = makeCacheKey(key);
+ cache.delete(k)
+ }
return optimistic;
} In diff --git a/node_modules/@apollo/client/cache/inmemory/inMemoryCache.js b/node_modules/@apollo/client/cache/inmemory/inMemoryCache.js
index b421d3a..6618215 100644
--- a/node_modules/@apollo/client/cache/inmemory/inMemoryCache.js
+++ b/node_modules/@apollo/client/cache/inmemory/inMemoryCache.js
@@ -129,6 +129,8 @@ var InMemoryCache = (function (_super) {
this.maybeBroadcastWatch(watch);
}
return function () {
+ _this.watchDep.dirty(watch);
+ _this.maybeBroadcastWatch.delete(watch);
_this.watches.delete(watch);
};
}; Above changes are related to #7086. Cleaning up the records in |
Inspired by @kamilkisiela's comment: #7149 (comment) Should fix #7149 and #7086.
Inspired by @kamilkisiela's comment: #7149 (comment) Should fix #7149 and #7086.
Inspired by @kamilkisiela's comment: #7149 (comment) Should fix #7149 and #7086.
Thanks to @dotansimha's reproduction and @kamilkisiela's proposed solution, I'm confident this leak is fixed in |
@dotansimha As a side note, the weak reference approach you used in your reproduction (using |
Thank you @benjamn ! @kamilkisiela you found something else with mutations that throws error, right? Maybe these tests can be wrapped and move to this repo? just to make sure CI does that as well and we are not experiencing any regression. We have a few thoughts around that, we can help with that. Let me know how that's sounds, we can schedule a meeting for that :) |
@dotansimha yes, quick LARGE coffee and then I'm going to create a reproduction with a fix. |
Intended outcome:
Object passed as
context
towatchQuery
method should be released whenunsubscribe
is called, instead of being retrained in memory.I'm opening this because this seems like another symptom of a related issue, but with a testable reproduction, and including the heap snapshots files.
I believe this is very related to what @Torsten85 posted here: #6985 (comment)
Actual outcome:
context
object are retained in memory by AC3:How to reproduce the issue:
Use
watchQuery
withcontext
set. Apollo keeps reference to that object even if query has done, andunsubscribe
has been called (and all references has been destructed).You can find a complete reproduction (as a Jest test + instructions) here:
https://github.com/dotansimha/apollo-very-pessimism
Versions
>=3
The text was updated successfully, but these errors were encountered: