Cannot findAll -> destroyRecord

[jacobq]

I don't know what I'm missing, but it seems like it's not possible to destroy (delete & save) records, at least not when using local (IndexedDB) or in-memory database adapters (haven't tried others). That's hard for me to believe for an addon that apparently has significant usage & maturity, so I'd like to think I'm doing something wrong. Could someone help me figure out what it is?

Complete reproduction repo is available here:
https://github.com/jacobq/ember-pouch-test-problem

Part of #235
Possibly related to #113 or #217

[broerse]

Don't have time to look at your problem now. Take a look at this working code: https://github.com/broerse/ember-cli-blog/blob/master/app/routes/posts.js#L37

Will try to find time 😄

[jacobq]

In case it helps, you can reproduce the problem in the app by running this in the console:

var store = Myapp.__container__.lookup('service:store');
store.createRecord('post', {}).save()
  .then(() => store.findAll('post')
  .then(p => p.forEach(post => post.destroyRecord())))

Error: Attempted to handle event `pushedData` on <post:19058E23-5F9A-D59C-A788-349C91E1D50B> while in state root.deleted.inFlight. 
    at new EmberError (http://localhost:4200/assets/vendor.js:24714:31)
    at InternalModel._unhandledEvent (http://localhost:4200/assets/vendor.js:114460:13)
    at InternalModel.send (http://localhost:4200/assets/vendor.js:114308:14)
    at InternalModel.pushedData (http://localhost:4200/assets/vendor.js:114263:12)
    at InternalModel.setupData (http://localhost:4200/assets/vendor.js:114180:12)
    at Class._load (http://localhost:4200/assets/vendor.js:120046:21)
    at Class._pushInternalModel (http://localhost:4200/assets/vendor.js:120396:32)
    at internalModelOrModels._backburner.join (http://localhost:4200/assets/vendor.js:120343:38)
    at Backburner._run (http://localhost:4200/assets/vendor.js:39176:35)
    at Backburner._join (http://localhost:4200/assets/vendor.js:39154:29)"

Note, however, that if the destroyRecord is scheduled to run later the problem doesn't occur.

var store = Myapp.__container__.lookup('service:store');
store.createRecord('post', {}).save()
  .then(() => store.findAll('post')
  .then(p => p.forEach(Ember.run.next(post => post.destroyRecord())))) // works

It seems that the application avoids this problem (and perhaps this is also related to what was happening in #188) by never calling createRecord and destroyRecord in close proximity / the same run loop cycle.

I found that by adding a simple acceptance test for this app in my fork I could intermittently reproduce the error when running with ember test --server:

However, the only 100% reliable way to reproduce the problem that I have found so far is to use findAll then call destroyRecord on the returned object(s).

Also, it may be worth noting that unloadedDocumentChanged can be called after store is destroyed, e.g. during test tear-down. While I don't think that this is itself causing the problem, it is a subtle bug in itself that may reveal something deeper.

I hope this information is helpful for fixing the problem. I have spent too much time already diving into this :/

[llunn]

Note, however, that if the destroyRecord is scheduled to run later the problem doesn't occur.

This is not an ember-pouch issue. For lack of better vocabulary, this should be considered expected unexpected behaviour of Ember Data working within the confines of the ember scheduler and a quasi-async environment. It is completely possible that the record actually has been created but the store is in some intermediate state as it's waiting for a callback to resolve. You can use the isCreated, isSaving, isNew, isValid, etc model properties to check for the model's actual state instead of making an assumption that the store and the adapter are in agreement on state.

It seems that the application avoids this problem (and perhaps this is also related to what was happening in #188) by never calling createRecord and destroyRecord in close proximity / the same run loop cycle.

Yes, likely, and a good indicator that there might be more to investigate with how the ember scheduler assigns priority and/or determines batching order, but I think this has nothing to do with ember-pouch.

[jacobq]

This is not an ember-pouch issue.

I confess that I really do not understand enough about the inner workings of ember-data and ember-pouch, but I have not seen this behavior with other adapters, and it seems to break the contract of basic high-level functions like createRecord and destroyRecord. So while I appreciate the insight you've shared here, I think concluding that everything is fine / nothing should change is a mistake. If indeed this is not technically a bug, then at the very least the documentation (e.g. for destroyRecord) should be updated to make this clear. Currently it says that creating a record creates it and destroying it destroys it, and implies that it doesn't matter what kinds of async things are happening underneath. It would seem reasonable to me that waiting for findAll to resolve, for example, would be sufficient to guarantee that store has the most up-to-date information from the adapter (as of the time it was called).

[broerse]

@jacobq I am not sure but I think this should be fixed already. What is your ember-data version?

Edit: I see you user ember-data 3.5.0 already. That should work. Will take a look.

[jlami]

I've made a ember-data issue.

For now it might be good to know a few workarounds:

You use findAll to loop over all the items in the store. You can use peekAll to only process the ones already in the store. The findAll is the culprit here, since it returns faster the second time you call it. It then immediately gives you the same results the peekAll would give you and later might add extra items, or update existing items when the reload that has been started in the background finishes. See https://api.emberjs.com/ember-data/3.8/classes/DS.Store/methods/findAll?anchor=findAll
The problem is in those background reloads that send data to ember-data at a time that it does not expect it (right when it has already told the adapter to delete it). So using peekAll will prevent this altogether, while still getting the same results, since findAll returned with the same results already.

You can also tell the findAll to not reload in the background by adding shouldBackgroundReloadAll() { return false; }, to your adapter, or force it to always wait for the complete reload and not give the peekAll results first by specifying the following: store.findAll('person', {reload: true}).
The first solution effectively makes the findAll a peekAll while the reload option disables the peekAll behavior.

But if you go for the always reload option, it might still give you problems if you have multiple findAll requests running simultaneously. Only an ember-data solution will fix this. Your run.next workaround could still give you problems, as no one actually knows when the findAll will return that causes the pushData. That could also be right after your run.next has started the save.

PS.
If you use the following code it might be good to also set shouldBackgroundReloadAll to false, since all changes will already be pushed to the store this way.
https://github.com/broerse/ember-cli-blog/blob/2b1494f24f82e2727cee0323d2119a79f615275e/app/adapters/application.js#L61-L69

[jacobq]

In case it's helpful, the use case that lead to the creation of this issue is having an instance-initializer that enforces the existence of certain records (creates them if they don't exist) and non-existence of others (deletes existing ones unless they meet some criterion). It would do a store.findAll('model-of-interest').then(/* delete unwanted */)

[jacobq]

I finally dug into this and figured out what is going wrong. The crux of it is that store.findAll() does not wait for adapter.findAll() to resolve. This is by design as the ember-data API docs note:

When the returned promise resolves depends on the reload behavior, configured via the passed options hash and the result of the adapter's shouldReloadAll method. If { reload: true } is passed or adapter.shouldReloadAll evaluates to true, then the returned promise resolves once the adapter returns data, regardless if there are already records in the store. If no reload is indicated via the above mentioned ways, then the promise immediately resolves with all the records currently loaded in the store. Optionally, if adapter.shouldBackgroundReloadAll evaluates to true, then a background reload is started. Once this resolves, the array with which the promise resolves, is updated automatically so it contains all the records in the store.

Why is this a problem? It is a problem is cases when the records may be deleted (modified?) between the time findRecord is called and the time it resolves, causing "Error: Attempted to handle event pushedData on model:id while in state root.deleted.inFlight." inside of the ember-data that handles the resolved data from the adapter (evidently _pushedInternalModel).

How can the problem be avoided / fixed? I see two approaches:

1. Set adapter.shouldReloadAll to return true (and perhaps also adapter.shouldReloadRecord). This does prevent the problem. However, there is a comment here that suggests that this is undesirable since we have another mechanism in place to ensure records are kept up to date.
   Applications that may encounter this situation can currently work around it by extending ember-pouch's Adapter to override these methods, trading off ~10ms of additional latency for more deterministic behavior.

2. Detect this situation by chaining onto this.get('db').rel.findX(...) promise and examining the record snapshots' state. For example,

  findAll_original: function(store, type /*, sinceToken */) {
    // TODO: use sinceToken
    this._init(store, type);
    return this.get('db').rel.find(this.getRecordTypeName(type));
  },
  
  findAll_proposed: function(store, type, _neverSet, snapshotRecordArray) {
    this._init(store, type);
    var recordTypeName = this.getRecordTypeName(type);
    var dbPromise = this.get('db').rel.find(recordTypeName);
    return dbPromise.then(data => {
      var states = snapshotRecordArray.snapshots().map(sar => sar._internalModel.currentState.stateName);
      if (states.some(s => s.includes('deleted.inFlight'))) {
        //throw Error(`Records appear to have been deleted between calling pouch adapter's findAll and its underlying tasks finishing`);
        // Remove deleted records from result
        var key = data[recordTypeName] ? recordTypeName : pluralize(recordTypeName);
        data[key] = data[key].filter((_r, i) => !states[i].includes('deleted.inFlight'));
      }
      return data;
    })
  },

What other solutions do people think might be appropriate? Would a PR that implements the second approach be acceptable?