How to do you check for `isDestroyed`?

Design
1

In our app, we’ve been practicing “routable component” patterns. E.G. not using controllers (except for query-params). One issue that comes up more any more is handling async promise operations (such as timers or ajax) when the component could be destroyed by the user navigating away.

Here is an example:

// app/components/x-routable/post.js

export default Ember.Component.extend({
  /* ... */
  actions: {
    saveDraft() {
      this.set('isSavingDraft', true);
      this.get('post').save().finally(() => {
        this.set('isSavingDraft', false);
      });
    }
  }
});

In this case if the post is saving and the user navigates away from the page, once the promise is resolved, the this.set('isSavingDraft', false); will result in an assertion (Error: Assertion Failed: calling set on destroyed object).

The easiest solution is to just check this.isDestroyed on the promise handler, but that’s not very elegant and creates a lot of boilerplate across the app. Example:

saveDraft() {
  this.set('isSavingDraft', true);
  this.get('post').save().finally(() => {
    if(!this.isDestroyed) {
      this.set('isSavingDraft', false);
    }
  });
}

I toyed around with the idea of creating a macro/decorator for the promise handler, but you’d have to pass in the scope (or bind it). Example:

function isDestroyed(target, func) {
  if(!target.isDestroyed) {
    func();
  }
}

this.get('post').save().finally(isDestroyed(this, () => {
  this.set('isSavingDraft', false);
}));

I’d be curious to hear what others think and if they’ve run into this.

1 Like
2

I wrote a little utility component to abstract this boilerplate logic away.

https://github.com/jasonmit/async-container

3

So while that does apply in the exact case I provided (isSavingDraft), there are still plenty of other cases where that component doesn’t work. Another very common example is fetching data from a promise and storing it on the component. Example:

// app/components/x-routable/post.js

export default Ember.Component.extend({
  /* ... */
  actions: {
    showDetails() {
      let store = this.get('store');
      store.query(/* ...*/).then(postDetails => {
        this.set('postDetails', postDetails);
      });
    }
  }
});
4

https://github.com/jasonmit/async-container#display-the-results-of-the-promise I just added support for this. In the case where I am using <button> could just as well be a component you are passing attributes into, in this case the promise result.

5

I’m not knocking your addon, but I don’t think it fits my use case and it’s not really the heart of my question. I think it has utility, but it’s not really what I’m asking about.

One major issue is that it requires a new component to be in the ancestor hierarchy. So for my “routable component” it’s not really applicable unless I restructure a lot of my code to live in a new lower component. Not to mention adding a new element to the mix could cause some havoc with styles.

Ultimately, I think when routable components lands and people begin to transition away from controllers this scenario will become more common and I think a JS side solution will be the most desirable.

6

Agreed and most often I see people not guarding and thus their apps break in these scenarios. That said, the need to guard seems dubious. Perhaps calling set on a component that is destroyed should result in a noop. I guess I don’t know enough about the history of why this guarding was introduced. Perhaps these things will be flushed out by the time routeable components land in stable.

To answer your original question, I don’t see an issue with the unlessDestroyed utility function.

7

Also, if you don’t feel like passing this around as an arg you can create a Mixin out of it.

import Ember from 'ember';

export default Ember.Mixin.create({
  unlessDestroyed(func) {
    return (...args) => {
      if (this.isDestroyed || this.isDestroying) { return; }
      return func.apply(this, args);
    };
  }
});

import Ember from 'ember';
import UnlessDestroyed from '../mixins/unless-destroyed';

export default Ember.Component.extend(UnlessDestroyed, {
  actions: {
    save() {
      return this.get('post').save().finally(this.unlessDestroyed(function() {
        this.set('isSavingDraft', false);
      }));
    }
  }
});
1 Like
8

@workmanw — we’ve definitely run into this exact problem as well. I think I like the solution you have better than the one we came up with (using that isDestroyed() function).

But, for sake of conversation: since we’re in the habit of using the functional set Ember.set(object, key, value) rather than this.set, we created a simple function, called safeSet, that checks wether the target is destroyed before attempting to set the value.

function safeSet(context, key, value) {
  if (!context.isDestroyed) {
    return Ember.set(context, key, value);
  }
}

Problem with this is you have to remember to use it anywhere you might set a value after the promise resolves, which is why I like your solution much better.

I did see recently that macthy created a new ember addon ( https://github.com/machty/ember-concurrency ) that also addresses this. I like the promise of his idea, but my initial scan through the examples left me a little confused.

That being said, I wonder if the real right solution might be to get support for cancelable promises. I think I’ve seen around that this is a proposed feature for the Promises spec. In the meantime, I suppose it wouldn’t be too hard to create a new CancelablePromise wrapper around RSVP.Promise. Then, like with manually created event listeners, you’ll need to cancel the promise when the component is about to get destroyed.

5 Likes
9

Yea, I think that’s probably the right solution long term.

@Spencer_Price @jasonmit Thanks for weighing in. At least I know that I’m not alone in having these use cases.

– Wesley

10

That would really hurt the ability to detect memory leaks however. I actually liked @Spencer_Price 's solution better. Not the most elegant, but I like the fact that the code explicitly states the intent. Could even call it setUnlessDestroyed.

Cancellable promises are hard. Cancelling a single promise is not, but defining what happens when you start chaining them is not easy. Can I cancel the second in a chain? If I do so, should it also cancel upstream and/or downstream promises? Should there be a way to revert a promise that gets cancelled after resolving? I don’t expect those issues to be solved and resulting solutions to be implemented in the near future.

11

What do you mean? Can you explain more.

12

One of the main purposes of throwing an error when attempting to set a value on a destroyed object is it allows catching memory leaks due to improperly unregistered handlers, which is one of the primary causes of memory leaks.

Such a handler will be called at some later point, due to the registered event occurring. Assuming it does something useful, it is likely it will change some properties, or call some methods that will. That will throw an error, with a stack trace that leads directly to the handlers that was not properly unregistered. If it did not, it would simply do its job, silently keeping a deleted object up to date.

3 Likes
13

Talk about timing. @stefan was working on this issue the last day or so and landed this experimental project that aims to solve what we’re discussing GitHub - stefanpenner/ember-weak-ref

14

I still think components should not make AJAX calls. These really should be put in a service that will continue to exist after the component is destroyed.

15

I firmly disagree. The root of why is that services are singletons and components are not. When you have more than one component of the same type, IE lists, and they need to get extra data or make a server call based on user action, that resulting state has to live somewhere. That is the heart of this problem.

The actual $.ajax call can live in a service, that’s fine. But when the AJAX promise is fulfilled and the resulting state is returned, it has to live somewhere. If it lives on the service, it would have to do so in an Array with other similar data. If that’s the case, then every component has to scour through a list of data and map it back into it’s UI. That is likely to cause more performance issues, not less.

Here is a real world use case from our app. We have a list of entities, call them ResearchItem. We display 50 research items at a time in a collapsed fashion. The user can expand them to see more details and we go to the server to fetch that data. We actually have a service to do this, the function getMoreDetails(researchItem) returns a promise. Our list components register handlers on the promise and store the data on the component when it resolves.

I think that there are probably many apps that are more simplistic in nature, such as form software, and don’t require multiple AJAX calls to fill a page with data. And for them components not making AJAX calls is probably fine. But you can’t always assume that.

16

Very good implementation. Now you just have to make sure to remove those handlers when the component is destroyed!

17

Hi everyone,

ember-concurrency has progressed quite a bit in the last few weeks and at this point I can recommend that you start using it in your apps. It really strikes at the root of so many problems (some you didn’t even know you had) with managing async in Ember with only promises/callbacks at your disposal. If you look at the docs today and it’s still unclear how it can help your app, please let me know, and I’ll do what I can to make things even more clear.

If I’ve done my job with ember-concurrency you’ll never have to check this.isDestroyed, or implement a hook to cancel a timer, or implement your own isLoading/isRunning/isProcessing flags, or write boilerplate code to prevent a task from running concurrently, ever again.

EDIT: if you’re wondering where to start, check out the website: http://ember-concurrency.com/

1 Like
18

@machty — I should have followed up on this thread myself. I cannot express how excited I am about ember-concurrency now since the latest version landed. (Currently in the midst of refactoring so so many things to using it).

As a consumer of open source and a somewhat-infrequent-contributor-and-not-proud-about-it, thank you. (Not sure I ever expressed a thank you when you delivered the async router three years ago too ;-)).

2 Likes
19

@machty On this post: So have you tried ember-concurrency yet? you stated:

Never write an if(this.isDestroyed) check ever again 1 → instead, use a Task, which gets automatically canceled when the object it lives on is destroyed

I’m not sure that’s completely fair. If I understand right, tasks are essentially decorators? You can’t use them anonymously, right? If that’s true, you’d have to create a class function for every promise handler (resolve/reject/finally) right?

Maybe there is a way to make it work that I’m not seeing. Given the first code snippet in my original question, how would you modify that to use a task so I didn’t have to check isDestroyed?

20

I’m not sure I follow what you’re saying about having to define a class function, but my point was that if you yield aPromise, if that promise fulfills, then you continue where you left off (just as yield timeout(123) continues where you left off once the timer elapses), if the promise rejects, it’s treated like an exception thrown from that point in the generator function (which fwiw is exactly how it works when you await aPromise in the proposed async-await syntax), but if, while that promise hasn’t yet resolved, the task is canceled because the object it lives on was destroyed (e.g. a Component is unrendered) then you’re guaranteed that it will NOT resume the generator function and start doing a bunch of this.set('prop', 'boom')s on a now-destroyed object.

So I can’t think of a situation, if you’re using ember-concurrency tasks and the yield keyword (rather than a chain of promises), where you’d wind up running code outside the lifetime of an object. Let me know if I’m misunderstanding you though.