perf: fix: eliminate SSR/Test memory leak (#8436)

* perf: fix: eliminate SSR/Test memory leak * more leak catching * more infra * fixup all the leaks * fix lint * fix tsc
emberjs · Feb 27, 2023 · d279c7d · d279c7d
1 parent 58fd04a
commit d279c7d
Show file tree

Hide file tree

Showing 10 changed files with 38 additions and 9 deletions.
diff --git a/packages/model/addon/-private/model.js b/packages/model/addon/-private/model.js
@@ -38,6 +38,7 @@ export function lookupLegacySupport(record) {
   let support = LEGACY_SUPPORT.get(identifier);
 
   if (!support) {
+    assert(`Memory Leak Detected`, !record.isDestroyed && !record.isDestroying);
     support = new LegacySupport(record);
     LEGACY_SUPPORT.set(identifier, support);
     LEGACY_SUPPORT.set(record, support);

diff --git a/packages/record-data/addon/-private/graph/graph.ts b/packages/record-data/addon/-private/graph/graph.ts
@@ -410,6 +410,15 @@ export class Graph {
 
     if (DEBUG) {
       Graphs.delete(getStore(this.store) as unknown as RecordDataStoreWrapper);
+      if (Graphs.size) {
+        Graphs.forEach((_, key) => {
+          assert(
+            `Memory Leak Detected, likely the test or app instance previous to this was not torn down properly`,
+            // @ts-expect-error
+            !key.isDestroyed && !key.isDestroying
+          );
+        });
+      }
     }
 
     this.identifiers.clear();

diff --git a/packages/record-data/addon/-private/graph/index.ts b/packages/record-data/addon/-private/graph/index.ts
@@ -38,6 +38,10 @@ export function graphFor(store: RecordDataStoreWrapper | Store): Graph {
 
     // in DEBUG we attach the graph to the main store for improved debuggability
     if (DEBUG) {
+      // @ts-expect-error
+      if (getStore(wrapper).isDestroying) {
+        throw new Error(`Memory Leak Detected During Teardown`);
+      }
       Graphs.set(getStore(wrapper) as unknown as RecordDataStoreWrapper, graph);
     }
   }

diff --git a/packages/record-data/addon/-private/record-data.ts b/packages/record-data/addon/-private/record-data.ts
@@ -19,7 +19,7 @@ import { Dict } from '@ember-data/types/q/utils';
 
 import { LocalRelationshipOperation } from './graph/-operations';
 import { isImplicit } from './graph/-utils';
-import { graphFor } from './graph/index';
+import { graphFor, peekGraph } from './graph/index';
 import type BelongsToRelationship from './relationships/state/belongs-to';
 import type ManyRelationship from './relationships/state/has-many';
 
@@ -322,7 +322,7 @@ export default class SingletonRecordData implements RecordData {
   unloadRecord(identifier: StableRecordIdentifier): void {
     const cached = this.__peek(identifier);
     const storeWrapper = this.__storeWrapper;
-    graphFor(storeWrapper).unload(identifier);
+    peekGraph(storeWrapper)?.unload(identifier);
 
     // effectively clearing these is ensuring that
     // we report as `isEmpty` during teardown.
@@ -630,8 +630,8 @@ function _directlyRelatedRecordDatasIterable(
   storeWrapper: RecordDataStoreWrapper,
   originating: StableRecordIdentifier
 ) {
-  const graph = graphFor(storeWrapper);
-  const initializedRelationships = graph.identifiers.get(originating);
+  const graph = peekGraph(storeWrapper);
+  const initializedRelationships = graph?.identifiers.get(originating);
 
   if (!initializedRelationships) {
     return EMPTY_ITERATOR;

diff --git a/packages/store/addon/-private/caches/instance-cache.ts b/packages/store/addon/-private/caches/instance-cache.ts
@@ -37,7 +37,7 @@ import { assertIdentifierHasId } from '../store-service';
 import coerceId, { ensureStringId } from '../utils/coerce-id';
 import constructResource from '../utils/construct-resource';
 import normalizeModelName from '../utils/normalize-model-name';
-import { removeRecordDataFor, setRecordDataFor } from './record-data-for';
+import { RecordDataForIdentifierCache, removeRecordDataFor, setRecordDataFor } from './record-data-for';
 
 let _peekGraph: peekGraph;
 if (HAS_RECORD_DATA_PACKAGE) {
@@ -237,6 +237,10 @@ export class InstanceCache {
     let record = this.__instances.record.get(identifier);
 
     if (!record) {
+      assert(
+        `Cannot create a new record instance while the store is being destroyed`,
+        !this.store.isDestroying && !this.store.isDestroyed
+      );
       const recordData = this.getRecordData(identifier);
 
       record = this.store.instantiateRecord(
@@ -693,3 +697,9 @@ function _isLoading(cache: InstanceCache, identifier: StableRecordIdentifier): b
     req.getPendingRequestsForRecord(identifier).some((req) => req.type === 'query')
   );
 }
+
+export function _clearCaches() {
+  RecordCache.clear();
+  StoreMap.clear();
+  RecordDataForIdentifierCache.clear();
+}
diff --git a/packages/store/addon/-private/caches/record-data-for.ts b/packages/store/addon/-private/caches/record-data-for.ts
@@ -9,7 +9,7 @@ import type { RecordInstance } from '@ember-data/types/q/record-instance';
  * Model or Identifier
  */
 
-const RecordDataForIdentifierCache = new Map<StableRecordIdentifier | RecordInstance, RecordData>();
+export const RecordDataForIdentifierCache = new Map<StableRecordIdentifier | RecordInstance, RecordData>();
 
 export function setRecordDataFor(identifier: StableRecordIdentifier | RecordInstance, recordData: RecordData): void {
   assert(

diff --git a/packages/store/addon/-private/index.ts b/packages/store/addon/-private/index.ts
@@ -57,4 +57,5 @@ export { default as RecordArrayManager, fastPush } from './managers/record-array
 export { default as SnapshotRecordArray } from './network/snapshot-record-array';
 
 // leaked for private use / test use, should investigate removing
+export { _clearCaches } from './caches/instance-cache';
 export { default as recordDataFor, removeRecordDataFor } from './caches/record-data-for';
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
diff --git a/tests/main/package.json b/tests/main/package.json
@@ -101,7 +101,6 @@
     "ember-data": "workspace:4.8.7",
     "ember-decorators-polyfill": "^1.1.5",
     "ember-disable-prototype-extensions": "^1.1.3",
-    "ember-export-application-global": "^2.0.1",
     "ember-inflector": "^4.0.2",
     "ember-load-initializers": "^2.1.2",
     "ember-maybe-import-regenerator": "^1.0.0",

diff --git a/tests/main/tests/unit/model-test.js b/tests/main/tests/unit/model-test.js
@@ -12,6 +12,7 @@ import Model, { attr, attr as DSattr } from '@ember-data/model';
 import JSONSerializer from '@ember-data/serializer/json';
 import JSONAPISerializer from '@ember-data/serializer/json-api';
 import { recordIdentifierFor } from '@ember-data/store';
+import { _clearCaches } from '@ember-data/store/-private';
 import testInDebug from '@ember-data/unpublished-test-infra/test-support/test-in-debug';
 
 module('unit/model - Model', function (hooks) {
@@ -225,6 +226,9 @@ module('unit/model - Model', function (hooks) {
           },
         });
       }, /You may not set 'id' as an attribute on your model/);
+
+      store.unloadAll();
+      _clearCaches();
     });
 
     test(`a collision of a record's id with object function's name`, async function (assert) {
@@ -626,6 +630,9 @@ module('unit/model - Model', function (hooks) {
           },
           'We throw for legacy-style classes'
         );
+
+        store.unloadAll();
+        _clearCaches();
       });
     }