emberjs / data

WarpDrive is a lightweight data library for web apps — universal, typed, reactive, and ready to scale.
https://api.emberjs.com/ember-data/release
MIT License
3.04k stars 1.33k forks source link

Race condition with computed property and belongsTo #1856

Closed sandstrom closed 10 years ago

sandstrom commented 10 years ago

When a computed property on a 1-to-1 relationship references the other part of the relationship, and it has just been loaded into the store (or is in the process of being loaded, after say a POST), an extra GET request is fired.

When this extra GET request is returned there is duplicate data in the store (two objects with the same id).


A rough order of events is this:

  1. Data from POST loaded into store
  2. Listeners notified (e.g. this.notifyPropertyChange('data');).
  3. Computed filter triggered
  4. Computed property triggered
  5. get triggers store.fetchRecord(belongsTo);
  6. A GET for the newly received (via POST response) cat is fired.
  7. After this GET has completed there are two objects with the same id in the store (causing issues).

Semi-complete example of what causes this:

App = Ember.Application.create();

App.TailsController = Ember.ArrayController.extend({
  detachedTails: Em.computed.filter('content.@each.isAttached', function(tail) {
    !tail.get('isAttached') // causes problems
  })
});

App.Cat = DS.model.extend({
  tail: DS.belongsTo('tail')
});

App.Tail = DS.model.extend({
  cat: DS.belongsTo('cat'),

  isAttached: (function(){ 
    // !!@get('data.cat') // workaround that works
    !!this.get('cat') // causes extra xhr and duplicate data
  }).property('cat')
});

var tail = myStore.find('tail', '123');
var cat = myStore.createRecord('cat');

cat.set('tail', tail);
tail.set('cat', cat); // needed due to ember bug, but not really part of this issue

cat.save(); // will POST to server

// when POST succeeeds, the `detachedTails` computed property will trigger a call to `isAttached` which in turn will trigger a GET for the
// newly created cat. Despite the POST just got the cat payload back. I'm guessing this is some kind of race condition.

// By adjusting `isAttached` to check for `this.get('data.cat')` instead the issue is resolved.
sly7-7 commented 10 years ago

@sandstrom It would be great if you could test that scenario against canary.

sandstrom commented 10 years ago

@sly7-7 Our code code has been refactored since April, but I didn't a preliminary test just now and it seems like it's gone (probably after SSOT-refactor). The tricky thing is that this was a race condition, depending on latency, so it only manifested itself in certain conditions to begin with.

However, I'll close this now. We can always reopen if it would resurface. Thanks for pinging!

sly7-7 commented 10 years ago

Thanks :)