Merge pull request #18717 from rwjblue/container-cleanup-race-oh-my

[BUGFIX lts] Ensure instantiation cannot happen after destruction.
emberjs · Jan 31, 2020 · 522c2d0 · 522c2d0
2 parents 2c5b05b + 298a086
commit 522c2d0
Show file tree

Hide file tree

Showing 2 changed files with 136 additions and 17 deletions.
diff --git a/packages/@ember/-internals/container/lib/container.ts b/packages/@ember/-internals/container/lib/container.ts
@@ -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';
@@ -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);
   }
@@ -161,8 +163,9 @@ export default class Container {
    @method destroy
    */
   destroy(): void {
-    destroyDestroyables(this);
     this.isDestroying = true;
+
+    destroyDestroyables(this);
   }
 
   finalizeDestroy(): void {
@@ -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));
@@ -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 }
@@ -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);

diff --git a/packages/@ember/-internals/container/tests/container_test.js b/packages/@ember/-internals/container/tests/container_test.js
@@ -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
@@ -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\)/);
+    }
   }
 );