From c68962078af93a81080aa1fa1c6a01b949ff5323 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 16 May 2019 20:19:49 -0700 Subject: [PATCH 1/2] Replace the adapter cancellation methods with a cancellation token system --- ui/app/adapters/watchable.js | 99 +++++++----------------------- ui/app/utils/classes/xhr-token.js | 11 ++++ ui/app/utils/properties/watch.js | 25 ++++---- ui/tests/unit/adapters/job-test.js | 64 +++++++------------ 4 files changed, 66 insertions(+), 133 deletions(-) create mode 100644 ui/app/utils/classes/xhr-token.js diff --git a/ui/app/adapters/watchable.js b/ui/app/adapters/watchable.js index c4fbc5ad8228..fd16fcdf0a12 100644 --- a/ui/app/adapters/watchable.js +++ b/ui/app/adapters/watchable.js @@ -1,4 +1,4 @@ -import { get, computed } from '@ember/object'; +import { get } from '@ember/object'; import { assign } from '@ember/polyfills'; import { inject as service } from '@ember/service'; import queryString from 'query-string'; @@ -9,52 +9,24 @@ export default ApplicationAdapter.extend({ watchList: service(), store: service(), - xhrs: computed(function() { - return { - list: {}, - track(key, xhr) { - if (this.list[key]) { - this.list[key].push(xhr); - } else { - this.list[key] = [xhr]; - } - }, - cancel(key) { - while (this.list[key] && this.list[key].length) { - this.remove(key, this.list[key][0]); - } - }, - remove(key, xhr) { - if (this.list[key]) { - xhr.abort(); - this.list[key].removeObject(xhr); - } - }, - }; - }), - - ajaxOptions() { + ajaxOptions(url, type, options) { const ajaxOptions = this._super(...arguments); - const key = this.xhrKey(...arguments); - - const previousBeforeSend = ajaxOptions.beforeSend; - ajaxOptions.beforeSend = function(jqXHR) { - if (previousBeforeSend) { - previousBeforeSend(...arguments); - } - this.xhrs.track(key, jqXHR); - jqXHR.always(() => { - this.xhrs.remove(key, jqXHR); - }); - }; + const cancellationToken = (options || {}).cancellationToken; + if (cancellationToken) { + delete options.cancellationToken; + + const previousBeforeSend = ajaxOptions.beforeSend; + ajaxOptions.beforeSend = function(jqXHR) { + cancellationToken.capture(jqXHR); + if (previousBeforeSend) { + previousBeforeSend(...arguments); + } + }; + } return ajaxOptions; }, - xhrKey(url, method /* options */) { - return `${method} ${url}`; - }, - findAll(store, type, sinceToken, snapshotRecordArray, additionalParams = {}) { const params = assign(this.buildQuery(), additionalParams); const url = this.urlForFindAll(type.modelName); @@ -63,7 +35,9 @@ export default ApplicationAdapter.extend({ params.index = this.watchList.getIndexFor(url); } + const cancellationToken = get(snapshotRecordArray || {}, 'adapterOptions.cancellationToken'); return this.ajax(url, 'GET', { + cancellationToken, data: params, }); }, @@ -76,7 +50,9 @@ export default ApplicationAdapter.extend({ params.index = this.watchList.getIndexFor(url); } + const cancellationToken = get(snapshot || {}, 'adapterOptions.cancellationToken'); return this.ajax(url, 'GET', { + cancellationToken, data: params, }).catch(error => { if (error instanceof AbortError) { @@ -86,7 +62,8 @@ export default ApplicationAdapter.extend({ }); }, - reloadRelationship(model, relationshipName, watch = false) { + reloadRelationship(model, relationshipName, options = { watch: false, cancellationToken: null }) { + const { watch, cancellationToken } = options; const relationship = model.relationshipFor(relationshipName); if (relationship.kind !== 'belongsTo' && relationship.kind !== 'hasMany') { throw new Error( @@ -110,6 +87,7 @@ export default ApplicationAdapter.extend({ } return this.ajax(url, 'GET', { + cancellationToken, data: params, }).then( json => { @@ -143,39 +121,4 @@ export default ApplicationAdapter.extend({ return this._super(...arguments); }, - - cancelFindRecord(modelName, id) { - if (!modelName || id == null) { - return; - } - const url = this.urlForFindRecord(id, modelName); - this.xhrs.cancel(`GET ${url}`); - }, - - cancelFindAll(modelName) { - if (!modelName) { - return; - } - let url = this.urlForFindAll(modelName); - const params = queryString.stringify(this.buildQuery()); - if (params) { - url = `${url}?${params}`; - } - this.xhrs.cancel(`GET ${url}`); - }, - - cancelReloadRelationship(model, relationshipName) { - if (!model || !relationshipName) { - return; - } - const relationship = model.relationshipFor(relationshipName); - if (relationship.kind !== 'belongsTo' && relationship.kind !== 'hasMany') { - throw new Error( - `${relationship.key} must be a belongsTo or hasMany, instead it was ${relationship.kind}` - ); - } else { - const url = model[relationship.kind](relationship.key).link(); - this.xhrs.cancel(`GET ${url}`); - } - }, }); diff --git a/ui/app/utils/classes/xhr-token.js b/ui/app/utils/classes/xhr-token.js new file mode 100644 index 000000000000..20b6386b3e95 --- /dev/null +++ b/ui/app/utils/classes/xhr-token.js @@ -0,0 +1,11 @@ +export default class XHRToken { + capture(xhr) { + this._xhr = xhr; + } + + abort() { + if (this._xhr) { + this._xhr.abort(); + } + } +} diff --git a/ui/app/utils/properties/watch.js b/ui/app/utils/properties/watch.js index e5f9464fec29..05c4ebfd92e3 100644 --- a/ui/app/utils/properties/watch.js +++ b/ui/app/utils/properties/watch.js @@ -3,12 +3,14 @@ import { get } from '@ember/object'; import RSVP from 'rsvp'; import { task } from 'ember-concurrency'; import wait from 'nomad-ui/utils/wait'; +import XHRToken from 'nomad-ui/utils/classes/xhr-token'; import config from 'nomad-ui/config/environment'; const isEnabled = config.APP.blockingQueries !== false; export function watchRecord(modelName) { return task(function*(id, throttle = 2000) { + const token = new XHRToken(); if (typeof id === 'object') { id = get(id, 'id'); } @@ -17,7 +19,7 @@ export function watchRecord(modelName) { yield RSVP.all([ this.store.findRecord(modelName, id, { reload: true, - adapterOptions: { watch: true }, + adapterOptions: { watch: true, cancellationToken: token }, }), wait(throttle), ]); @@ -25,9 +27,7 @@ export function watchRecord(modelName) { yield e; break; } finally { - this.store - .adapterFor(modelName) - .cancelFindRecord(modelName, id); + token.abort(); } } }).drop(); @@ -35,21 +35,20 @@ export function watchRecord(modelName) { export function watchRelationship(relationshipName) { return task(function*(model, throttle = 2000) { + const token = new XHRToken(); while (isEnabled && !Ember.testing) { try { yield RSVP.all([ this.store .adapterFor(model.constructor.modelName) - .reloadRelationship(model, relationshipName, true), + .reloadRelationship(model, relationshipName, { watch: true, cancellationToken: token }), wait(throttle), ]); } catch (e) { yield e; break; } finally { - this.store - .adapterFor(model.constructor.modelName) - .cancelReloadRelationship(model, relationshipName); + token.abort(); } } }).drop(); @@ -57,19 +56,21 @@ export function watchRelationship(relationshipName) { export function watchAll(modelName) { return task(function*(throttle = 2000) { + const token = new XHRToken(); while (isEnabled && !Ember.testing) { try { yield RSVP.all([ - this.store.findAll(modelName, { reload: true, adapterOptions: { watch: true } }), + this.store.findAll(modelName, { + reload: true, + adapterOptions: { watch: true, cancellationToken: token }, + }), wait(throttle), ]); } catch (e) { yield e; break; } finally { - this.store - .adapterFor(modelName) - .cancelFindAll(modelName); + token.abort(); } } }).drop(); diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index 34cf5fc9a8e3..f9c8687cd06b 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -1,10 +1,10 @@ -import EmberObject from '@ember/object'; import { run } from '@ember/runloop'; import { assign } from '@ember/polyfills'; import { settled } from '@ember/test-helpers'; import { setupTest } from 'ember-qunit'; import { module, test } from 'qunit'; import { startMirage } from 'nomad-ui/initializers/ember-cli-mirage'; +import XHRToken from 'nomad-ui/utils/classes/xhr-token'; module('Unit | Adapter | Job', function(hooks) { setupTest(hooks); @@ -253,12 +253,14 @@ module('Unit | Adapter | Job', function(hooks) { await this.initializeUI(); const { pretender } = this.server; + const token = new XHRToken(); + pretender.get('/v1/jobs', () => [200, {}, '[]'], true); this.subject() .findAll(null, { modelName: 'job' }, null, { reload: true, - adapterOptions: { watch: true }, + adapterOptions: { watch: true, cancellationToken: token }, }) .catch(() => {}); @@ -267,7 +269,7 @@ module('Unit | Adapter | Job', function(hooks) { // Schedule the cancelation before waiting run.next(() => { - this.subject().cancelFindAll('job'); + token.abort(); }); await settled(); @@ -279,12 +281,13 @@ module('Unit | Adapter | Job', function(hooks) { const { pretender } = this.server; const jobId = JSON.stringify(['job-1', 'default']); + const token = new XHRToken(); pretender.get('/v1/job/:id', () => [200, {}, '{}'], true); this.subject().findRecord(null, { modelName: 'job' }, jobId, { reload: true, - adapterOptions: { watch: true }, + adapterOptions: { watch: true, cancellationToken: token }, }); const { request: xhr } = pretender.requestReferences[0]; @@ -292,7 +295,7 @@ module('Unit | Adapter | Job', function(hooks) { // Schedule the cancelation before waiting run.next(() => { - this.subject().cancelFindRecord('job', jobId); + token.abort(); }); await settled(); @@ -304,17 +307,18 @@ module('Unit | Adapter | Job', function(hooks) { const { pretender } = this.server; const plainId = 'job-1'; + const token = new XHRToken(); const mockModel = makeMockModel(plainId); pretender.get('/v1/job/:id/summary', () => [200, {}, '{}'], true); - this.subject().reloadRelationship(mockModel, 'summary', true); + this.subject().reloadRelationship(mockModel, 'summary', true, token); const { request: xhr } = pretender.requestReferences[0]; assert.equal(xhr.status, 0, 'Request is still pending'); // Schedule the cancelation before waiting run.next(() => { - this.subject().cancelReloadRelationship(mockModel, 'summary'); + token.abort(); }); await settled(); @@ -326,20 +330,23 @@ module('Unit | Adapter | Job', function(hooks) { const { pretender } = this.server; const jobId = JSON.stringify(['job-1', 'default']); + const token1 = new XHRToken(); + const token2 = new XHRToken(); pretender.get('/v1/job/:id', () => [200, {}, '{}'], true); this.subject().findRecord(null, { modelName: 'job' }, jobId, { reload: true, - adapterOptions: { watch: true }, + adapterOptions: { watch: true, cancellationToken: token1 }, }); this.subject().findRecord(null, { modelName: 'job' }, jobId, { reload: true, - adapterOptions: { watch: true }, + adapterOptions: { watch: true, cancellationToken: token2 }, }); const { request: xhr } = pretender.requestReferences[0]; + const { request: xhr2 } = pretender.requestReferences[1]; assert.equal(xhr.status, 0, 'Request is still pending'); assert.equal(pretender.requestReferences.length, 2, 'Two findRecord requests were made'); assert.equal( @@ -348,44 +355,15 @@ module('Unit | Adapter | Job', function(hooks) { 'The two requests have the same URL' ); - // Schedule the cancelation before waiting - run.next(() => { - this.subject().cancelFindRecord('job', jobId); - }); - - await settled(); - assert.ok(xhr.aborted, 'Request was aborted'); - }); - - test('canceling a find record request will never cancel a request with the same url but different method', async function(assert) { - await this.initializeUI(); - - const { pretender } = this.server; - const jobId = JSON.stringify(['job-1', 'default']); - - pretender.get('/v1/job/:id', () => [200, {}, '{}'], true); - pretender.delete('/v1/job/:id', () => [204, {}, ''], 200); - - this.subject().findRecord(null, { modelName: 'job' }, jobId, { - reload: true, - adapterOptions: { watch: true }, - }); - - this.subject().stop(EmberObject.create({ id: jobId })); - - const { request: getXHR } = pretender.requestReferences[0]; - const { request: deleteXHR } = pretender.requestReferences[1]; - assert.equal(getXHR.status, 0, 'Get request is still pending'); - assert.equal(deleteXHR.status, 0, 'Delete request is still pending'); - - // Schedule the cancelation before waiting + // Schedule the cancelation and resolution before waiting run.next(() => { - this.subject().cancelFindRecord('job', jobId); + token1.abort(); + pretender.resolve(xhr2); }); await settled(); - assert.ok(getXHR.aborted, 'Get request was aborted'); - assert.notOk(deleteXHR.aborted, 'Delete request was aborted'); + assert.ok(xhr.aborted, 'Request one was aborted'); + assert.notOk(xhr2.aborted, 'Request two was not aborted'); }); test('when there is no region set, requests are made without the region query param', async function(assert) { From 3c1de2df913970b10c36be867d3c1cfd4a54e174 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 20 May 2019 11:08:16 -0700 Subject: [PATCH 2/2] Standardize on Abort over Cancel --- ui/app/adapters/watchable.js | 22 +++++++++++----------- ui/app/utils/properties/watch.js | 6 +++--- ui/tests/unit/adapters/job-test.js | 14 +++++++------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/ui/app/adapters/watchable.js b/ui/app/adapters/watchable.js index fd16fcdf0a12..bb2903ab447d 100644 --- a/ui/app/adapters/watchable.js +++ b/ui/app/adapters/watchable.js @@ -11,13 +11,13 @@ export default ApplicationAdapter.extend({ ajaxOptions(url, type, options) { const ajaxOptions = this._super(...arguments); - const cancellationToken = (options || {}).cancellationToken; - if (cancellationToken) { - delete options.cancellationToken; + const abortToken = (options || {}).abortToken; + if (abortToken) { + delete options.abortToken; const previousBeforeSend = ajaxOptions.beforeSend; ajaxOptions.beforeSend = function(jqXHR) { - cancellationToken.capture(jqXHR); + abortToken.capture(jqXHR); if (previousBeforeSend) { previousBeforeSend(...arguments); } @@ -35,9 +35,9 @@ export default ApplicationAdapter.extend({ params.index = this.watchList.getIndexFor(url); } - const cancellationToken = get(snapshotRecordArray || {}, 'adapterOptions.cancellationToken'); + const abortToken = get(snapshotRecordArray || {}, 'adapterOptions.abortToken'); return this.ajax(url, 'GET', { - cancellationToken, + abortToken, data: params, }); }, @@ -50,9 +50,9 @@ export default ApplicationAdapter.extend({ params.index = this.watchList.getIndexFor(url); } - const cancellationToken = get(snapshot || {}, 'adapterOptions.cancellationToken'); + const abortToken = get(snapshot || {}, 'adapterOptions.abortToken'); return this.ajax(url, 'GET', { - cancellationToken, + abortToken, data: params, }).catch(error => { if (error instanceof AbortError) { @@ -62,8 +62,8 @@ export default ApplicationAdapter.extend({ }); }, - reloadRelationship(model, relationshipName, options = { watch: false, cancellationToken: null }) { - const { watch, cancellationToken } = options; + reloadRelationship(model, relationshipName, options = { watch: false, abortToken: null }) { + const { watch, abortToken } = options; const relationship = model.relationshipFor(relationshipName); if (relationship.kind !== 'belongsTo' && relationship.kind !== 'hasMany') { throw new Error( @@ -87,7 +87,7 @@ export default ApplicationAdapter.extend({ } return this.ajax(url, 'GET', { - cancellationToken, + abortToken, data: params, }).then( json => { diff --git a/ui/app/utils/properties/watch.js b/ui/app/utils/properties/watch.js index 05c4ebfd92e3..ee6f5f9ec3cf 100644 --- a/ui/app/utils/properties/watch.js +++ b/ui/app/utils/properties/watch.js @@ -19,7 +19,7 @@ export function watchRecord(modelName) { yield RSVP.all([ this.store.findRecord(modelName, id, { reload: true, - adapterOptions: { watch: true, cancellationToken: token }, + adapterOptions: { watch: true, abortToken: token }, }), wait(throttle), ]); @@ -41,7 +41,7 @@ export function watchRelationship(relationshipName) { yield RSVP.all([ this.store .adapterFor(model.constructor.modelName) - .reloadRelationship(model, relationshipName, { watch: true, cancellationToken: token }), + .reloadRelationship(model, relationshipName, { watch: true, abortToken: token }), wait(throttle), ]); } catch (e) { @@ -62,7 +62,7 @@ export function watchAll(modelName) { yield RSVP.all([ this.store.findAll(modelName, { reload: true, - adapterOptions: { watch: true, cancellationToken: token }, + adapterOptions: { watch: true, abortToken: token }, }), wait(throttle), ]); diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index f9c8687cd06b..6c7c94503504 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -233,7 +233,7 @@ module('Unit | Adapter | Job', function(hooks) { const plainId = 'job-1'; const mockModel = makeMockModel(plainId); - this.subject().reloadRelationship(mockModel, 'summary', true); + this.subject().reloadRelationship(mockModel, 'summary', { watch: true }); assert.equal( pretender.handledRequests[0].url, '/v1/job/job-1/summary?index=1', @@ -241,7 +241,7 @@ module('Unit | Adapter | Job', function(hooks) { ); await settled(); - this.subject().reloadRelationship(mockModel, 'summary', true); + this.subject().reloadRelationship(mockModel, 'summary', { watch: true }); assert.equal( pretender.handledRequests[1].url, '/v1/job/job-1/summary?index=2', @@ -260,7 +260,7 @@ module('Unit | Adapter | Job', function(hooks) { this.subject() .findAll(null, { modelName: 'job' }, null, { reload: true, - adapterOptions: { watch: true, cancellationToken: token }, + adapterOptions: { watch: true, abortToken: token }, }) .catch(() => {}); @@ -287,7 +287,7 @@ module('Unit | Adapter | Job', function(hooks) { this.subject().findRecord(null, { modelName: 'job' }, jobId, { reload: true, - adapterOptions: { watch: true, cancellationToken: token }, + adapterOptions: { watch: true, abortToken: token }, }); const { request: xhr } = pretender.requestReferences[0]; @@ -311,7 +311,7 @@ module('Unit | Adapter | Job', function(hooks) { const mockModel = makeMockModel(plainId); pretender.get('/v1/job/:id/summary', () => [200, {}, '{}'], true); - this.subject().reloadRelationship(mockModel, 'summary', true, token); + this.subject().reloadRelationship(mockModel, 'summary', { watch: true, abortToken: token }); const { request: xhr } = pretender.requestReferences[0]; assert.equal(xhr.status, 0, 'Request is still pending'); @@ -337,12 +337,12 @@ module('Unit | Adapter | Job', function(hooks) { this.subject().findRecord(null, { modelName: 'job' }, jobId, { reload: true, - adapterOptions: { watch: true, cancellationToken: token1 }, + adapterOptions: { watch: true, abortToken: token1 }, }); this.subject().findRecord(null, { modelName: 'job' }, jobId, { reload: true, - adapterOptions: { watch: true, cancellationToken: token2 }, + adapterOptions: { watch: true, abortToken: token2 }, }); const { request: xhr } = pretender.requestReferences[0];