Prevent onSync infinite loops (#1943)

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 {

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') {

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;

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 () => {

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;
+    }
   }
 });