[refactor] Common function for Ember-App instance destruction

[arjansingh]

@rwjblue @kratiahuja

This was the suggested refactor I had in mind for the timeout work. I think adding a helper function makes the logic more compact, and eliminates a lot of the state checks that we needed to do when the code was inline.

[rwjblue]

After a quick read this seems good, tests added for hanging processes still pass so it seems to work.

@kratiahuja - Can you review / confirm also?

[kratiahuja]

In general this looks much better to me than what I had done.

[kratiahuja]

We need to make sure we call instance.destroy() only once. Can we make sure if the instance is destroyed with a timer doesn't call instance.destroy() here?

[kratiahuja]

Ohoo sorry this will work. I had only added tests for this. You can drop this.

[kratiahuja]

Can we add some comments to this function just like other functions have. Makes it easier to generate an API at a later point.

[arjansingh]

@kratiahuja I thought about what you said and I created a slightly more complicated slow-app test case which:

• Has a delayed route model that takes 100ms to load.
• Is based on the shoebox app test case, so it also has some actual application code that executes. (basic-app is purely boilerplate from what I can tell).

Interestingly, destroying the application instance in a timeout still does not stop the application instance from executing. Because of that my new test intermittently throws this error:

Error while processing route: index Assertion Failed: calling set on destroyed object Error: Assertion Failed: calling set on destroyed object
    at << stack omitted for brevity >>

It happens with both your original code and my refactored code. It is caught in the promise chain's catch hook so it isn't fatal, but now I'm questioning whether this timeout code is good practice.

@kratiahuja and @rwjblue, thoughts?

[kratiahuja]

@arjansingh Does your slow app have Ember.onerror? I would be curious to see if it bubbles the error up and throws a better error?

I had thought about this and that error is actually expected. The reason being (if I understand the semantics of run loop correctly), if something is scheduled on the run loop, it depends on when it runs. It will run (when it comes to the front of the queue) irrespective of what the state of your instance is when it runs.

In general, the idea that I had of using the destroyAppInstanceInMs was to keep it relatively higher than the average time it takes for visit to complete for your app routes. For example, we do get metrics on the average response time visit takes for a particular app. This will ensure you do not destroy the app early. IMO we need some fail safe mechanism in general or apps can cause CPU usage to reach its peak unknowingly and we should educate the users in a better way with giving such options.

Another usecase of something similar happening (without destroyAppInstanceInMs) is:

1. Your app's route that is being run has an afterModel hook that is used to schedule something in the afterRender queue to run something (let's assume you after some ms). For example some thing like:

afterModel: function() {
  Ember.run.schedule('afterRender', function() {
    Ember.run.later(this, function() {
        // run something after 500 ms when scheduled in the `afterRender` queue
    }, 500);
  });

1. If your visit finishes before the above actually comes in front of the queue and the above 500 ms timer is hit, the instance will be destroyed.
2. Now when the above runs it will throw a similar error if it is actually depending on the instance.
3. If you happen to do some window/document access in the Ember.run.later function and do not have an Ember.onerror, it is fairly possible app will cause the node process to crash.

If you have an Ember.onerror hook also (which will prevent the crash), it is still considered a bad practice from server side. You are going to waste CPU cycles when you have already finished rendering and closed the session.

I would prefer we slowly add these fail safe mechanisms in fastboot itself to make this library more robust. As a long term thing, I would like to also help evolve the fastboot userguide to educate the users on the best practices for server side rendering of their ember apps. More like "how should i write my ember app such that it is performant on server side as well?"

I would like @rwjblue to chime in his thoughts too 😄

[krisselden]

The code and code it is refactoring seem to be unaware that the run loop is used in destroy and that it is async.

[kratiahuja]

yes that is because we don't run the destroy in the context of the sandbox. We actually should do that.

[rwjblue]

Your app's route that is being run has an afterModel hook that is used to schedule something in the afterRender queue to run something (let's assume you after some ms). For example some thing like:

afterModel: function() {
  Ember.run.schedule('afterRender', function() {
    Ember.run.later(this, function() {
        // run something after 500 ms when scheduled in the `afterRender` queue
    }, 500);
  });

Whenever you schedule things for later like this you should always keep track of the returned timeoutID (or whatever we call the cancel token), and cancel it on willDestroy:

afterModel: function() {
  Ember.run.schedule('afterRender',() => {
    if (this._isDestroying) { return; }

    this._cancelId = Ember.run.later(() => {
        // run something after 500 ms when scheduled in the `afterRender` queue
    }, 500);
  });
},

willDestroy() {
  Ember.run.cancel(this._cancelId);
}

The code above is definitely more annoying to write, but is more correct than the initial snippet.

[rwjblue]

we don't run the destroy in the context of the sandbox. We actually should do that.

Agreed.

[rwjblue]

Writing the above reply made me remember that it isn't possible to cancel things that are scheduled by run.schedule so I submitted emberjs/ember.js#14550 to fix...

[kratiahuja]

@rwjblue Agreed we should always cancel the scheduled task. But a lot of times apps don't do that causing node process to probably crash on the server side. This is the reason I would like the library to be more robust to such hard failures. On the client side, you only have one instance in the browser so it may not hit that hard.

After speaking with Kris offline last night, he suggested an idea that the instance destroy in Node should probably allow you to automatically cancel things scheduled by that instance in the run loop. It's not a blocker for this PR but something we probably should evaluate in the long run.

[kratiahuja]

@rwjblue just saw you published ember-lifeline which answers my long term question. Thank you!

[kratiahuja]

Opened an issue to track destroying of app instance and other tasks in the sandbox context: #94

[arjansingh]

So the take aways are:

1. Call destroy from within the Sandbox. (Or even the Ember App with an instance initializer?)
2. Cancel all tasks queued on the run loop.

Do we still want my refactor? We should at least keep the new test case.

[kratiahuja]

@arjansingh I think we should still have the refactor since it is much neater and cleaner API. Regarding the take aways:

1. I opened an issue to track this. Let's fix this as a seperate PR so it is easy to track. We may need to even create the instance in the sandbox.
2. I think this is something the app should do but we should still make the library robust (so we should still destroyAppInstanceInMs) incase an app doesn't follow the best practices.

I will let @rwjblue make the final call.

[kratiahuja]

@rwjblue Is there anything preventing this from merging? In general, I like @arjansingh refactor in this PR

[arjansingh]

Not needed. Closing.