Skip to content
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.

Sign up for GitHub

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

[FEAT] TrackableRequests for when async leakage is detected #5545

Merged
merged 1 commit into from
Aug 17, 2018
Merged

[FEAT] TrackableRequests for when async leakage is detected #5545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion addon/-legacy-private/system/relationships/state/belongs-to.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion addon/-legacy-private/system/relationships/state/has-many.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
90 changes: 86 additions & 4 deletions addon/-legacy-private/system/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eventually we should thread this through, and it super supersede the _trackAsyncRequestStart trace

}
}

let promise = resolver.promise;

internalModel.loadingData(promise);
Expand Down Expand Up @@ -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',
}
);
}
}
}
},

Expand Down Expand Up @@ -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];
Expand Down
5 changes: 3 additions & 2 deletions addon/-private/system/store/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 1 addition & 1 deletion addon/-record-data-private/system/relationships/state/belongs-to.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion addon/-record-data-private/system/relationships/state/has-many.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
88 changes: 85 additions & 3 deletions addon/-record-data-private/system/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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',
}
);
}
}
}
},

Expand Down Expand Up @@ -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];
Expand Down
54 changes: 40 additions & 14 deletions tests/integration/store-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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();
Expand All @@ -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) {
Expand Down Expand Up @@ -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)));
};
}

Expand Down
Loading