Add guard & more helpful error massage when calling _setupRelationships when store is destroyed #5282

danwenzel · 2017-12-18T16:02:11Z

This PR adds a guard against trying to set up relationships after the store has been destroyed, providing a more helpful error message.

Background:

We ran into a test failure which showed the following error:

TypeError: Cannot read property 'push' of null
    at Class._setupRelationshipsForModel

similar to this issue: miragejs/ember-cli-mirage#1220

After some digging, it turned out that the store had been destroyed too early in the test, and this line was causing the error: https://github.com/emberjs/data/blob/master/addon/-private/system/store.js#L2418

The error happened because the store sets _pushedInternalModels to null when destroyed: https://github.com/emberjs/data/blob/master/addon/-private/system/store.js#L2706, so trying to later call push on it causes an error.

This PR adds an assertion that makes it clearer what's going on, making for faster debugging.

danwenzel · 2017-12-18T17:08:55Z

Not sure why the test is failing in Phantom... It's passing just fine for me locally

rwjblue · 2017-12-20T19:35:42Z

addon/-private/system/store.js

@@ -2415,6 +2415,11 @@ Store = Service.extend({
      return;
    }

+    if (this.isDestroyed) {
+      assert('Attempting to set up relationships after store has been destroyed');


You can add the isDestroyed check directly in the assert invocation. That will allow all of the code to be stripped in production builds...

I believe it would be something like:

assert('Attempting to set up relationships after store has been destroyed', !this.isDestroying && !this.isDestroyed);

In prod, Ember.assert is stripped, so this test would fail

danwenzel · 2017-12-21T14:53:38Z

Alright, so I noticed that the failing test was in the production environment, so I added a conditional in the test file. Let me know if there's a better way to do that.

rwjblue · 2017-12-21T15:42:16Z

tests/integration/store-test.js

  assert.equal(personWillDestroy.called.length, 1, 'expected person to have recieved willDestroy once');
  assert.equal(carWillDestroy.called.length, 1, 'expected car to recieve willDestroy once');
  assert.equal(carsWillDestroy.called.length, 1, 'expected person.cars to recieve willDestroy once');
  assert.equal(adapterPopulatedPeopleWillDestroy.called.length, 1, 'expected adapterPopulatedPeople to recieve willDestroy once');
  assert.equal(filterdPeopleWillDestroy.called.length, 1, 'expected filterdPeople.willDestroy to have been called once');
+  if (config.environment !== 'production') {
+    assert.throws(() => {


I think we generally use assert.expectAssertion(() => {}, /some matcher/) for this. That custom assertion is provided by https://github.com/workmanw/ember-qunit-assert-helpers/ and is aware of development vs production...

@rwjblue - If I do that, then the production test still fails, because it moves on and hits the original TypeError: Cannot read property 'push' of null.

This is why I had originally built the assertion into a conditional. It would allow an early return - in production, the assertion would be skipped, but you wouldn't hit that error.

Or another idea: Maybe we want to just throw instead of assert? i.e. maybe we want this better error message in production too? This case is currently throwing an error in prod either way.

@rwjblue ⬆️ Any thoughts on this?

Pinging again... @rwjblue ⬆️ Any thoughts?

@danwenzel I don't think we want to throw in prod

Also I'm happy to pick this up from where @rwjblue and help you get it over the line. I've been working to fix a lot of these async cleanup issues in Ember-data and Ember so that folks get better info about what went wrong.

Thanks @runspired ! Any thoughts on the other issues above? If we want to leave production alone, we'll have to think of some way to only call that method in the non-prod tests.

@danwenzel see https://github.com/emberjs/data/blob/master/tests/unit/store/push-test.js#L7

Thanks, @runspired , that worked perfectly! All passing now

…ore-destroyed

danwenzel · 2018-04-09T16:34:15Z

@runspired @rwjblue - Ok to ship this?

runspired · 2018-04-09T17:16:07Z

tests/integration/store-test.js

+  assert.throws(() => {
+    store._setupRelationshipsForModel(null, { relationships: {} });
+  }, /Attempting to set up relationships after store has been destroyed/);
+});


We should design a test scenario similar to a real-world scenario that would have caused this to happen. I'm especially interested in such a test because:

It may be that we need to adjust our waiters [FEAT] Prevent async test leakage (feat. adds test waiters) #5422

_setupRelationshipsForModel should still occur in the same run-loop as whatever scheduled it. This indicates to me that this either might be the wrong location to assert, or that we could improve the message to tell people exactly what they need to do to resolve the error.

Also note, for createRecord causing this, we've improved things: #5415

Hi @runspired. I agree that we should eventually dig more into the root issue, but this PR at least makes the situation 1 step better - Someone encountering this in dev or test will know more about what's going on and fix it faster.

I unfortunately won't have time to dig deeper for a while, so maybe we could ship this as-is?

@danwenzel if you can jot down some notes for me about roughly the setup you encountered in the wild, and give me commit access to your fork, I will continue to improve this :)

Hi @runspired , sorry for the delay.

I can't remember the details of my setup, but this gives a pretty good example of the issue: miragejs/ember-cli-mirage#1220.

I've given you access to my fork. Let me know if you need anything else

runspired · 2018-08-08T18:33:39Z

Will revisit this soon, I have a pretty good idea of how to design a good test for it, and likely we can simply do the "right" thing and no-op here vs. error.

Basically, the waiters in #5545 and an assert about using store.push when the store is destroyed should be enough, any other async internal timing things should be on ember-data to cleanup.

runspired · 2018-08-28T17:46:10Z

After revisiting I confirmed that there are only two ways for _setupRelationships to be called: via commit and via push/_push. #5545 has guarded and tested commit, I will follow up with a new PR that guards push/_push.

Dan Wenzel added 2 commits December 18, 2017 09:47

Guard against calling _setupRelationships when store is destroyed

6675af1

Remove unused var

8b5c659

danwenzel mentioned this pull request Dec 18, 2017

Simple test case and having error: Cannot read property 'push' of null miragejs/ember-cli-mirage#1220

Closed

rwjblue requested changes Dec 20, 2017

View reviewed changes

Dan Wenzel added 2 commits December 21, 2017 08:31

only assert non-prod

30676bb

Don't assert assertion in production

b1c291b

In prod, Ember.assert is stripped, so this test would fail

rwjblue reviewed Dec 21, 2017

View reviewed changes

Dan Wenzel added 4 commits March 30, 2018 07:59

Merge branch 'master' into add-assertion-for-setuprelationships-if-st…

bfa9216

…ore-destroyed

Use testInDebug to only run test in non-prod

df966c6

Merge branch 'master' into add-assertion-for-setuprelationships-if-st…

5d25f7f

…ore-destroyed

restore missing newline

11e55cc

runspired requested changes Apr 9, 2018

View reviewed changes

runspired added the towards-beta label Aug 8, 2018

runspired closed this Aug 28, 2018

runspired mentioned this pull request Aug 28, 2018

[FEAT] assert when methods are called on a destroyed store instance #5605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add guard & more helpful error massage when calling _setupRelationships when store is destroyed #5282

Add guard & more helpful error massage when calling _setupRelationships when store is destroyed #5282

danwenzel commented Dec 18, 2017 •

edited

Loading

danwenzel commented Dec 18, 2017

rwjblue Dec 20, 2017

danwenzel Dec 21, 2017

danwenzel commented Dec 21, 2017

rwjblue Dec 21, 2017

danwenzel Dec 21, 2017

danwenzel Dec 26, 2017

danwenzel Jan 2, 2018

danwenzel Jan 24, 2018

runspired Mar 29, 2018

runspired Mar 29, 2018

danwenzel Apr 2, 2018

runspired Apr 2, 2018

danwenzel Apr 5, 2018

danwenzel commented Apr 9, 2018

runspired Apr 9, 2018

runspired Apr 9, 2018

danwenzel Apr 17, 2018 •

edited

Loading

runspired Apr 19, 2018

danwenzel May 7, 2018

runspired May 7, 2018

runspired commented Aug 8, 2018

runspired commented Aug 28, 2018

Add guard & more helpful error massage when calling _setupRelationships when store is destroyed #5282

Add guard & more helpful error massage when calling _setupRelationships when store is destroyed #5282

Conversation

danwenzel commented Dec 18, 2017 • edited Loading

Background:

danwenzel commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwenzel commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwenzel commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwenzel Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Aug 8, 2018

runspired commented Aug 28, 2018

danwenzel commented Dec 18, 2017 •

edited

Loading

danwenzel Apr 17, 2018 •

edited

Loading