Skip to content

Commit

Permalink
Ensure adapters and serializers are destroyed upon store destruction.
Browse files Browse the repository at this point in the history
Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
  • Loading branch information
rwjblue committed Jan 30, 2020
1 parent d6bd5f1 commit bf459de
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 4 deletions.
1 change: 1 addition & 0 deletions packages/-ember-data/node-tests/fixtures/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ module.exports = {
'(public) @ember-data/adapter MinimumAdapterInterface#shouldReloadAll [OPTIONAL]',
'(public) @ember-data/adapter MinimumAdapterInterface#shouldReloadRecord [OPTIONAL]',
'(public) @ember-data/adapter MinimumAdapterInterface#updateRecord',
'(public) @ember-data/adapter MinimumAdapterInterface#destroy [OPTIONAL]',
'(public) @ember-data/adapter RESTAdapter#coalesceFindRequests',
'(public) @ember-data/adapter RESTAdapter#createRecord',
'(public) @ember-data/adapter RESTAdapter#deleteRecord',
Expand Down
28 changes: 28 additions & 0 deletions packages/-ember-data/tests/integration/store/adapter-for-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { assign } from '@ember/polyfills';
import { run } from '@ember/runloop';

import { module, test } from 'qunit';

Expand Down Expand Up @@ -278,4 +279,31 @@ module('integration/store - adapterFor', function(hooks) {
);
assert.ok(jsonApiAdapter === adapter, 'We fell back to the -json-api adapter instance for the per-type adapter');
});

test('adapters are destroyed', async function(assert) {
let { owner } = this;
let didInstantiate = false;
let didDestroy = false;

class AppAdapter extends TestAdapter {
didInit() {
didInstantiate = true;
}

destroy() {
didDestroy = true;
}
}

owner.register('adapter:application', AppAdapter);

let adapter = store.adapterFor('application');

assert.ok(adapter instanceof AppAdapter, 'precond - We found the correct adapter');
assert.ok(didInstantiate, 'precond - We instantiated the adapter');

run(store, 'destroy');

assert.ok(didDestroy, 'adapter was destroyed');
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { assign } from '@ember/polyfills';
import { run } from '@ember/runloop';

import { module, test } from 'qunit';

Expand Down Expand Up @@ -454,4 +455,31 @@ module('integration/store - serializerFor', function(hooks) {
);
}
);

test('serializers are destroyed', async function(assert) {
let { owner } = this;
let didInstantiate = false;
let didDestroy = false;

class AppSerializer extends TestSerializer {
didInit() {
didInstantiate = true;
}

destroy() {
didDestroy = true;
}
}

owner.register('serializer:application', AppSerializer);

let serializer = store.serializerFor('application');

assert.ok(serializer instanceof AppSerializer, 'precond - We found the correct serializer');
assert.ok(didInstantiate, 'precond - We instantiated the serializer');

run(store, 'destroy');

assert.ok(didDestroy, 'serializer was destroyed');
});
});
23 changes: 19 additions & 4 deletions packages/store/addon/-private/system/core-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3487,14 +3487,29 @@ abstract class CoreStore extends Service {
}
}

destroy() {
// enqueue destruction of any adapters/serializers we have created
for (let adapterName in this._adapterCache) {
let adapter = this._adapterCache[adapterName];
if (typeof adapter.destroy === 'function') {
adapter.destroy();
}
}

for (let serializerName in this._serializerCache) {
let serializer = this._serializerCache[serializerName];
if (typeof serializer.destroy === 'function') {
serializer.destroy();
}
}

return super.destroy();
}

willDestroy() {
super.willDestroy();
this.recordArrayManager.destroy();

// Check if we need to null this out
// this._adapterCache = null;
// this._serializerCache = null;

identifierCacheFor(this).destroy();

this.unloadAll();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,18 @@ interface Adapter {
* @return {boolean} true if the a new request for all records of the type in SnapshotRecordArray should be made in the background, false otherwise
*/
shouldBackgroundReloadAll?(store: Store, snapshotArray: SnapshotRecordArray): boolean;

/**
* In some situations the adapter may need to perform cleanup when destroyed,
* that cleanup can be done in `destroy`.
*
* If not implemented, the store does not inform the adapter of destruction.
*
* @method destroy [OPTIONAL]
* @public
* @optional
*/
destroy?(): void;
}

export default Adapter;

0 comments on commit bf459de

Please sign in to comment.