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] More assertions for Application lifecycle methods #18960

Merged
merged 1 commit into from
May 9, 2020

Conversation

chancancode
Copy link
Member

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.

@chancancode
Copy link
Member Author

...seems like this broke many things 😅

packages/@ember/application/lib/application.js Outdated Show resolved Hide resolved
packages/@ember/application/lib/application.js Outdated Show resolved Hide resolved
@@ -516,7 +526,7 @@ const Application = Engine.extend({
@method domReady
*/
domReady() {
if (this.isDestroyed) {
if (this.isDestroying || this.isDestroyed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, once we go this.isDestroying = true we never change it again. I think checking both makes the code easier to read (and avoids everyone having to internalize that particular detail) but I don't think it is actually going to change anything about the semantics.

packages/@ember/application/lib/application.js Outdated Show resolved Hide resolved
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.
@rwjblue rwjblue merged commit 7180f51 into master May 9, 2020
@rwjblue rwjblue deleted the application-assertions branch May 9, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants