Skip to content

Commit

Permalink
Free up internalModels
Browse files Browse the repository at this point in the history
`internalModel`s need to stay around as long as they're connected to a live
record via relationships.  Once an entire subgraph is removed however, we can
free all of it.

Also add a couple of "late private" fields to `internalModel`'s constructor for
shape preservation.

Includes work from @igorT and @sly7-7

[Fix emberjs#3296]
[Supercede emberjs#3301]
  • Loading branch information
hjdivad committed Oct 19, 2016
1 parent 1ebbf5e commit 75c1a75
Show file tree
Hide file tree
Showing 10 changed files with 196 additions and 92 deletions.
99 changes: 84 additions & 15 deletions addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,20 +95,18 @@ export default function InternalModel(type, id, store, _, data) {
this.store = store;
this._data = data || new EmptyObject();
this.modelName = type.modelName;
this.dataHasInitialized = false;
this._loadingPromise = null;
//Look into making this lazy
this._deferredTriggers = [];
this._attributes = new EmptyObject();
this._inFlightAttributes = new EmptyObject();
this._relationships = new Relationships(this);
this._recordArrays = undefined;
this.currentState = RootState.empty;
this.recordReference = new RecordReference(store, this);
this.references = {};
this.isReloading = false;
this._isDestroyed = false;
this.isError = false;
this.error = null;
this._resetRecord();
this._attributes = new EmptyObject();
this._inFlightAttributes = new EmptyObject();
this.__ember_meta__ = null;
this[Ember.GUID_KEY] = Ember.guidFor(this);
/*
Expand Down Expand Up @@ -184,6 +182,14 @@ InternalModel.prototype = {
this.record = null;
},

dematerializeRecord() {
if (this.record) {
this.record.destroy();

this.updateRecordArrays();
}
},

deleteRecord() {
this.send('deleteRecord');
},
Expand Down Expand Up @@ -235,8 +241,68 @@ InternalModel.prototype = {
return this.record;
},

unloadRecord() {
directlyRelatedInternalModels() {
var array = [];
this.type.eachRelationship((key, relationship) => {
if (this._relationships.has(key)) {
var related = this._relationships.get(key).members.toArray();
array.push(...related);
}
});
return array;
},

allRelatedInternalModels() {
let array = [];
let queue = [];
let seen = [];
queue.push(this);
seen.push(this);
while (queue.length > 0) {
let node = queue.shift();
array.push(node);
let related = node.directlyRelatedInternalModels();
related.forEach(function(internalModel) {
if (seen.indexOf(internalModel) === -1) {
queue.push(internalModel);
seen.push(internalModel);
}
});
}
return array;
},

unloadRecord: function() {
this.send('unloadRecord');
this.dematerializeRecord();
var relatedInternalModels = this.allRelatedInternalModels();
var allUnloaded = true;
var record;
for (var i=0; i < relatedInternalModels.length; i++) {
record = relatedInternalModels[i].record;
if (record && !(record.get('isDestroyed') || record.get('isDestroying'))) {
allUnloaded = false;
break;
}
}
if (allUnloaded) {
relatedInternalModels.forEach((internalModel) => {
if (!internalModel.isDestroyed) {
internalModel.destroy();
}
});
}
},

destroy: function() {
this._isDestroyed = true;
Ember.run.schedule('destroy', this, this.willDestroy, this);
},

willDestroy() {
Ember.assert("Cannot destroy an internalModel while its record is materialized", !this.record);
this.store._removeFromIdMap(this);
//other cleanup
},

eachRelationship(callback, binding) {
Expand Down Expand Up @@ -277,14 +343,7 @@ InternalModel.prototype = {
return this._isDestroyed;
},

destroy() {
this._isDestroyed = true;
if (this.record) {
return this.record.destroy();
}
},

/*
/**
@method createSnapshot
@private
*/
Expand Down Expand Up @@ -324,6 +383,7 @@ InternalModel.prototype = {
@private
*/
pushedData() {
this._isDestroyed = false;
this.send('pushedData');
},

Expand Down Expand Up @@ -584,6 +644,15 @@ InternalModel.prototype = {
});
},

_resetRecord() {
this.dataHasInitialized = false;
this.currentState = RootState.empty;
this.isReloading = false;
this.isError = false;
this.error = null;
this.record = null;
},

/*
When a find request is triggered on the store, the user can optionally pass in
attributes and relationships to be preloaded. These are meant to behave as if they
Expand Down
6 changes: 3 additions & 3 deletions addon/-private/system/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -803,9 +803,9 @@ var Model = Ember.Object.extend(Ember.Evented, {
willDestroy() {
//TODO Move!
this._super(...arguments);
this._internalModel.clearRelationships();
this._internalModel.recordObjectWillDestroy();
//TODO should we set internalModel to null here?

this._internalModel._resetRecord();
this._internalModel = null;
},

// This is a temporary solution until we refactor DS.Model to not
Expand Down
8 changes: 0 additions & 8 deletions addon/-private/system/model/states.js
Original file line number Diff line number Diff line change
Expand Up @@ -461,10 +461,6 @@ const RootState = {
// you out of the in-flight state.
rolledBack() { },
unloadRecord(internalModel) {
// clear relationships before moving to deleted state
// otherwise it fails
internalModel.clearRelationships();
internalModel.transitionTo('deleted.saved');
},

propertyWasReset() { },
Expand Down Expand Up @@ -575,10 +571,6 @@ const RootState = {
},

unloadRecord(internalModel) {
// clear relationships before moving to deleted state
// otherwise it fails
internalModel.clearRelationships();
internalModel.transitionTo('deleted.saved');
},

didCommit(internalModel) {
Expand Down
1 change: 0 additions & 1 deletion addon/-private/system/record-array-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ export default Ember.Object.extend({
updateRecordArrays() {
heimdall.increment(updateRecordArrays);
this.changedRecords.forEach(internalModel => {

if (internalModel.isDestroyed ||
internalModel.currentState.stateName === 'root.deleted.saved') {
this._recordWasDeleted(internalModel);
Expand Down
14 changes: 8 additions & 6 deletions addon/-private/system/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -1610,19 +1610,19 @@ Store = Service.extend({
let types = new Array(keys.length);

for (let i = 0; i < keys.length; i++) {
types[i] = typeMaps[keys[i]]['type'].modelName;
types[i] = typeMaps[keys[i]].type.modelName;
}

types.forEach(this.unloadAll, this);
} else {
let typeClass = this.modelFor(modelName);
let typeMap = this.typeMapFor(typeClass);
let records = typeMap.records.slice();
let record;
let internalModel;

for (let i = 0; i < records.length; i++) {
record = records[i];
this.unloadRecord(record);
internalModel = records[i];
internalModel.unloadRecord();
}

typeMap.metadata = new EmptyObject();
Expand Down Expand Up @@ -2430,12 +2430,14 @@ Store = Service.extend({
@param {InternalModel} internalModel
*/
_dematerializeRecord(internalModel) {
internalModel.dematerializeRecord();
},

_removeFromIdMap(internalModel) {
var type = internalModel.type;
var typeMap = this.typeMapFor(type);
var id = internalModel.id;

internalModel.updateRecordArrays();

if (id) {
delete typeMap.idToRecord[id];
}
Expand Down
9 changes: 2 additions & 7 deletions tests/integration/records/load-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,14 @@ module("integration/load - Loading Records", {
}
});

test("When loading a record fails, the isLoading is set to false", function(assert) {
test("When loading a record fails, the record is not left behind", function(assert) {
env.adapter.findRecord = function(store, type, id, snapshot) {
return Ember.RSVP.reject();
};

run(function() {
env.store.findRecord('post', 1).then(null, assert.wait(function() {
// store.recordForId is private, but there is currently no other way to
// get the specific record instance, since it is not passed to this
// rejection handler
var post = env.store.recordForId('post', 1);

assert.equal(post.get("isLoading"), false, "post is not loading anymore");
assert.equal(env.store.hasRecordForId('post', 1), false);
}));
});
});
99 changes: 76 additions & 23 deletions tests/integration/records/unload-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ test("unloading all records also updates record array from peekAll()", function(
});


test("unloading a record also clears its relationship", function(assert) {
test("a relationship to an unloaded record can be restored", function(assert) {
var adam, bob;

// disable background reloading so we do not re-create the relationship.
Expand Down Expand Up @@ -289,55 +289,108 @@ test("unloading a record also clears its relationship", function(assert) {
person.unloadRecord();
});

assert.equal(person.get('cars.length'), undefined);
assert.expectAssertion(function () {
bob.get('person');
}, /You looked up the 'person' relationship on a 'car' with id 1 but some of the associated records were not loaded/);

adam = person;
});
});
});

test("unloading a record also clears the implicit inverse relationships", function(assert) {
var adam, bob;
// disable background reloading so we do not re-create the relationship.
env.adapter.shouldBackgroundReloadRecord = () => false;

run(function() {
run(function () {
env.store.push({
data: {
type: 'person',
id: '1',
attributes: {
name: 'Adam Sunderland'
},
relationships: {
cars: {
data: [
{ type: 'car', id: '1' }
]
}
}
}
});
adam = env.store.peekRecord('person', 1);
});

run(function() {
var rematerializedPerson = bob.get('person');
assert.equal(rematerializedPerson.get('.id'), '1');
// the person is rematerialized; the previous person is *not* re-used
assert.notEqual(rematerializedPerson, adam);
});


test('unloading a disconnected subgraph clears the relevant internal models', function(assert) {
env.adapter.shouldBackgroundReloadRecord = () => false;

run(function () {
env.store.push({
data: {
type: 'group',
type: 'person',
id: '1',
attributes: {
name: 'Could be Anybody'
},
relationships: {
people: {
cars: {
data: [
{ type: 'person', id: '1' }
{ type: 'car', id: '1' },
{ type: 'car', id: '2' }
]
}
}
}
});
bob = env.store.peekRecord('group', 1);
});

run(function() {
env.store.findRecord('group', 1).then(function(group) {
assert.equal(group.get('people.length'), 1, 'The inital length of people is correct');
var person = env.store.peekRecord('person', 1);
run(function() {
person.unloadRecord();
});
run(function () {
env.store.push({
data: {
type: 'car',
id: '1',
attributes: {
make: 'Nissan',
model: 'Altima'
},
relationships: {
person: {
data: { type: 'person', id: '1' }
}
}
}
});
});

assert.equal(group.get('people.length'), 0, 'Person was removed from the people array');
run(function () {
env.store.push({
data: {
type: 'car',
id: '2',
attributes: {
make: 'Tesla',
model: 'S'
},
relationships: {
person: {
data: { type: 'person', id: '1' }
}
}
}
});
});

assert.equal(env.store.typeMapFor(env.store.modelFor('person')).records.length, 1);
assert.equal(env.store.typeMapFor(env.store.modelFor('car')).records.length, 2);

run(function () {
env.store.peekRecord('person', 1).unloadRecord();
env.store.peekRecord('car', 1).unloadRecord();
env.store.peekRecord('car', 2).unloadRecord();
});

assert.equal(env.store.typeMapFor(env.store.modelFor('person')).records.length, 0);
assert.equal(env.store.typeMapFor(env.store.modelFor('car')).records.length, 0);
});
Loading

0 comments on commit 75c1a75

Please sign in to comment.