WICG / cookie-store

Asynchronous access to cookies from JavaScript
https://wicg.github.io/cookie-store/
Apache License 2.0
143 stars 35 forks source link

Revise Change Events API. #111

Closed bunneyster closed 4 years ago

bunneyster commented 4 years ago

This revision ties subscriptions to a service worker registration instead of a service worker version. Since multiple service workers at different lifecycle stages may co-exist, it is difficult to reason about how to reconcile subscription/unsubscriptions from each service worker. Associating the subscriptions to the registration is clearer since only 1 registration exists at a time.

mkruisselbrink commented 4 years ago

FWIW, not supporting unsubscribing at all, and only supporting subscribing during the install event was supposed to make it easy to reason about what service worker gets which events. I.e. the version of the service worker that subscribed to cookie notifications will get exactly the events the subscribed for, no more, no less.

I'd say in some ways it becomes much harder to reason about which service worker gets which events after this PR. I.e. now a service worker registering for cookie change events in its install event will actually cause the previous version of the service worker to start receiving notification.

Also not entirely sure what use cases require a document being able to register for cookie change events on behalf of a service worker? If we do indeed need to support that, the changes probably makes sense, but I'm not sure what use cases that would address?

mkruisselbrink commented 4 years ago

(more for background: To make it clear you could only subscribe during the install event, the original explainer also had registering for change events be part of the InstallEvent interface. Not sure why that API was changed, the commit (38e0f5c94192f855e58261bcb49f5a9650b66cf2) where that change to the explainer was made doesn't seem to explain what prompted the changes).

bunneyster commented 4 years ago

@mkruisselbrink Good point; I suppose the developer would have to take precautions in their change event handler if a subscription introduced by a newer version could potentially yield extraneous change events.

pwnall commented 4 years ago

@mek Thank you for raising concerns here!

I initiated this change after learning that we can have multiple SWs with the same scope running simultaneously.

I was concerned that in the previous model (which I liked for a bunch of reasons), when a cookie change happens, it's unclear which SW would handle it, in the case where we have two SWs (with their own subscription lists) running for the same scope. In the current model, once the registration has a matching subscription, we'll route cookie changes to the active SW.

Right now, it seems to me that the newer model is easier to reason about. I think the new model has a higher upfront cost for developers -- as you pointed out, apps have to keep the code handling the registration in sync with the code in the SW. On the flip side, I think the new model handles race conditions better, so it's gentler when it comes to edge cases.

WDYT?

mkruisselbrink commented 4 years ago

Per offline conversation, I think I now better understand the motivation for this change. I.e. the concern is that in the old model there is some time between when a service worker registers for cookie change notifications (in the install event) and when it actually starts receiving events (after it is activated). And that time means that a naive implementation that snapshots the current state of the cookies the service worker is interested in at install time might end up with a snapshot that is out of sync (because of these missed events).

In the newly proposed model this isn't as much a concern since after registering for cookie change events at least some service worker will immediately start receiving events. In the new model we should strongly discourage service workers from registering for cookie change events in their install event, since that could still result in the same race conditions where the old version of the service worker will get the cookie change events until the new version gets activated (and that version might not know what to do with them).

From offline conversation (although not reflected in the current pull request) it is also the intention to reject "subscribe" calls if there is no active version yet (to avoid having a period of time when there are cookie change subscriptions but no service worker to dispatch events to). However the sample code as currently written in this PR wouldn't work with that requirement, as the promise returned by navigator.serviceWorker.register() resolves before the newly installed worker is activated (it is resolved in step 7 in https://w3c.github.io/ServiceWorker/#install, and it isn't until step 22 when the newly installed worker could become active).

I'd also be curious which model actual web developers would find easier to reason about/program against. Both seem like they have rough edges and edge cases where the behavior might be different than expected, so it's not clear to me which one would make more sense from that point of view.

(perhaps a middle ground, that would address what I understand is the problem with the existing model, would be to keep pretty much everything the same, except we only allow registration for cookie change events at the time a service worker becomes active, rather than during the install event. It appears to me that would solve all the race conditions, without introducing a different set of race conditions?)

pwnall commented 4 years ago

@mkruisselbrink I'm not sure what you mean by "keep pretty much everything the same" -- does "the same" refer to this PR's proposal, or to the status quo?

mkruisselbrink commented 4 years ago

@mkruisselbrink I'm not sure what you mean by "keep pretty much everything the same" -- does "the same" refer to this PR's proposal, or to the status quo?

That was referring to the status quo, i.e. the only change to the status quo would be to change "Service workers have to subscribe for change events during the install stage" to "Service workers have to subscribe for change events during the activate stage"

pwnall commented 4 years ago

Thank you for the quick reply!

If I'm understanding correctly, your proposal is that each SW version starts with a blank slate of subscriptions, and populates its subscriptions during the activate stage.

Does this mean that there's a window of time when a SW doesn't observe any cookie changes while it's being updated? Would the SW have to re-snapshot the cookie jar on activate?

mkruisselbrink commented 4 years ago

If I'm understanding correctly, your proposal is that each SW version starts with a blank slate of subscriptions, and populates its subscriptions during the activate stage.

Yes, that's correct.

Does this mean that there's a window of time when a SW doesn't observe any cookie changes while it's being updated?

While it is being activated at least, yes. From the moment the activate event is fired till the moment the service worker subscribes to cookie changes it will miss any changes. Subscribing during install didn't have that problem, but of course that had the problem that snapshotting during install would miss changes until activate, and snapshotting during activate might include changes for which it hasn't received events yet.

Would the SW have to re-snapshot the cookie jar on activate?

Yes, if a new version of the SW happens to want to monitor the same cookies as the previous version, it would have to re-snapshot the cookie jar on activate.

Although I wonder if in practice re-snapshotting during activate isn't what websites will have to (or at least end up) doing in the proposal in this PR anyway? At least the "first" version of a service worker will have to do snapshotting when subscribing for cookie change events, and (from what I understand) can't do that subscribing until its activate event. So the simplest solution seems for a service worker to just always snapshot, rather than first try to figure out what might have already been snapshotted and/or subscribed to and only subscribing/snapshotting if the previous version didn't already do so? It seems like any kind of logic that tries to branch on the state of an arbitrarily earlier version of the service worker would be needlessly fragile...

It would be interesting to see sample code for the use cases we apparently are trying to solve here (and explicitly mention in the explainer what those use cases are), rather than the heavily simplified code we have now. It appears every approach has potentially interesting edge cases, and I am not sure which edge cases are worse, or which edge cases are harder to work around... The current explainer really seems to focus more on describing the exact API and how it behaves, rather than describing what problems we're trying to solve and how the API can be used to solve those problems (and yes, that's also my fault). There are some use cases listed, but they don't involve service workers at all, and then the rest of the document just describes the various methods...

mkruisselbrink commented 4 years ago

Actually, what I said about no use cases involving service workers isn't true of course. There is the last use case that does involve service workers. But its code snippet doesn't try to do any of the snapshotting or anything, which makes it seem like that isn't actually needed for the use cases we're trying to address.

bunneyster commented 4 years ago

@pwnall Could you please take another look at these changes?

bunneyster commented 4 years ago

Thank you for the review!