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

Cannot update watchers for model after it has been destroyed #5387

Closed
amk221 opened this issue Mar 18, 2018 · 25 comments
Closed

Cannot update watchers for model after it has been destroyed #5387

amk221 opened this issue Mar 18, 2018 · 25 comments

Comments

@amk221
Copy link
Contributor

amk221 commented Mar 18, 2018

When exiting a route, I need a specific model to be unloaded from the store.

However, if I then try to refresh the route, I get the error

Cannot update watchers for `name…-model::ember323:1>` after it has been destroyed

Here is an example Twiddle:

https://ember-twiddle.com/dfff8b960e58063511927f2768dd7b7c?openFiles=routes.application.js%2C
(Trying to come up with a better example)

The only other mention to this error I found was #5111. (But I don't know enough to say if it is related)

@runspired
Copy link
Contributor

@amk221 given the other issue you mentioned is resolved by #5378, could you test against that PR?

@amk221
Copy link
Contributor Author

amk221 commented Mar 19, 2018

Using the latest ED still has the problem.

I've narrowed down the scenario a bit now (Twiddle)

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 19, 2018

@amk221 did you try against @runspired's branch runspired:fix-peek-all ?

@amk221
Copy link
Contributor Author

amk221 commented Mar 19, 2018

I don't think it's fixed it for my use case. I set up an example repo, and yarn link'd fix-peek-all

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 20, 2018

It's interresting, it seems like there are some timing issues, or maybe a problem with the routing process.
When you hit refresh, it goes to the model hook, load Foo and bar (which takes 250ms), so goes to the loading route, so goes in the resetController which unloads foo. So it setupController with an unloaded record (isDestroyed is true), and it seems like ember (glimmer maybe) does not like that.

I tried to unload the record directly in the model hook, and return it. Weird enough then, the record is not isDestroyed.

@amk221
Copy link
Contributor Author

amk221 commented Mar 20, 2018

  • I wasn't sure how to use runspired:fix-peek-all via package.json, which is why I explained above that I used yarn link (hence package.json appearing not to use the fixed branch)
  • Yes, it does seem like a timing issue - Although the code 'reads correctly', the hooks fire in an order which is counter to what you'd expect.
  • In my app, I worked around the issue by utilising beforeModel instead.

@hoIIer
Copy link

hoIIer commented May 1, 2018

@amk221 could you elaborate on how you used beforeModel? I'm getting this exact same issue where I have a model hook that returns a hash, when transitioning out of the route (sibling/non-sibling) I unload a bunch of records from that page, then in some cases if I am transitioning to a sibling route e.g. /fooA -> /fooB, I get this error and I haven't been able to figure out why... thanks!

@amk221
Copy link
Contributor Author

amk221 commented May 1, 2018

@erichonkanen Originally put the unloadRecord in resetController, so to reset things after the route has been used. But, as this was causing the timing issue I flipped it - so that I do the resetting before the route is entered... to the same effect. That's all!

johncowen pushed a commit to hashicorp/consul that referenced this issue May 11, 2018
@ppcano
Copy link
Contributor

ppcano commented Sep 3, 2018

Using ember 3.3.2 and ember-data 3.3.1, our app triggers the following assertion.

Error: Assertion Failed: Cannot create a new chain watcher for <li@model:xxxx::ember1696:yyyy> after it has been destroyed.

We are executing store.push in setupController and unloadAll on resetController, the exception appears often when entering the same page and the previous unloaded records are pushed again.

Our current workaround has been to use store.unloadRecord instead of store.unloadAll.

Before

  resetController: function(controller, isExiting) {
    this._super(...arguments);

    this.store.unloadAll('serie');
  }

After

 resetController: function(controller, isExiting) {
    this._super(...arguments);

    this.store.peekAll('serie').forEach((item) => {
      this.store.unloadRecord(item);
    });
  }

@lakjain
Copy link

lakjain commented Jan 11, 2019

Hi,
Wanted to know if a solution is found to above issue. I am upgrading Ember-cli from 2.11.1 to 2.18.2 in my project with Ember data - 2.18, and there are lot of issues triggered due to unloadAll not clearing the record properly from store.

@miketervela
Copy link

miketervela commented Jan 16, 2019

I'm having a similar problem here, reproduced in this twiddle:
https://ember-twiddle.com/d87770d65d2c92aecb89ececff2574f7?openFiles=templates.index.hbs%2C

Click "Add Inner" twice
Then click "Remove Last Inner" twice.
Click "Add Inner" again and you get
"Assertion Failed: Cannot update watchers for isActive on <twiddle@model:model-inner::ember185:null> after it has been destroyed."

I don't know how to use ember inspector with a twiddle, but in my app it looks like I'm getting PromiseManyArray with length 1 and no entries (or an undefined entry, hard to tell)

Also note that you can never unload more than two inners - it just keeps claiming to unload the same lastObject after that.

@arthur5005
Copy link

A work around would be really handy. It's weird issues like these that me really frustrated when using ember-data.

@miketervela
Copy link

miketervela commented Jan 16, 2019

One weak workaround I've found (though still testing for success) is to not use Ember.computed.filterBy and similar 'reduce computed' array functions. Especially if the value you are sorting or filtering on doesn't change, then you can probably get away with something like:

Ember.computed('myHasMany.[]', function() {
  return this.get('myHasMany').filter(item => {
    return item && item.get('isActive')
  }
}

instead of:
Ember.computed.filterBy('myHasMany', 'isActive')
That way ember doesn't have to bind to the individual members of the hasMany, just to the array size, and you can check for empty values in your filter callback.

Edit: Of course, this doesn't work if the value you're filtering on can change - only if the value is constant and the containing objects are simply added to or removed from the array

@miketervela
Copy link

miketervela commented Jan 19, 2019

So it turns out my problem was that I had an inverse relationship, and unloading a hasMany item of an inverse relationship doesn't unload cleanly. I overrode 'unloadRecord' of my hasMany target model to be the following:

  unloadRecord() {
    this.set('modelOutter', null);
    return this._super(...arguments);
  }

as shown in this updated twiddle
https://ember-twiddle.com/d87770d65d2c92aecb89ececff2574f7?openFiles=models.model-inner.js%2C

That seems to have solved my problem. Note that store.unloadRecord() simply calls model.unloadRecord() so it handles both situations. unloadAll does not use model.unloadRecord so may still cause the same problem, but it's a significantly different code path .

@lakjain
Copy link

lakjain commented Jan 28, 2019

Facing lot of issues including watchers issue, cannot find a new tag once model is destroyed, hasMany/belongsTo relationships issue when unloadAll is being used for the model. Is there any release/fix version planned which will have a solution to above issues as workarounds tend to fail in some or another scenario.

@lakjain
Copy link

lakjain commented Jan 28, 2019

Also would be really helpful, to have suggestions if anyone successfully downgraded to Ember data 2.12 with Ember-cli > 2.18.

@ghost
Copy link

ghost commented Jan 28, 2019

I am able to reproduce this using @amk221's demo app against 4c539f4.
Our workaround has been to do something like:

const list = this.store.peekAll('foo');
list && list.forEach(rec => rec.unloadRecord());

Note this workaround is the same as what @ppcano demonstrated above.

@ghost
Copy link

ghost commented Jan 29, 2019

@amk221 thank you for posting this demo app; it was helpful demonstrating this actually is not an ember-data bug.
The reproduction linked above actually has a race condition that ultimately leads to the unloadRecord being called on the same reference to foo. As such, it is not an ember-data issue but rather route's loading and unloading data without properly cleaning up. Please see this example that corrects the timing error. The comments in the route explain how to toggle to a version of the demo with broken behavior. Additionally, you can see the same exception thrown in this updated example that does not use ember-data. Thanks to @runspired for pointing out the replication bug and walking me through in detail discord.

@runspired
Copy link
Contributor

After having gone through the reproduction I'm going to close this issue as it seems fairly clear to me that at least in the reproduction the fault lies with app code.

The discord thread linked by @efx above in which I discuss this is a good starting point if you want a deeper explanation, but a couple of key take-aways:

  • Route.resetController occurs after the model() hook is called in a refresh, which means you are handling the new model.
  • record.unloadRecord() attempts to move the underlying state of a record into a "never loaded" state, which also means that the existing record given to the ui will be torn down (since we shouldn't have a record anymore). In this reproduction, the issue is primarily that unloadRecord is being called on the refreshed model (e.g. "reload" then "unload", vs the intent of "unload" then "reload")
  • any time you tell the store to unload a record, or you destroy a record, you should also eliminate any retained references to the record's class instance. In this example, controller.model.foo is a retained instance and since controller is a long lived singleton unloading controller.model.foo without then setting foo to null is a mistake: a mistake that happens to not matter if proper timing of requests results in replacing foo with a different record.

@ppcano
Copy link
Contributor

ppcano commented Feb 19, 2019

you should also eliminate any retained references to the record's class instance

This recommendation has fixed a case where we experienced this problem.

@Redsandro
Copy link

Redsandro commented Feb 12, 2020

In an app with some more complexity, the mentioned workarounds aren't sufficient. Here is how I ended up fixing the issue:

    model(params) {

        return this.store.findRecord('mymodel', params.mymodel_id, { reload: true })
    resetController(controller, isExiting) {
        this._super(...arguments)

        controller.set('model', null)

I'm not sure why, it was a last-ditch attempt, but it works.

@e-monson
Copy link

e-monson commented Mar 3, 2020

@Redsandro Did you do that instead of or in addition to using unloadAll/unloadRecord?

@Redsandro
Copy link

@e-monson To my own surprise, I did it instead of and it worked.

Full disclosure: I added it in an attempt to fix my issue. Which worked. Then I removed all the fixes mentioned here. And it still worked. So the net result is the above.

@Emerson
Copy link

Emerson commented Mar 12, 2020

I just want to chime in and say that this "Cannot update watchers" change is causing us a lot of pain during our upgrade attempt. I understand it's probably for the best and likely due to some bad practices on our end, but we're having a really hard time tracking down the exact causes of this in our code.

In our case, we have a bunch of nested routes, controllers, templates, and components (legacy app 👍) that rely on the computed properties of a record that gets unloaded due to a web socket event.

We've ended up triggering a custom willUnloadRecord event on the ED record in question so that we can try to clean up any references before the record actually gets unloaded... but it's proving to be a lot of work and feels pretty hacky.

One thing that would make this easier to deal with is clear error messages that tell us exactly where we can find the offending watchers (template/computed property). As it stands, we are just sifting through stack traces and commenting things out.

Feel for anyone else going through this - it's a real pain 😔

@Redsandro
Copy link

One thing that would make this easier to deal with is clear error messages that tell us exactly where we can find the offending watchers (template/computed property).

Oh yes please would love this. I have some small projects where I just couldn't find the cause, too. I've simply removed some functionality because I don't have time to do the trial and error. The null trick doesn't work in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests