Prevent reactive variables from retaining otherwise unreachable InMemoryCache objects.

.circleci/config.yml

@@ -3,7 +3,7 @@ version: 2
 jobs:
   Filesize:
     docker:
-      - image: circleci/node:12
+      - image: circleci/node:14
     steps:
       - checkout
       - restore_cache:
@@ -26,7 +26,7 @@ jobs:
 
   Tests:
     docker:
-      - image: circleci/node:12
+      - image: circleci/node:14
     steps:
       - checkout
       - restore_cache:

CHANGELOG.md

@@ -1,3 +1,10 @@
+## Apollo Client 3.3.9
+
+### Bug Fixes
+
+- Prevent reactive variables from retaining otherwise unreachable `InMemoryCache` objects. <br/>
+  [@benjamn](https://github.com/benjamn) in [#7661](https://github.com/apollographql/apollo-client/pull/7661)
+
 ## Apollo Client 3.3.8
 
 ### Bug Fixes

package.json

@@ -45,8 +45,10 @@
     "clean": "rimraf -r dist coverage lib",
     "test": "jest --config ./config/jest.config.js",
     "test:debug": "BABEL_ENV=server node --inspect-brk node_modules/.bin/jest --config ./config/jest.config.js --runInBand",
-    "test:ci": "npm run coverage -- --ci --maxWorkers=2 --reporters=default --reporters=jest-junit",
+    "test:ci": "npm run test:coverage && npm run test:memory",
     "test:watch": "jest --config ./config/jest.config.js --watch",
+    "test:memory": "cd scripts/memory && npm i && npm test",
+    "test:coverage": "npm run coverage -- --ci --maxWorkers=2 --reporters=default --reporters=jest-junit",
     "coverage": "jest --config ./config/jest.config.js --verbose --coverage",
     "bundlesize": "npm run build && bundlesize",
     "predeploy": "npm run build",

scripts/memory/README.md

@@ -0,0 +1,11 @@
+# Memory and Garbage Collection tests
+
+This directory contains a few important memory-related tests that were
+difficult to run within the usual Jest environment. Instead, these tests
+run directly in Node.js, importing `@apollo/client` from the `../../dist`
+directory, rather than from `../../src`.
+
+## Running the tests
+
+Run `npm install` in this directory, followed by `npm test`. Failure is
+indicated by a non-zero exit code.

scripts/memory/package.json

@@ -0,0 +1,15 @@
+{
+  "name": "apollo-client-memory-tests",
+  "private": true,
+  "scripts": {
+    "preinstall": "cd ../.. && npm i && npm run build",
+    "test": "mocha --exit --expose-gc tests.js"
+  },
+  "dependencies": {
+    "@apollo/client": "file:../../dist",
+    "graphql": "^15.5.0"
+  },
+  "devDependencies": {
+    "mocha": "^8.2.1"
+  }
+}

scripts/memory/tests.js

@@ -0,0 +1,87 @@
+const assert = require("assert");
+const {
+  ApolloClient,
+  InMemoryCache,
+  gql,
+  makeVar,
+} = require("@apollo/client/core");
+
+function itAsync(message, testFn) {
+  const start = Date.now();
+  let timeout;
+  (function pollGC() {
+    gc(); // enabled by --expose-gc
+    // Passing --exit to mocha should cause the process to exit after
+    // tests pass/fail/timeout, but (in case that fails) we also set a
+    // hard limit of 10 seconds for GC polling.
+    if (Date.now() < start + 10000) {
+      timeout = setTimeout(pollGC, 100);
+    }
+  })();
+  return it(message, () => new Promise(testFn).finally(() => {
+    clearTimeout(timeout);
+  }));
+}
+
+const registries = [];
+function makeRegistry(callback, reject) {
+  assert.strictEqual(typeof callback, "function");
+  assert.strictEqual(typeof reject, "function");
+
+  const registry = new FinalizationRegistry(key => {
+    try {
+      callback(key);
+    } catch (error) {
+      // Exceptions thrown in FinalizationRegistry callbacks can be tricky
+      // for test frameworks to catch, without some help.
+      reject(error);
+    }
+  });
+
+  // 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);
+
+  return registry;
+}
+
+describe("garbage collection", () => {
+  itAsync("should collect client.cache after client.stop()", (resolve, reject) => {
+    const registry = makeRegistry(key => {
+      assert.strictEqual(key, "client.cache");
+      resolve();
+    }, reject);
+
+    const localVar = makeVar(123);
+
+    (function (client) {
+      registry.register(client.cache, "client.cache");
+
+      client.watchQuery({
+        query: gql`query { local }`,
+      }).subscribe({
+        next(result) {
+          assert.deepStrictEqual(result.data, {
+            local: 123,
+          });
+
+          client.stop();
+        },
+      });
+
+    })(new ApolloClient({
+      cache: new InMemoryCache({
+        typePolicies: {
+          Query: {
+            fields: {
+              local() {
+                return localVar();
+              },
+            },
+          },
+        },
+      }),
+    }));
+  });
+});

src/cache/inmemory/reactiveVars.ts

@@ -1,5 +1,5 @@
+import { dep, OptimisticDependencyFunction } from "optimism";
 import { Slot } from "@wry/context";
-import { dep } from "optimism";
 import { InMemoryCache } from "./inMemoryCache";
 import { ApolloCache } from '../../core';
 
@@ -12,8 +12,6 @@ export interface ReactiveVar<T> {
 
 export type ReactiveListener<T> = (value: T) => any;
 
-const varDep = dep<ReactiveVar<any>>();
-
 // Contextual Slot that acquires its value when custom read functions are
 // called in Policies#readField.
 export const cacheSlot = new Slot<ApolloCache<any>>();
@@ -31,11 +29,24 @@ function consumeAndIterate<T>(set: Set<T>, callback: (item: T) => any) {
   }
 }
 
-const varsByCache = new WeakMap<ApolloCache<any>, Set<ReactiveVar<any>>>();
+const cacheInfoMap = new WeakMap<ApolloCache<any>, {
+  vars: Set<ReactiveVar<any>>;
+  dep: OptimisticDependencyFunction<ReactiveVar<any>>;
+}>();
+
+function getCacheInfo(cache: ApolloCache<any>) {
+  let info = cacheInfoMap.get(cache)!;
+  if (!info) {
+    cacheInfoMap.set(cache, info = {
+      vars: new Set,
+      dep: dep(),
+    });
+  }
+  return info;
+}
 
 export function forgetCache(cache: ApolloCache<any>) {
-  const vars = varsByCache.get(cache);
-  if (vars) vars.forEach(rv => rv.forgetCache(cache));
+  getCacheInfo(cache).vars.forEach(rv => rv.forgetCache(cache));
 }
 
 // Calling forgetCache(cache) serves to silence broadcasts and allows the
@@ -47,8 +58,7 @@ export function forgetCache(cache: ApolloCache<any>) {
 // you won't be able to call recallCache(cache), and the cache will
 // automatically disappear from the varsByCache WeakMap.
 export function recallCache(cache: ApolloCache<any>) {
-  const vars = varsByCache.get(cache);
-  if (vars) vars.forEach(rv => rv.attachCache(cache));
+  getCacheInfo(cache).vars.forEach(rv => rv.attachCache(cache));
 }
 
 export function makeVar<T>(value: T): ReactiveVar<T> {
@@ -59,13 +69,15 @@ export function makeVar<T>(value: T): ReactiveVar<T> {
     if (arguments.length > 0) {
       if (value !== newValue) {
         value = newValue!;
-        // First, invalidate any fields with custom read functions that
-        // consumed this variable, so query results involving those fields
-        // will be recomputed the next time we read them.
-        varDep.dirty(rv);
-        // Next, broadcast changes to any caches that have previously read
-        // from this variable.
-        caches.forEach(broadcast);
+        caches.forEach(cache => {
+          // Invalidate any fields with custom read functions that
+          // consumed this variable, so query results involving those
+          // fields will be recomputed the next time we read them.
+          getCacheInfo(cache).dep.dirty(rv);
+          // Broadcast changes to any caches that have previously read
+          // from this variable.
+          broadcast(cache);
+        });
         // Finally, notify any listeners added via rv.onNextChange.
         consumeAndIterate(listeners, listener => listener(value));
       }
@@ -74,8 +86,10 @@ export function makeVar<T>(value: T): ReactiveVar<T> {
       // context via cacheSlot. This isn't entirely foolproof, but it's
       // the same system that powers varDep.
       const cache = cacheSlot.getValue();
-      if (cache) attach(cache);
-      varDep(rv);
+      if (cache) {
+        attach(cache);
+        getCacheInfo(cache).dep(rv);
+      }
     }
 
     return value;
@@ -90,9 +104,7 @@ export function makeVar<T>(value: T): ReactiveVar<T> {
 
   const attach = rv.attachCache = cache => {
     caches.add(cache);
-    let vars = varsByCache.get(cache)!;
-    if (!vars) varsByCache.set(cache, vars = new Set);
-    vars.add(rv);
+    getCacheInfo(cache).vars.add(rv);
     return rv;
   };
 

src/core/QueryInfo.ts

@@ -304,7 +304,7 @@ export class QueryInfo {
     if (options.fetchPolicy === 'no-cache') {
       this.diff = { result: result.data, complete: true };
 
-    } else if (allowCacheWrite) {
+    } else if (!this.stopped && allowCacheWrite) {
       if (shouldWriteResult(result, options.errorPolicy)) {
         // Using a transaction here so we have a chance to read the result
         // back from the cache before the watch callback fires as a result