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.

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

[BUGFIX lts] Ensure instantiation cannot happen after destruction. #18717

Merged
merged 1 commit into from
Jan 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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