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

Add logging that hints at relationship bug ≥ ED3.5 #243

Closed
wants to merge 1 commit into from

Conversation

backspace
Copy link
Collaborator

This isn’t a change I think should actually be merged, but it helps reveal what’s causing this test failure start with Ember Data 3.5.

I feel like I have insufficient knowledge of how the internals of RecordData etc work to understand what’s happening and fix it. But you can see from the console.log statements that there are two copies of the related model being stored; one seems maybe-deleted but still hanging around?

           [
              {
                "id": "29072F20-1883-0097-8660-51714430829F",
                "clientId": 2,
                "data": {}
              },
              {
                "id": "29072F20-1883-0097-8660-51714430829F",
                "data": {
                  "name": "pineapple",
                  "rev": "1-52f8d66785bec8cc48d5599314941d9e"
                }
              }
            ]

The actual ingredients returned later in the test have the same id:

            LOG: found ingredients: 29072F20-1883-0097-8660-51714430829F: pineapple,29072F20-1883-0097-8660-51714430829F: pineapple

This is only happening when sync is false and saveHasMany is false. I suppose there could be a hack that filters duplicate records out of a hasMany fetch but that seems less than desirable.

@jlami
Copy link
Collaborator

jlami commented May 27, 2019

I think I tracked it down to ember-data not clearing all its caches in store.unloadAll().

https://github.com/emberjs/data/blob/24e69e2d14ece1e363da9b65e174b19f59f782ba/addon/-private/system/store.js#L2128-L2143

If I replace this._identityMap.clear(); with this._identityMap = new IdentityMap(); in that function the bug disappears. This might also give some insight into why it only breaks after other tests.

But this is just my preliminary results, I want to look a bit further into this issue.

@backspace
Copy link
Collaborator Author

oh wow, nice find, thanks for digging into this!

@jacobq
Copy link
Contributor

jacobq commented Nov 11, 2019

FWIW, this problem also appears intermittent. For example, if I run ember test --server --filter="integration | adapter | basic crud ops > dont save hasMany" then ~30 tests get run, sometimes 1 fails (same as noted previously), but if I hit refresh to run again it seems to pass about 50% of the time. If I add > sync to that ~12 tests run and pass every time.

@jacobq
Copy link
Contributor

jacobq commented Nov 11, 2019

I think I found the problem, or at least a work-around. Here we are calling store.unloadAll() as if it did its work immediately, but in fact it schedules it to be done on the next run loop. Adding promiseToRunLater(1) to the chain here before the call to findRecord makes the problem go away.

jacobq added a commit to jacobq/ember-pouch that referenced this pull request Nov 11, 2019
The test labeled "creating an associated record stores
a reference to it in the parent" was failing intermittently
when run with other tests under conditions "dont save hasMany"
and "sync". This problem appears to be caused by the
test logic not waiting for `store.unloadAll()` to complete.
The Ember Data API docs state that this function schedules
its work to be done during the next run loop so we wait
for an additional cycle after calling it.

See also:

* https://api.emberjs.com/ember-data/3.14/classes/Store/methods/unloadAll?anchor=unloadAll
* pouchdb-community#242
* pouchdb-community#243
jacobq added a commit to jacobq/ember-pouch that referenced this pull request Nov 11, 2019
The test labeled "creating an associated record stores
a reference to it in the parent" was failing intermittently
when run with other tests under conditions "dont save hasMany"
and "sync". This problem appears to be caused by the
test logic not waiting for `store.unloadAll()` to complete.
The Ember Data API docs state that this function schedules
its work to be done during the next run loop.
However, instead of adding a delay and making assumptions
about this, we wrap calls to this function in `run` to
ensure that it has finished before continuing.

See also:

* https://api.emberjs.com/ember-data/3.14/classes/Store/methods/unloadAll?anchor=unloadAll
* pouchdb-community#242
* pouchdb-community#243
jacobq added a commit to jacobq/ember-pouch that referenced this pull request Nov 11, 2019
The test labeled "creating an associated record stores
a reference to it in the parent" was failing intermittently
when run with other tests under conditions "dont save hasMany"
and "sync". This problem appears to be caused by the
test logic not waiting for `store.unloadAll()` to complete.
The Ember Data API docs state that this function schedules
its work to be done during the next run loop.
However, instead of adding a delay and making assumptions
about this, we wrap calls to this function in `run` to
ensure that it has finished before continuing.

See also:

* https://api.emberjs.com/ember-data/3.14/classes/Store/methods/unloadAll?anchor=unloadAll
* pouchdb-community#242
* pouchdb-community#243
jacobq added a commit to jacobq/ember-pouch that referenced this pull request Nov 11, 2019
The test labeled "creating an associated record stores
a reference to it in the parent" was failing intermittently
when run with other tests under conditions "dont save hasMany"
and "sync". This problem appears to be caused by the
test logic not waiting for `store.unloadAll()` to complete.
The Ember Data API docs state that this function schedules
its work to be done during the next run loop.
However, instead of adding a delay and making assumptions
about this, we wrap calls to this function in `run` to
ensure that it has finished before continuing.

See also:

* https://api.emberjs.com/ember-data/3.14/classes/Store/methods/unloadAll?anchor=unloadAll
* pouchdb-community#242
* pouchdb-community#243
jacobq added a commit to jacobq/ember-pouch that referenced this pull request Nov 11, 2019
The test labeled "creating an associated record stores
a reference to it in the parent" was failing intermittently
with the recently added ember LTS versions (e.g. 3.4)
when run with other tests under conditions "dont save hasMany"
and "sync". This problem appears to be caused by the
test logic not waiting for `store.unloadAll()` to complete.
The Ember Data API docs state that this function schedules
its work to be done during the next run loop, so this commit
wraps these calls with `Ember.run` to ensure that it finishes
before continuing.

See also:

* https://api.emberjs.com/ember-data/3.14/classes/Store/methods/unloadAll?anchor=unloadAll
* pouchdb-community#242
* pouchdb-community#243
jacobq added a commit to jacobq/ember-pouch that referenced this pull request Nov 11, 2019
The test labeled "creating an associated record stores
a reference to it in the parent" was failing intermittently
with the recently added ember LTS versions (e.g. 3.4)
when run with other tests under conditions "dont save hasMany"
and "sync". This problem appears to be caused by the
test logic not waiting for `store.unloadAll()` to complete.
The Ember Data API docs state that this function schedules
its work to be done during the next run loop, so this commit
wraps these calls with `Ember.run` to ensure that it finishes
before continuing.

See also:

* https://api.emberjs.com/ember-data/3.14/classes/Store/methods/unloadAll?anchor=unloadAll
* pouchdb-community#242
* pouchdb-community#243
jacobq added a commit to jacobq/ember-pouch that referenced this pull request Nov 11, 2019
The test labeled "creating an associated record stores
a reference to it in the parent" was failing intermittently
with the recently added ember LTS versions (e.g. 3.4)
when run with other tests under conditions "dont save hasMany"
and "sync". This problem appears to be caused by the
test logic not waiting for `store.unloadAll()` to complete.
The Ember Data API docs state that this function schedules
its work to be done during the next run loop, so this commit
wraps these calls with `Ember.run` to ensure that it finishes
before continuing.

See also:

* https://api.emberjs.com/ember-data/3.14/classes/Store/methods/unloadAll?anchor=unloadAll
* pouchdb-community#242
* pouchdb-community#243
backspace pushed a commit that referenced this pull request Nov 11, 2019
This includes a change to fix tests on Ember Data 3.5+, which closes
#243. It also closes #242 by making it redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants