background-sync's RequestQueue class has non-deterministic behavior

[philipwalton]

As I was looking into converting workbox-background-sync to v3, I notice a potentially problematic issue where the generated IndexedDB object store name can be non-deterministic. Here's the basic gist of the code that illustrates the issue:

import {defaultQueueName} from './constants';

let _queueCounter = 0;

class RequestQueue {
  constructor({queueName = defaultQueueName + '_' + _queueCounter++}) {
    this._queueName = queueName;
    // ...
  }

  async initQueue() {
    const idbQueue = await this.db.get(this._queueName);
    // ...
  }
  // ...
}

In the above code, setting the queueName value to defaultQueueName + '_' + _queueCounter++ will guarantee the queue name is unique; however, since the service worker doesn't stay alive the entire time the page is alive, there's no guarantee that the second time the service worker starts up, it'll generate the same object store name for this queue.

This means that when the sync event fires, the logic used to store the queue in IndexedDB may be replaying an entirely different set of requests.

I think most of the time this isn't a concern because presumably most queues are instantiated synchronously in the top-level execution context of the service worker (i.e. not in an event listener) and will instantiate in the same order every time, but since we can't guarantee that will always be the case, I think we need to change this.

What should change

I don't think there's any way to use a default queue name (i.e. object store name) without potentially conflicting with other RequestQueue instances, so I propose we require background sync queue instances to supply their own object store name (and we can warn that the name shouldn't be randomly generated).

Furthermore, I think we should call this property objectStoreName or something that makes it clear this name is persisting to the database and needs to be the same each time the code is run or the replay won't work.

Open questions

Do we want to fix this now or wait until v3? I think it's probably safe to wait until v3 since it'll require a major version bump anyway (since it's a breaking change), but I wanted to hear what others think.

/cc @gauntface @jeffposnick @addyosmani @prateekbh

[gauntface]

I would fix this in V3, it seems like a bit big change in terms of API and usage pattern.

I suspect we haven't caught this because testing most likely doesn't handle terminating and bringing up a new service worker instance (perhaps something an integration test should be doing (killing browser and starting it up a second time).

Either way - good spot and + 1 refactoring in v3

[addyosmani]

I think most of the time this isn't a concern because presumably most queues are instantiated synchronously in the top-level execution context of the service worker (i.e. not in an event listener) and will instantiate in the same order every time, but since we can't guarantee that will always be the case, I think we need to change this.

Euuugh. Thanks for spotting this, @philipwalton. That's a pretty gnarly non-determinism issue I don't think we would have spotted without more manual digging.

Do we want to fix this now or wait until v3? I think it's probably safe to wait until v3 since it'll require a major version bump anyway (since it's a breaking change), but I wanted to hear what others think.

I agree with Matt that addressing this in V3 sounds reasonable given the likely scope of change needed.

[philipwalton]

This should be fixed in v3 via #871.