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 release] Fix container destroy timing #16754

Merged
merged 3 commits into from
Jun 20, 2018
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
9 changes: 8 additions & 1 deletion packages/container/lib/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,15 @@ export default class Container {
factoryManagerCache!: { [key: string]: FactoryManager<any, any> };
readonly validationCache!: { [key: string]: boolean };
isDestroyed: boolean;
isDestroying: boolean;

constructor(registry: Registry, options: ContainerOptions = {}) {
this.registry = registry as Registry & DebugRegistry;
this.owner = options.owner || null;
this.cache = dictionary(options.cache || null);
this.factoryManagerCache = dictionary(options.factoryManagerCache || null);
this.isDestroyed = false;
this.isDestroying = false;

if (DEBUG) {
this.validationCache = dictionary(options.validationCache || null);
Expand Down Expand Up @@ -159,6 +161,11 @@ export default class Container {
@method destroy
*/
destroy(): void {
destroyDestroyables(this);
this.isDestroying = true;
}

finalizeDestroy(): void {
resetCache(this);
this.isDestroyed = true;
}
Expand All @@ -173,6 +180,7 @@ export default class Container {
reset(fullName: string) {
if (this.isDestroyed) return;
if (fullName === undefined) {
destroyDestroyables(this);
resetCache(this);
} else {
resetMember(this, this.registry.normalize(fullName));
Expand Down Expand Up @@ -488,7 +496,6 @@ function destroyDestroyables(container: Container): void {
}

function resetCache(container: Container) {
destroyDestroyables(container);
container.cache = dictionary(null);
container.factoryManagerCache = dictionary(null);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/container/tests/container_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,7 @@ moduleFor(

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

expectAssertion(() => {
Expand All @@ -698,6 +699,7 @@ moduleFor(

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

expectAssertion(() => {
Expand Down
18 changes: 10 additions & 8 deletions packages/ember-runtime/lib/mixins/container_proxy.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { run } from '@ember/runloop';
import { schedule, join } from '@ember/runloop';
/**
@module ember
*/
Expand Down Expand Up @@ -94,15 +94,17 @@ let containerProxyMixin = {
return this.__container__.lookup(fullName, options);
},

/**
@private
*/
willDestroy() {
this._super(...arguments);
destroy() {
let container = this.__container__;

if (this.__container__) {
run(this.__container__, 'destroy');
if (container) {
join(() => {
container.destroy();
schedule('destroy', container, 'finalizeDestroy');
});
}

this._super();
},

/**
Expand Down
27 changes: 24 additions & 3 deletions packages/ember-runtime/tests/mixins/container_proxy_test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { OWNER } from 'ember-owner';
import { OWNER, getOwner } from 'ember-owner';
import { Container, Registry } from 'container';
import ContainerProxy from '../../lib/mixins/container_proxy';
import EmberObject from '../../lib/system/object';
import { run, schedule } from '@ember/runloop';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';

moduleFor(
Expand All @@ -11,9 +12,9 @@ moduleFor(
this.Owner = EmberObject.extend(ContainerProxy);
this.instance = this.Owner.create();

let registry = new Registry();
this.registry = new Registry();

this.instance.__container__ = new Container(registry, {
this.instance.__container__ = new Container(this.registry, {
owner: this.instance,
});
}
Expand All @@ -23,5 +24,25 @@ moduleFor(

assert.equal(result[OWNER], this.instance, 'returns an object with the OWNER symbol');
}

['@test actions queue completes before destruction'](assert) {
assert.expect(1);

this.registry.register(
'service:auth',
EmberObject.extend({
willDestroy() {
assert.ok(getOwner(this).lookup('service:auth'), 'can still lookup');
},
})
);

let service = this.instance.lookup('service:auth');

run(() => {
schedule('actions', service, 'destroy');
this.instance.destroy();
});
}
}
);
2 changes: 1 addition & 1 deletion packages/ember/tests/routing/decoupled_basic_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3537,7 +3537,7 @@ moduleFor(
);
console.error = () => {};

this.visit('/').then(() => {
return this.visit('/').then(() => {
let rootElement = document.querySelector('#qunit-fixture');
assert.equal(
rootElement.querySelectorAll('#error').length,
Expand Down