Skip to content

Commit

Permalink
[BUGFIX release] Use resetCache on container destroy to help reduce
Browse files Browse the repository at this point in the history
noise when a container is leaked.

Add asserts if lookup and factoryFor are called after destroy.
  • Loading branch information
krisselden committed May 2, 2018
1 parent 7d494bb commit 5e74fc3
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 2 deletions.
6 changes: 5 additions & 1 deletion packages/container/lib/container.js
Expand Up @@ -84,6 +84,7 @@ export default class Container {
@return {any}
*/
lookup(fullName, options) {
assert('expected container not to be destroyed', !this.isDestroyed);
assert('fullName must be a proper full name', this.registry.isValidFullName(fullName));
return lookup(this, this.registry.normalize(fullName), options);
}
Expand All @@ -95,7 +96,7 @@ export default class Container {
@method destroy
*/
destroy() {
destroyDestroyables(this);
resetCache(this);
this.isDestroyed = true;
}

Expand All @@ -107,6 +108,7 @@ export default class Container {
@param {String} fullName optional key to reset; if missing, resets everything
*/
reset(fullName) {
if (this.isDestroyed) return;
if (fullName === undefined) {
resetCache(this);
} else {
Expand Down Expand Up @@ -138,6 +140,7 @@ export default class Container {
@return {any}
*/
factoryFor(fullName, options = {}) {
assert('expected container not to be destroyed', !this.isDestroyed);

This comment has been minimized.

Copy link
@bendemboski

bendemboski May 4, 2018

Contributor

FYI, this line is now causing this issue. I'm not sure if it's something ember-data is doing wrong, or if this assertion is overly aggressive.

let normalizedName = this.registry.normalize(fullName);

assert('fullName must be a proper full name', this.registry.isValidFullName(normalizedName));
Expand All @@ -156,6 +159,7 @@ export default class Container {
return factoryFor(this, normalizedName, fullName);
}
}

/*
* Wrap a factory manager in a proxy which will not permit properties to be
* set on the manager.
Expand Down
34 changes: 34 additions & 0 deletions packages/container/tests/container_test.js
Expand Up @@ -671,6 +671,40 @@ moduleFor(
assert.deepEqual(Object.keys(instance), []);
}

[`@test assert when calling lookup after destroy on a container`](assert) {
let registry = new Registry();
let container = registry.container();
let Component = factory();
registry.register('component:foo-bar', Component);
let instance = container.lookup('component:foo-bar');
assert.ok(instance, 'precond lookup successful');

this.runTask(() => {
container.destroy();
});

expectAssertion(() => {
container.lookup('component:foo-bar');
});
}

[`@test assert when calling factoryFor after destroy on a container`](assert) {
let registry = new Registry();
let container = registry.container();
let Component = factory();
registry.register('component:foo-bar', Component);
let instance = container.factoryFor('component:foo-bar');
assert.ok(instance, 'precond lookup successful');

this.runTask(() => {
container.destroy();
});

expectAssertion(() => {
container.factoryFor('component:foo-bar');
});
}

// this is skipped until templates and the glimmer environment do not require `OWNER` to be
// passed in as constructor args
['@skip #factoryFor does not add properties to the object being instantiated'](assert) {
Expand Down
Expand Up @@ -16,6 +16,7 @@ class LifeCycleHooksTest extends RenderingTest {
this.components = {};
this.componentRegistry = [];
this.teardownAssertions = [];
this.viewRegistry = this.owner.lookup('-view-registry:main');
}

afterEach() {
Expand Down Expand Up @@ -59,7 +60,7 @@ class LifeCycleHooksTest extends RenderingTest {
}

assertRegisteredViews(label) {
let viewRegistry = this.owner.lookup('-view-registry:main');
let viewRegistry = this.viewRegistry;
let topLevelId = getViewId(this.component);
let actual = Object.keys(viewRegistry)
.sort()
Expand Down

0 comments on commit 5e74fc3

Please sign in to comment.