Skip to content

Commit

Permalink
[BUGFIX lts] Ensure instantiation cannot happen after destruction.
Browse files Browse the repository at this point in the history
This changes a bit about how destruction occurs and is enforced. Without
these changes, it is possible to introduce memory leaks in FastBoot in
a few (somewhat specialized) scenarios. Some examples:

* if you were to call `owner.lookup` sometime after the owner itself was
  destroyed.  During development and tests we prevent this (by way of an
  assertion), but in production we happily recreate the instances
  (without marking them as `isDestroying`) and never destroy them
* if you were to call `owner.lookup` _during_ destruction (e.g. in
  another services `willDestroy`) we would emit no warning / assertion
  even in development mode, and we would never destroy the instances
  created

In order to prevent these situations, this commit introduces these
changes:

1. ensure that looking things up from the container _during_
   destruction emits a deprecation
2. ensure that looking things up from the container _after_ destruction
   throws an error (**not** an assertion)
3. ensure that any objects that _are_ instantiated during container
   destruction (even though deprecated) will themselves be destroyed

(cherry picked from commit 298a086)
  • Loading branch information
rwjblue committed Jan 31, 2020
1 parent 2ef87f8 commit 645dab5
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 17 deletions.
41 changes: 36 additions & 5 deletions packages/@ember/-internals/container/lib/container.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Factory, LookupOptions, Owner, OWNER, setOwner } from '@ember/-internals/owner';
import { dictionary, HAS_NATIVE_PROXY } from '@ember/-internals/utils';
import { EMBER_MODULE_UNIFICATION } from '@ember/canary-features';
import { assert } from '@ember/debug';
import { assert, deprecate } from '@ember/debug';
import { assign } from '@ember/polyfills';
import { DEBUG } from '@glimmer/env';
import Registry, { DebugRegistry, Injection } from './registry';
Expand Down Expand Up @@ -149,7 +149,9 @@ export default class Container {
@return {any}
*/
lookup(fullName: string, options: LookupOptions): any {
assert('expected container not to be destroyed', !this.isDestroyed);
if (this.isDestroyed) {
throw new Error(`Can not call \`.lookup\` after the owner has been destroyed`);
}
assert('fullName must be a proper full name', this.registry.isValidFullName(fullName));
return lookup(this, this.registry.normalize(fullName), options);
}
Expand All @@ -161,8 +163,9 @@ export default class Container {
@method destroy
*/
destroy(): void {
destroyDestroyables(this);
this.isDestroying = true;

destroyDestroyables(this);
}

finalizeDestroy(): void {
Expand Down Expand Up @@ -211,7 +214,9 @@ export default class Container {
@return {any}
*/
factoryFor<T, C>(fullName: string, options: LookupOptions = {}): Factory<T, C> | undefined {
assert('expected container not to be destroyed', !this.isDestroyed);
if (this.isDestroyed) {
throw new Error(`Can not call \`.factoryFor\` after the owner has been destroyed`);
}
let normalizedName = this.registry.normalize(fullName);

assert('fullName must be a proper full name', this.registry.isValidFullName(normalizedName));
Expand Down Expand Up @@ -397,7 +402,17 @@ function instantiateFactory(
// SomeClass { singleton: true, instantiate: true } | { singleton: true } | { instantiate: true } | {}
// By default majority of objects fall into this case
if (isSingletonInstance(container, fullName, options)) {
return (container.cache[normalizedName] = factoryManager.create());
let instance = (container.cache[normalizedName] = factoryManager.create());

// if this lookup happened _during_ destruction (emits a deprecation, but
// is still possible) ensure that it gets destroyed
if (container.isDestroying) {
if (typeof instance.destroy === 'function') {
instance.destroy();
}
}

return instance;
}

// SomeClass { singleton: false, instantiate: true }
Expand Down Expand Up @@ -561,6 +576,22 @@ class FactoryManager<T, C> {
}

create(options?: { [prop: string]: any }) {
let { container } = this;

if (container.isDestroyed) {
throw new Error(
`Can not create new instances after the owner has been destroyed (you attempted to create ${this.fullName})`
);
}

if (DEBUG) {
deprecate(
`Instantiating a new instance of ${this.fullName} while the owner is being destroyed is deprecated.`,
!container.isDestroying,
{ id: 'container.lookup-on-destroy', until: '3.20.0' }
);
}

let injectionsCache = this.injections;
if (injectionsCache === undefined) {
let { injections, isDynamic } = injectionsFor(this.container, this.normalizedName);
Expand Down
112 changes: 100 additions & 12 deletions packages/@ember/-internals/container/tests/container_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -680,37 +680,37 @@ moduleFor(
[`@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');
registry.register('service:foo', factory());

let instance = container.lookup('service:foo');
assert.ok(instance, 'precond lookup successful');

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

expectAssertion(() => {
container.lookup('component:foo-bar');
});
assert.throws(() => {
container.lookup('service:foo');
}, /Can not call `.lookup` after the owner has been destroyed/);
}

[`@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');
registry.register('service:foo', factory());

let instance = container.lookup('service:foo');
assert.ok(instance, 'precond lookup successful');

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

expectAssertion(() => {
container.factoryFor('component:foo-bar');
});
assert.throws(() => {
container.factoryFor('service:foo');
}, /Can not call `.factoryFor` after the owner has been destroyed/);
}

// this is skipped until templates and the glimmer environment do not require `OWNER` to be
Expand All @@ -735,6 +735,94 @@ moduleFor(
// not via registry/container shenanigans
assert.deepEqual(Object.keys(instance), []);
}

'@test instantiating via container.lookup during destruction emits a deprecation'(assert) {
let registry = new Registry();
let container = registry.container();
class Service extends factory() {
destroy() {
expectDeprecation(() => {
container.lookup('service:other');
}, /Instantiating a new instance of service:other while the owner is being destroyed is deprecated/);
}
}
registry.register('service:foo', Service);
registry.register('service:other', factory());
let instance = container.lookup('service:foo');
assert.ok(instance, 'precond lookup successful');

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

'@test instantiating via container.lookup during destruction enqueues destruction'(assert) {
let registry = new Registry();
let container = registry.container();
let otherInstance;
class Service extends factory() {
destroy() {
expectDeprecation(() => {
otherInstance = container.lookup('service:other');
}, /Instantiating a new instance of service:other while the owner is being destroyed is deprecated/);

assert.ok(otherInstance.isDestroyed, 'service:other was destroyed');
}
}
registry.register('service:foo', Service);
registry.register('service:other', factory());
let instance = container.lookup('service:foo');
assert.ok(instance, 'precond lookup successful');

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

'@test instantiating via container.factoryFor().create() during destruction emits a deprecation'(
assert
) {
let registry = new Registry();
let container = registry.container();
class Service extends factory() {
destroy() {
expectDeprecation(() => {
let Factory = container.factoryFor('service:other');
Factory.create();
}, /Instantiating a new instance of service:other while the owner is being destroyed is deprecated/);
}
}
registry.register('service:foo', Service);
registry.register('service:other', factory());
let instance = container.lookup('service:foo');
assert.ok(instance, 'precond lookup successful');

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

'@test instantiating via container.factoryFor().create() after destruction throws an error'(
assert
) {
let registry = new Registry();
let container = registry.container();
registry.register('service:foo', factory());
registry.register('service:other', factory());
let Factory = container.factoryFor('service:other');

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

assert.throws(() => {
Factory.create();
}, /Can not create new instances after the owner has been destroyed \(you attempted to create service:other\)/);
}
}
);

Expand Down

0 comments on commit 645dab5

Please sign in to comment.