[FEAT] TrackableRequests for when async leakage is detected

improve waiter erroring improve trace infra and add tests only run waiter tests in dev
emberjs · Aug 17, 2018 · bc1bdac · bc1bdac
1 parent db9cf7a
commit bc1bdac
Show file tree

Hide file tree

Showing 9 changed files with 581 additions and 27 deletions.
diff --git a/addon/-legacy-private/system/relationships/state/belongs-to.js b/addon/-legacy-private/system/relationships/state/belongs-to.js
@@ -19,7 +19,8 @@ export default class BelongsToRelationship extends Relationship {
    * true if there is no inverse
    * false if the inverse exists and is not loaded (empty)
    *
-   * @returns {boolean}
+   * @property
+   * @return {boolean}
    */
   get allInverseRecordsAreLoaded() {
     let internalModel = this.inverseInternalModel;

diff --git a/addon/-legacy-private/system/relationships/state/has-many.js b/addon/-legacy-private/system/relationships/state/has-many.js
@@ -33,7 +33,8 @@ export default class ManyRelationship extends Relationship {
    * true if there are no inverse records
    * false if the inverse records exist and any are not loaded (any empty)
    *
-   * @returns {boolean}
+   * @property
+   * @return {boolean}
    */
   get allInverseRecordsAreLoaded() {
     // check currentState for unloaded records

diff --git a/addon/-legacy-private/system/store.js b/addon/-legacy-private/system/store.js
@@ -220,9 +220,52 @@ Store = Service.extend({
     this._serializerCache = Object.create(null);
 
     if (DEBUG) {
-      this.__asyncRequestCount = 0;
+      if (this.shouldTrackAsyncRequests === undefined) {
+        this.shouldTrackAsyncRequests = false;
+      }
+      if (this.generateStackTracesForTrackedRequests === undefined) {
+        this.generateStackTracesForTrackedRequests = false;
+      }
+
+      this._trackedAsyncRequests = [];
+      this._trackAsyncRequestStart = label => {
+        let trace =
+          'set `store.generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated';
+
+        if (this.generateStackTracesForTrackedRequests) {
+          try {
+            throw new Error(`EmberData TrackedRequest: ${label}`);
+          } catch (e) {
+            trace = e;
+          }
+        }
+
+        let token = Object.freeze({
+          label,
+          trace,
+        });
+
+        this._trackedAsyncRequests.push(token);
+        return token;
+      };
+      this._trackAsyncRequestEnd = token => {
+        let index = this._trackedAsyncRequests.indexOf(token);
+
+        if (index === -1) {
+          throw new Error(
+            `Attempted to end tracking for the following request but it was not being tracked:\n${token}`
+          );
+        }
+
+        this._trackedAsyncRequests.splice(index, 1);
+      };
+
       this.__asyncWaiter = () => {
-        return this.__asyncRequestCount === 0;
+        let shouldTrack = this.shouldTrackAsyncRequests;
+        let tracked = this._trackedAsyncRequests;
+        let isSettled = tracked.length === 0;
+
+        return shouldTrack !== true || isSettled;
       };
 
       Ember.Test.registerWaiter(this.__asyncWaiter);
@@ -833,6 +876,24 @@ Store = Service.extend({
       options,
     };
 
+    if (DEBUG) {
+      if (this.generateStackTracesForTrackedRequests === true) {
+        let trace;
+
+        try {
+          throw new Error(`Trace Origin for scheduled fetch for ${modelName}:${id}.`);
+        } catch (e) {
+          trace = e;
+        }
+
+        // enable folks to discover the origin of this findRecord call when
+        // debugging. Ideally we would have a tracked queue for requests with
+        // labels or local IDs that could be used to merge this trace with
+        // the trace made available when we detect an async leak
+        pendingFetchItem.trace = trace;
+      }
+    }
+
     let promise = resolver.promise;
 
     internalModel.loadingData(promise);
@@ -2840,6 +2901,27 @@ Store = Service.extend({
 
     if (DEBUG) {
       Ember.Test.unregisterWaiter(this.__asyncWaiter);
+      let shouldTrack = this.shouldTrackAsyncRequests;
+      let tracked = this._trackedAsyncRequests;
+      let isSettled = tracked.length === 0;
+
+      if (!isSettled) {
+        if (shouldTrack) {
+          throw new Error(
+            'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
+              tracked.map(o => o.label).join('\n\t - ')
+          );
+        } else {
+          warn(
+            'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
+              tracked.map(o => o.label).join('\n\t - '),
+            false,
+            {
+              id: 'ds.async.leak.detected',
+            }
+          );
+        }
+      }
     }
   },
 
@@ -3043,11 +3125,11 @@ function isInverseRelationshipInitialized(store, internalModel, data, key, model
 }
 
 /**
- *
+ * @function
  * @param store
  * @param cache modelFactoryCache
  * @param normalizedModelName already normalized modelName
- * @returns {*}
+ * @return {*}
  */
 function getModelFactory(store, cache, normalizedModelName) {
   let factory = cache[normalizedModelName];

diff --git a/addon/-private/system/store/common.js b/addon/-private/system/store/common.js
@@ -34,14 +34,15 @@ export function _objectIsAlive(object) {
 }
 
 export function guardDestroyedStore(promise, store, label) {
+  let token;
   if (DEBUG) {
-    store.__asyncRequestCount++;
+    token = store._trackAsyncRequestStart(label);
   }
   let wrapperPromise = resolve(promise, label).then(v => promise);
 
   return _guard(wrapperPromise, () => {
     if (DEBUG) {
-      store.__asyncRequestCount--;
+      store._trackAsyncRequestEnd(token);
     }
     return _objectIsAlive(store);
   });

diff --git a/addon/-record-data-private/system/relationships/state/belongs-to.js b/addon/-record-data-private/system/relationships/state/belongs-to.js
@@ -199,7 +199,7 @@ export default class BelongsToRelationship extends Relationship {
    * true if there is no inverse
    * false if the inverse exists and is not loaded (empty)
    *
-   * @returns {boolean}
+   * @return {boolean}
    */
   get allInverseRecordsAreLoaded() {
     let modelData = this.inverseModelData;

diff --git a/addon/-record-data-private/system/relationships/state/has-many.js b/addon/-record-data-private/system/relationships/state/has-many.js
@@ -256,7 +256,7 @@ export default class ManyRelationship extends Relationship {
    * true if there are no inverse records
    * false if the inverse records exist and any are not loaded (any empty)
    *
-   * @returns {boolean}
+   * @return {boolean}
    */
   get allInverseRecordsAreLoaded() {
     // check currentState for unloaded records

diff --git a/addon/-record-data-private/system/store.js b/addon/-record-data-private/system/store.js
@@ -226,9 +226,52 @@ Store = Service.extend({
     this.modelDataWrapper = new ModelDataWrapper(this);
 
     if (DEBUG) {
-      this.__asyncRequestCount = 0;
+      if (this.shouldTrackAsyncRequests === undefined) {
+        this.shouldTrackAsyncRequests = false;
+      }
+      if (this.generateStackTracesForTrackedRequests === undefined) {
+        this.generateStackTracesForTrackedRequests = false;
+      }
+
+      this._trackedAsyncRequests = [];
+      this._trackAsyncRequestStart = label => {
+        let trace =
+          'set `store.generateStackTracesForTrackedRequests = true;` to get a detailed trace for where this request originated';
+
+        if (this.generateStackTracesForTrackedRequests) {
+          try {
+            throw new Error(`EmberData TrackedRequest: ${label}`);
+          } catch (e) {
+            trace = e;
+          }
+        }
+
+        let token = Object.freeze({
+          label,
+          trace,
+        });
+
+        this._trackedAsyncRequests.push(token);
+        return token;
+      };
+      this._trackAsyncRequestEnd = token => {
+        let index = this._trackedAsyncRequests.indexOf(token);
+
+        if (index === -1) {
+          throw new Error(
+            `Attempted to end tracking for the following request but it was not being tracked:\n${token}`
+          );
+        }
+
+        this._trackedAsyncRequests.splice(index, 1);
+      };
+
       this.__asyncWaiter = () => {
-        return this.__asyncRequestCount === 0;
+        let shouldTrack = this.shouldTrackAsyncRequests;
+        let tracked = this._trackedAsyncRequests;
+        let isSettled = tracked.length === 0;
+
+        return shouldTrack !== true || isSettled;
       };
 
       Ember.Test.registerWaiter(this.__asyncWaiter);
@@ -842,6 +885,24 @@ Store = Service.extend({
       options,
     };
 
+    if (DEBUG) {
+      if (this.generateStackTracesForTrackedRequests === true) {
+        let trace;
+
+        try {
+          throw new Error(`Trace Origin for scheduled fetch for ${modelName}:${id}.`);
+        } catch (e) {
+          trace = e;
+        }
+
+        // enable folks to discover the origin of this findRecord call when
+        // debugging. Ideally we would have a tracked queue for requests with
+        // labels or local IDs that could be used to merge this trace with
+        // the trace made available when we detect an async leak
+        pendingFetchItem.trace = trace;
+      }
+    }
+
     let promise = resolver.promise;
 
     internalModel.loadingData(promise);
@@ -3062,6 +3123,27 @@ Store = Service.extend({
 
     if (DEBUG) {
       Ember.Test.unregisterWaiter(this.__asyncWaiter);
+      let shouldTrack = this.shouldTrackAsyncRequests;
+      let tracked = this._trackedAsyncRequests;
+      let isSettled = tracked.length === 0;
+
+      if (!isSettled) {
+        if (shouldTrack) {
+          throw new Error(
+            'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
+              tracked.map(o => o.label).join('\n\t - ')
+          );
+        } else {
+          warn(
+            'Async Request leaks detected. Add a breakpoint here and set `store.generateStackTracesForTrackedRequests = true;`to inspect traces for leak origins:\n\t - ' +
+              tracked.map(o => o.label).join('\n\t - '),
+            false,
+            {
+              id: 'ds.async.leak.detected',
+            }
+          );
+        }
+      }
     }
   },
 
@@ -3231,7 +3313,7 @@ function _commit(adapter, store, operation, snapshot) {
  * @param store
  * @param cache modelFactoryCache
  * @param normalizedModelName already normalized modelName
- * @returns {*}
+ * @return {*}
  */
 function getModelFactory(store, cache, normalizedModelName) {
   let factory = cache[normalizedModelName];

diff --git a/tests/integration/store-test.js b/tests/integration/store-test.js
@@ -1,7 +1,7 @@
-import RSVP, { Promise as EmberPromise, resolve } from 'rsvp';
+import { Promise, resolve } from 'rsvp';
 import { run, next } from '@ember/runloop';
 import setupStore from 'dummy/tests/helpers/store';
-
+import Ember from 'ember';
 import testInDebug from 'dummy/tests/helpers/test-in-debug';
 import deepCopy from 'dummy/tests/helpers/deep-copy';
 import { module, test } from 'qunit';
@@ -76,7 +76,7 @@ test("destroying record during find doesn't cause error", function(assert) {
 
   let TestAdapter = DS.Adapter.extend({
     findRecord(store, type, id, snapshot) {
-      return new EmberPromise((resolve, reject) => {
+      return new Promise((resolve, reject) => {
         next(() => {
           store.unloadAll(type.modelName);
           reject();
@@ -93,31 +93,57 @@ test("destroying record during find doesn't cause error", function(assert) {
   return run(() => store.findRecord(type, id).then(done, done));
 });
 
-test('find calls do not resolve when the store is destroyed', function(assert) {
-  assert.expect(0);
+testInDebug('find calls do not resolve when the store is destroyed', async function(assert) {
+  assert.expect(2);
   let done = assert.async();
-
+  let next;
+  let nextPromise = new Promise(resolve => {
+    next = resolve;
+  });
   let TestAdapter = DS.Adapter.extend({
-    findRecord(store, type, id, snapshot) {
-      store.destroy();
-      resolve(null);
+    findRecord() {
+      next();
+      nextPromise = new Promise(resolve => {
+        next = resolve;
+      }).then(() => {
+        return {
+          data: { type: 'car', id: '1' },
+        };
+      });
+      return nextPromise;
     },
   });
 
   initializeStore(TestAdapter);
 
-  let type = 'car';
-  let id = 1;
+  // needed for LTS 2.16
+  Ember.Test.adapter.exception = e => {
+    throw e;
+  };
 
+  store.shouldTrackAsyncRequests = true;
   store.push = function() {
     assert('The test should have destroyed the store by now', store.get('isDestroyed'));
 
     throw new Error("We shouldn't be pushing data into the store when it is destroyed");
   };
+  store.findRecord('car', '1');
+
+  await nextPromise;
+
+  assert.throws(() => {
+    run(() => store.destroy());
+  }, /Async Request leaks detected/);
 
-  run(() => store.findRecord(type, id));
+  next();
+  await nextPromise;
 
-  setTimeout(() => done(), 500);
+  // ensure we allow the internal store promises
+  // to flush, potentially pushing data into the store
+  setTimeout(() => {
+    assert.ok(true, 'We made it to the end');
+    done();
+  }, 0);
 });
 
 test('destroying the store correctly cleans everything up', function(assert) {
@@ -228,7 +254,7 @@ test('destroying the store correctly cleans everything up', function(assert) {
 
 function ajaxResponse(value) {
   env.adapter.ajax = function(url, verb, hash) {
-    return run(RSVP, 'resolve', deepCopy(value));
+    return run(() => resolve(deepCopy(value)));
   };
 }