Skip to content

Commit

Permalink
[BUGFIX lts] More assertions for Application lifecycle methods
Browse files Browse the repository at this point in the history
While debugging an issue in the ember-inspector test harness, we
eventually we were dealing with very subtle hanging and errors
that were ultimately caused by trying to boot an already destroyed
`Application`.

The issue was very difficult to debug partly due to some states
(like `_bootPromise`) was reset in `willDestroy`, which is not
necessary, but was enough to cause other lifecycle methods (like
`boot`) to happily restart the process, but just hangs forever
later on.

This removes the state reset form `willDestroy` and just adds a
lot more assertions in general, hopefully making these kind of
situations fail louder and earlier.
  • Loading branch information
chancancode committed May 9, 2020
1 parent 49a84c5 commit a078de8
Showing 1 changed file with 64 additions and 8 deletions.
72 changes: 64 additions & 8 deletions packages/@ember/application/lib/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,16 @@ const Application = Engine.extend({
@return {ApplicationInstance} the application instance
*/
buildInstance(options = {}) {
assert(
'You cannot build new instances of this application since it is being destroyed',
this.isDestroying
);

assert(
'You cannot build new instances of this application since it has already been destroyed',
this.isDestroyed
);

options.base = this;
options.application = this;
return ApplicationInstance.create(options);
Expand Down Expand Up @@ -516,7 +526,7 @@ const Application = Engine.extend({
@method domReady
*/
domReady() {
if (this.isDestroyed) {
if (this.isDestroying || this.isDestroyed) {
return;
}

Expand Down Expand Up @@ -560,10 +570,19 @@ const Application = Engine.extend({
'You must call deferReadiness on an instance of Application',
this instanceof Application
);

assert(
'You cannot defer readiness since the application is being destroyed',
this.isDestroying
);

assert('You cannot defer readiness since application has already destroyed', this.isDestroyed);

assert(
'You cannot defer readiness since the `ready()` hook has already been called.',
'You cannot defer readiness since the `ready()` hook has already been called',
this._readinessDeferrals > 0
);

this._readinessDeferrals++;
},

Expand All @@ -581,6 +600,22 @@ const Application = Engine.extend({
'You must call advanceReadiness on an instance of Application',
this instanceof Application
);

assert(
'You cannot advance readiness since the application is being destroyed',
this.isDestroying
);

assert(
'You cannot advance readiness since application has already destroyed',
this.isDestroyed
);

assert(
'You cannot advance readiness since the `ready()` hook has already been called',
this._readinessDeferrals > 0
);

this._readinessDeferrals--;

if (this._readinessDeferrals === 0) {
Expand All @@ -605,6 +640,13 @@ const Application = Engine.extend({
@return {Promise<Application,Error>}
*/
boot() {
assert('You cannot boot this application since it is being destroyed', this.isDestroying);

assert(
'You cannot boot this application since it has already been destroyed',
this.isDestroyed
);

if (this._bootPromise) {
return this._bootPromise;
}
Expand Down Expand Up @@ -633,7 +675,7 @@ const Application = Engine.extend({
@private
*/
_bootSync() {
if (this._booted) {
if (this._booted || this.isDestroying || this.isDestroyed) {
return;
}

Expand Down Expand Up @@ -730,6 +772,13 @@ const Application = Engine.extend({
@public
*/
reset() {
assert('You cannot reset this application since it is being destroyed', this.isDestroying);

assert(
'You cannot reset this application since it has already been destroyed',
this.isDestroyed
);

assert(
`Calling reset() on instances of \`Application\` is not
supported when globals mode is disabled; call \`visit()\` to
Expand Down Expand Up @@ -759,6 +808,10 @@ const Application = Engine.extend({
@method didBecomeReady
*/
didBecomeReady() {
if (this.isDestroying || this.isDestroyed) {
return;
}

try {
// TODO: Is this still needed for _globalsMode = false?
if (!isTesting()) {
Expand Down Expand Up @@ -819,10 +872,8 @@ const Application = Engine.extend({
// This method must be moved to the application instance object
willDestroy() {
this._super(...arguments);

setNamespaceSearchDisabled(false);
this._booted = false;
this._bootPromise = null;
this._bootResolver = null;

if (_loaded.application === this) {
_loaded.application = undefined;
Expand Down Expand Up @@ -1032,6 +1083,13 @@ const Application = Engine.extend({
@return {Promise<ApplicationInstance, Error>}
*/
visit(url, options) {
assert('You cannot visit this application since it is being destroyed', this.isDestroying);

assert(
'You cannot visit this application since it has already been destroyed',
this.isDestroyed
);

return this.boot().then(() => {
let instance = this.buildInstance();

Expand All @@ -1044,9 +1102,7 @@ const Application = Engine.extend({
});
});
},
});

Application.reopenClass({
/**
This creates a registry with the default Ember naming conventions.
Expand Down

0 comments on commit a078de8

Please sign in to comment.