Apollo-client: AC3: Memory leak in `watchQuery` retains `context` object in memory even after `unsubscribe`

[dotansimha]

Intended outcome:

Object passed as context to watchQuery method should be released when unsubscribe 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: https://github.com/apollographql/apollo-client/issues/6985#issuecomment-701240849

Actual outcome:
context object are retained in memory by AC3:

How to reproduce the issue:
Use watchQuery with context set. Apollo keeps reference to that object even if query has done, and unsubscribe 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

[dotansimha]

Updated the reproduction with the heap snapshot file (https://github.com/dotansimha/apollo-very-pessimism#heap-snapshot)

[kamilkisiela]

We managed to fix the leak by patching optimism and @apollo/client packages (patches).

In optimism:

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 @apollo/client:

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 dep and wrap functions of optimism when watch is completed makes sure it doesn't hold a reference to QueryInfo => ObservableQuery => ObservableQuery.options => options.context anymore.

[benjamn]

Thanks to @dotansimha's reproduction and @kamilkisiela's proposed solution, I'm confident this leak is fixed in @apollo/[email protected] and @apollo/[email protected]. Thanks a ton!

[benjamn]

@dotansimha As a side note, the weak reference approach you used in your reproduction (using weak-napi) is 🤯 —definitely using that the next time I need to write a memory leak regression test that runs in Node!

[dotansimha]

Thank you @benjamn !
Btw, you can take a look at the rest of the branches in my reproduction, we added more use cases (watchQuery + mutation that effects it, subscription and some more).

@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 :)

[kamilkisiela]

@dotansimha yes, quick LARGE coffee and then I'm going to create a reproduction with a fix.