From 73f8006297eb9bc0fca0c43331f546dc97014e4c Mon Sep 17 00:00:00 2001 From: Philip Walton Date: Wed, 6 Mar 2019 22:53:47 -0800 Subject: [PATCH] Prevent onSync infinite loops --- packages/workbox-background-sync/Queue.mjs | 43 ++++++- .../workbox-google-analytics/initialize.mjs | 2 +- .../node/test-Queue.mjs | 106 ++++++++++++++---- .../integration/basic-example.js | 6 +- .../static/basic-example/sw.js | 14 +++ 5 files changed, 143 insertions(+), 28 deletions(-) diff --git a/packages/workbox-background-sync/Queue.mjs b/packages/workbox-background-sync/Queue.mjs index 61763b58a..ad777269d 100644 --- a/packages/workbox-background-sync/Queue.mjs +++ b/packages/workbox-background-sync/Queue.mjs @@ -41,6 +41,8 @@ class Queue { * containing the `queue` property (referencing this instance), and you * can use the callback to customize the replay behavior of the queue. * When not set the `replayRequests()` method is called. + * Note: if the replay fails after a sync event, make sure you throw an + * error, so the browser knows to retry the sync event later. * @param {number} [options.maxRetentionTime=7 days] The amount of time (in * minutes) a request may be retried. After this amount of time has * passed, the request will be deleted from the queue. @@ -183,11 +185,20 @@ class Queue { } await this._queueStore[`${operation}Entry`](entry); - await this.registerSync(); + if (process.env.NODE_ENV !== 'production') { logger.log(`Request for '${getFriendlyURL(request.url)}' has ` + `been added to background sync queue '${this._name}'.`); } + + // Don't register for a sync if we're in the middle of a sync. Instead, + // we wait until the sync is complete and call register if + // `this._requestsAddedDuringSync` is true. + if (this._syncInProgress) { + this._requestsAddedDuringSync = true; + } else { + await this.registerSync(); + } } /** @@ -280,7 +291,35 @@ class Queue { logger.log(`Background sync for tag '${event.tag}'` + `has been received`); } - event.waitUntil(this._onSync({queue: this})); + + const syncComplete = async () => { + this._syncInProgress = true; + + let syncError; + try { + await this._onSync({queue: this}); + } catch (error) { + syncError = error; + + // Rethrow the error. Note: the logic in the finally clause + // will run before this gets rethrown. + throw syncError; + } finally { + // New items may have been added to the queue during the sync, + // so we need to register for a new sync if that's happened... + // Unless there was an error during the sync, in which + // case the browser will automatically retry later, as long + // as `event.lastChance` is not true. + if (this._requestsAddedDuringSync && + !(syncError && !event.lastChance)) { + await this.registerSync(); + } + + this._syncInProgress = false; + this._requestsAddedDuringSync = false; + } + }; + event.waitUntil(syncComplete()); } }); } else { diff --git a/packages/workbox-google-analytics/initialize.mjs b/packages/workbox-google-analytics/initialize.mjs index df2de2fbf..f3609339c 100644 --- a/packages/workbox-google-analytics/initialize.mjs +++ b/packages/workbox-google-analytics/initialize.mjs @@ -94,7 +94,7 @@ const createOnSyncCallback = (config) => { logger.log(`Request for '${getFriendlyURL(url.href)}'` + `failed to replay, putting it back in the queue.`); } - return; + throw err; } } if (process.env.NODE_ENV !== 'production') { diff --git a/test/workbox-background-sync/node/test-Queue.mjs b/test/workbox-background-sync/node/test-Queue.mjs index ae10dedcb..20d225706 100644 --- a/test/workbox-background-sync/node/test-Queue.mjs +++ b/test/workbox-background-sync/node/test-Queue.mjs @@ -7,6 +7,7 @@ */ import {expect} from 'chai'; +import {reset as iDBReset} from 'shelving-mock-indexeddb'; import sinon from 'sinon'; import expectError from '../../../infra/testing/expectError'; import {Queue} from '../../../packages/workbox-background-sync/Queue.mjs'; @@ -21,45 +22,52 @@ const getObjectStoreEntries = async () => { }; const createSyncEventStub = (tag) => { - const event = new SyncEvent('sync', {tag}); + const ret = { + event: new SyncEvent('sync', {tag}), + // Default to resolving in the next microtask. + done: Promise.resolve(), - // Default to resolving in the next microtask. - let done = Promise.resolve(); + }; // Browsers will throw if code tries to call `waitUntil()` on a user-created // sync event, so we have to stub it. - event.waitUntil = (promise) => { + ret.event.waitUntil = (promise) => { // If `waitUntil` is called, defer `done` until after it resolves. if (promise) { - done = promise.then(done); + // Catch failures since all we care about is finished. + ret.done = promise.catch(() => undefined).then(ret.done); } }; - return {event, done}; -}; - -const clearIndexedDBEntries = async () => { - // Open a conection to the database (at whatever version exists) and - // clear out all object stores. This strategy is used because deleting - // databases inside service worker is flaky in FF and Safari. - // TODO(philipwalton): the version is not needed in real browsers, so it - // can be removed when we move away from running tests in node. - const db = await new DBWrapper('workbox-background-sync', 3).open(); - - // Edge cannot convert a DOMStringList to an array via `[...list]`. - for (const store of Array.from(db.db.objectStoreNames)) { - await db.clear(store); - } - await db.close(); + return ret; }; +// TODO(philipwalton): uncomment once we move away from the IDB mocks. +// const clearIndexedDBEntries = async () => { +// // Open a conection to the database (at whatever version exists) and +// // clear out all object stores. This strategy is used because deleting +// // databases inside service worker is flaky in FF and Safari. +// // TODO(philipwalton): the version is not needed in real browsers, so it +// // can be removed when we move away from running tests in node. +// const db = await new DBWrapper('workbox-background-sync').open(); + +// // Edge cannot convert a DOMStringList to an array via `[...list]`. +// for (const store of Array.from(db.db.objectStoreNames)) { +// await db.clear(store); +// } +// await db.close(); +// }; describe(`Queue`, function() { const sandbox = sinon.createSandbox(); beforeEach(async function() { Queue._queueNames.clear(); - await clearIndexedDBEntries(); + + // TODO(philipwalton): remove `iDBReset()` and re-add + // `clearIndexedDBEntries()` once we move away from the mocks. + // await clearIndexedDBEntries(); + iDBReset(); // Don't actually register for a sync event in any test, as it could // make the tests non-deterministic. @@ -90,7 +98,7 @@ describe(`Queue`, function() { }).not.to.throw(); }); - it(`adds a sync event listener runs the onSync function when a sync event is dispatched`, async function() { + it(`adds a sync event listener that runs the onSync function when a sync event is dispatched`, async function() { sandbox.spy(self, 'addEventListener'); const onSync = sandbox.spy(); @@ -136,6 +144,58 @@ describe(`Queue`, function() { .to.equal(queue); }); + it(`registers a tag if entries were added to the queue during a successful sync`, async function() { + const onSync = sandbox.stub().callsFake(async ({queue}) => { + await queue.pushRequest({request: new Request('/one')}); + await queue.pushRequest({request: new Request('/two')}); + await queue.pushRequest({request: new Request('/three')}); + }); + + const queue = new Queue('foo', {onSync}); + sandbox.spy(queue, 'registerSync'); + + const sync1 = createSyncEventStub('workbox-background-sync:foo'); + self.dispatchEvent(sync1.event); + await sync1.done; + + expect(queue.registerSync.callCount).to.equal(1); + }); + + it(`doesn't re-register after a sync event fails`, async function() { + const onSync = sandbox.stub().callsFake(async ({queue}) => { + await queue.pushRequest({request: new Request('/one')}); + throw new Error('sync failed'); + }); + + const queue = new Queue('foo', {onSync}); + sandbox.spy(queue, 'registerSync'); + + const sync1 = createSyncEventStub('workbox-background-sync:foo'); + self.dispatchEvent(sync1.event); + + await sync1.done; + + expect(queue.registerSync.callCount).to.equal(0); + }); + + it(`re-registers a tag after a sync event fails if event.lastChance is true`, async function() { + const onSync = sandbox.stub().callsFake(async ({queue}) => { + await queue.pushRequest({request: new Request('/one')}); + throw new Error('sync failed'); + }); + + const queue = new Queue('foo', {onSync}); + sandbox.spy(queue, 'registerSync'); + + const sync1 = createSyncEventStub('workbox-background-sync:foo'); + sync1.event.lastChance = true; + self.dispatchEvent(sync1.event); + + await sync1.done; + + expect(queue.registerSync.callCount).to.equal(1); + }); + it(`tries to run the sync logic on instantiation in browsers that don't support the sync event`, async function() { // Delete the SyncManager interface to mock a non-supporting browser. const originalSyncManager = registration.sync; diff --git a/test/workbox-google-analytics/integration/basic-example.js b/test/workbox-google-analytics/integration/basic-example.js index 212789a88..d70f8ca54 100644 --- a/test/workbox-google-analytics/integration/basic-example.js +++ b/test/workbox-google-analytics/integration/basic-example.js @@ -115,9 +115,11 @@ describe(`[workbox-google-analytics] Load and use Google Analytics`, function() }); expect(requests).to.have.lengthOf(0); - // Uncheck the "simulate offline" checkbox, which should let queued - // requests start to replay successfully. + // Uncheck the "simulate offline" checkbox and then trigger a sync. await simulateOfflineEl.click(); + await driver.executeAsyncScript(messageSW, { + action: 'dispatch-sync-event', + }); // Wait until all expected requests have replayed. await waitUntil(async () => { diff --git a/test/workbox-google-analytics/static/basic-example/sw.js b/test/workbox-google-analytics/static/basic-example/sw.js index 4276b97f6..fbe977d24 100644 --- a/test/workbox-google-analytics/static/basic-example/sw.js +++ b/test/workbox-google-analytics/static/basic-example/sw.js @@ -52,6 +52,20 @@ self.addEventListener('message', (evt) => { case 'get-spied-requests': evt.ports[0].postMessage(spiedRequests); break; + case 'dispatch-sync-event': + { + // Override `.waitUntil` so we can signal when the sync is done. + const originalSyncEventWaitUntil = SyncEvent.prototype.waitUntil; + SyncEvent.prototype.waitUntil = (promise) => { + return promise.then(() => evt.ports[0].postMessage(null)); + }; + + self.dispatchEvent(new SyncEvent('sync', { + tag: 'workbox-background-sync:workbox-google-analytics', + })); + SyncEvent.prototype.waitUntil = originalSyncEventWaitUntil; + break; + } } });