Ember.js: Em.run methods and destroyed objects

[tchak]

var MyController = Em.Controller.extend({
  actions: {
    doStuff: function() {
      Em.run.debounce(this, 'refresh', 100);
    }
  },
  refresh: function() {
    if (this.isDestroyed) { return; }
    this.set('content', {});
  }
});

// And then somewhere in my code (or tests)

App.reset();

I have to use this pattern for all my debounce / next calls. If not I experience "set on destroyed object" errors.

Could we "fix" Em.run async methods to not execute on destroyed objects?

cc @ebryn

[stefanpenner]

Initially i thought maybe we blow away the existing queues on reset, but that is likely going to be mega confusing.

i suspect having Em.run not perform on isDestroyed objects is unfortunately the only option. @kselden thoughts?

[tchak]

@stefanpenner hmm, why you think blowing existing queues is confusing? Is there a simple way to do it in my app if I want to?

[stefanpenner]

@tchak its currently just a very undefined area. I've seen people use the queues explicitly around App#reset to mitigate some currently issues. Such as doing the reset within an Ember.run.next

I have to admit, I have not looked closely, those usages may just be in error.

[tchak]

@stefanpenner another maybe related issue I have, is the fact that if I do something like this :

app.destroy();
app = App.create();

The app is recreated in a wired state. Like if the view were rerendered but actions not registered...
I tried to wrap the creation in a Em.run.next but it did not help. But adding a delay of 500 ms with Em.run.later helps... I am almost thinking it could help to return a promise from destroy method on the app that resolves once the app is completely cleared. Maybe the same with reset.

[stefanpenner]

ya we need to ensure reset/destroy/create occur in independent run-loops

[wagenet]

@stefanpenner So do you have a plan on what needs to be done here? Will you do it? or should we find someone else to?

[kurko]

fyi, I described this exact problem I had in another issue: https://github.com/emberjs/ember.js/issues/2842#issuecomment-31247055

[ebryn]

I don't think this is a bug. I believe you should be keeping a reference to the scheduled debounce and cancel it inside willDestroy.

[theworkerant]

Any news on this? As of 1.3.1 I can't seem to call App.reset() in my test suite after using the visit helper. reset is trying to set a route that isDestroyed. There's no debounce / next action happening in my app.

setURL: function(path) {
    set(this, 'path', path);  // this.isDestroyed == true
  }

Calling App.reset somewhere in my normally running application throws some other errors related to destroying already destroyed things.

Ember 1.2.0 doesn't have this problem.

[stefanpenner]

@theworkerant is it possible for you to provide a jsbin or jsiddle demonstrating this, it will make it easier to figure out whats going on.

[theworkerant]

Yea I'll work on that, all the fiddle templates I found with latest builds are just broken. I guess I'll make one from scratch.

[theworkerant]

False alarm, intensive stack trace analysis revealed my error.

I was doing this:

  didInsertElement: ->
    @$().modal "show"
    @$().bind "hide.bs.modal", $.proxy( (-> @get("controller").transitionToRoute("entries.index")), @)

As @ebryn suggested, keeping track of this during willDestroy solves the problem.

  willDestroyElement: ->
    @$().unbind "hide.bs.modal"

I guess it's not a bug, but I feel it's pretty hard to predict when these kinds of things will come up—arriving at the same error 2 different ways (this one and @tchak's) seems like maybe there should be some logic to "fix" Em.run. At least a much more helpful error message.

Again, this _does not_ happen in 1.2.0.

[stefanpenner]

@theworkerant you likely also want to use Ember.run within that proxy, or use Ember.run.bind

[wagenet]

Closing due to inactivity.

[devinus]

Can we reopen this? It's a problem for me when using itemController's with any observers that have async in them.

E.g.:

var ThingController = Em.ObjectController.extend({
  bar: null,
  updateFoo: function() {
    var self = this;
    this.get('store').fetchBaz().then(function(baz) {
      self.set('bar', baz);
    });
  }.observes('bar')
});

[stefanpenner]

this isn't a bug, but we should explore patterns to handle this that are more accessible

[devinus]

Yeah, it's not a bug--but it just seems really ugly without a good pattern.

[stefanpenner]

i do same gnarly stuff and "black hole" the promise, but i need to think of a way that makes sense to people who don't maintain a promise library

[wagenet]

@stefanpenner ping.

[brisc]

Can we reopen this? Using async code that may be executed after a view is destroying / destroyed is a common issue. Ember guides or the community doesn't provide a nice solution / pattern to handle this case. Checking for isDestroyed or isDestroying in each async block seems really ugly.

[stefanpenner]

var ThingController = Em.ObjectController.extend({
  bar: null,
  updateFoo: function() {
    var self = this;
    this.get('store').fetchBaz().then(function(baz) {
      if (self.isDestroyed} { return; } // <-- solution today
      self.set('bar', baz);
    });
  }.observes('bar')
});

I have been experimenting with https://gist.github.com/stefanpenner/e1c95f77cbea29630fec#file-async-object-js but it is by no means something I would suggest a stable idea yet.

[BryanCrotaz]

I'm seeing this "bug" in Ember.run.debounce in https://github.com/knownasilya/ember-cli-toggle.

My thoughts - when you schedule Ember.run.debounce, you're saying you want this code to get run once, soon.

Therefore should willDestroy guarantee to run any outstanding debounced functions before calling super? Is there a nice way to do this as part of the framework?

[sarus]

I just ran into this issue today when running a unit test on a component that used Ember.run.later to fade in and out some text.

I was getting the error:

Error: Assertion Failed: calling set on destroyed object at http://localhost:7357/assets/vendor.js, line 15408

The worst part is that the error causes a test to fail that is not related to the test that is actually causing the problem making a bit tricky to figure out.

Using stefanpenner's suggested fix of if (self.isDestroyed) { return; } inside the Ember.run.later function worked to fix the problem in testing.

Edit: See stefanpenners post below which makes the good point that I should be cleaning up the timer when the component is destroyed if it hasn't run yet to prevent the issue I'm having in testing.

[stefanpenner]

Therefore should willDestroy guarantee to run any outstanding debounced functions before calling super? Is there a nice way to do this as part of the framework?

nope, in general destructors having extra side-affects are seen as anti-patterns

[stefanpenner]

Using stefanpenner's suggested fix of if (self.isDestroyed} { return; } inside the Ember.run.later function worked to fix the problem in testing.

In idiomatic OO, if you created something, you must clean it up. In a GC'd language this often "just happens", but this isn't true with timers. So if you have an object that spawns an Ember.run.later you should also be sure to cancel it.

Although verbose, the following would be the correct solution.

export default Ember.Object.extend({
  startPolling() {
    if (this._poller) { throw new Error('cannot have to concurrent pollers'); }
    this._poller = Ember.run.later(....)
  },
  stopPolling() {
    Ember.run.cancel(this._poller);
  },

  willDestroy() {
    this._super(...arguments);
    this.stopPolling()
  }
});

It should be possible to add sugar, but I believe that should be left to the addon ecosystem to flesh out.

[sarus]

@stefanpenner

Thanks! That seems like a better way to clean up and gets rid of the testing issue.

[stefanpenner]

Thanks! That seems like a better way to clean up and gets rid of the testing issue.

it is also important in non-testing scenarios. Imaging leaving lingering pollers as the user navigates the app.

[machty]

This tweet brought me here.

Apologies if I've been over-advertising, but for posterity, the much-better alternative to implementing lifecycle hooks to clean up your async (and avoid calling set on destroyed object errors) is to use the ember-concurrency addon. In e-c land, the way to safe debounce is to make a restartable task that does a timeout at the beginning of it, a la the typeahead example.

[BryanCrotaz]

I'd say advertise as much as you like. ember-concurrency has solved some
really hard issues for us by snapping its fingers. I agree that it's a good
solution in this case.