New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promise on navigator.serviceWorker that resolves when page is controlled #799

Closed
jeffposnick opened this Issue Dec 10, 2015 · 26 comments

Comments

Projects
None yet
5 participants
@jeffposnick
Contributor

jeffposnick commented Dec 10, 2015

In the following sample code (live version), I ended up making a wrong assumption that might catch other developers by surprise:

Excerpt from index.html:

navigator.serviceWorker.register('sw.js');
navigator.serviceWorker.ready.then(() => {
  // I thought the page would be controlled at this point, thanks to clients.claim()
  console.log('.ready resolved, and navigator.serviceWorker.controller is', navigator.serviceWorker.controller);
  navigator.serviceWorker.addEventListener('controllerchange', () => {
    console.log('Okay, now things are under control. navigator.serviceWorker.controller is', navigator.serviceWorker.controller);
  });
});

sw.js:

self.addEventListener('install', event => event.waitUntil(self.skipWaiting()));
self.addEventListener('activate', event => event.waitUntil(self.clients.claim()));

The assumption I had was that, by virtue of waiting on clients.claim() inside the SW's activate handler, the page will end up being controlled once .ready resolves. However, that's apparently a bad assumption, because the spec for clients.claim() states that the promise doesn't wait before resolving.

Is there any appetite for changing the behavior of clients.claim() so that the promise it returned waits until all the controller changes have taken place before it resolves? That would allow event.waitUntil(self.clients.claim()) within an activate handler to have the effect that I assumed it had, and it would by extension make the .ready promise more useful.

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 10, 2015

Member

Well, I think .ready() resolves as soon as the active worker is set. This is before the activate event is even dispatched. The SW running the activate event is already set as registration.active. So I don't think we can conceivably wait for the activate event's waitUntil() to resolve to get the effect you were expecting.

Also FWIW, the clients.claim() spec seems buggy at the moment. For example, it says Handle Service Worker Client Unload should be run for each client in parallel. This algorithm, however, does things like:

  • If registration's uninstalling flag is set, invoke Clear Registration algorithm passing registration as its argument and abort these steps.
  • If registration's waiting worker is not null, run Activate algorithm, or its equivalent, with registration as the argument.
Member

wanderview commented Dec 10, 2015

Well, I think .ready() resolves as soon as the active worker is set. This is before the activate event is even dispatched. The SW running the activate event is already set as registration.active. So I don't think we can conceivably wait for the activate event's waitUntil() to resolve to get the effect you were expecting.

Also FWIW, the clients.claim() spec seems buggy at the moment. For example, it says Handle Service Worker Client Unload should be run for each client in parallel. This algorithm, however, does things like:

  • If registration's uninstalling flag is set, invoke Clear Registration algorithm passing registration as its argument and abort these steps.
  • If registration's waiting worker is not null, run Activate algorithm, or its equivalent, with registration as the argument.
@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 10, 2015

Member

Oh, the navigation fetch should not complete until activate waitUntil resolves, though. So I think your test case should work.

We explicitly do this waiting in firefox, but I think @matto might have told me chrome does not wait yet.

The live version above does seem to work as you expect in firefox nightly.

Member

wanderview commented Dec 10, 2015

Oh, the navigation fetch should not complete until activate waitUntil resolves, though. So I think your test case should work.

We explicitly do this waiting in firefox, but I think @matto might have told me chrome does not wait yet.

The live version above does seem to work as you expect in firefox nightly.

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 10, 2015

Member

Sorry, I was confused. From a fresh uninstalled state I do see the behavior you describe.

Member

wanderview commented Dec 10, 2015

Sorry, I was confused. From a fresh uninstalled state I do see the behavior you describe.

@jeffposnick

This comment has been minimized.

Show comment
Hide comment
@jeffposnick

jeffposnick Dec 10, 2015

Contributor

After thinking about this more, I realized that what I initially proposed, with the assumptions I was making, would lead to a deadlock.

Let me rephrase this feature request: it would be useful if there were a promise exposed on navigator.serviceWorker that only resolved once the page was controlled by (any) service worker. I had incorrectly assumed that navigator.serviceWorker.ready was such a promise, and I had written a bunch of (flakey!) unit tests based on that assumption, but that's wrong, so I guess I'm asking for a new promise in addition to .ready.

Listening for controllerchange on navigator.serviceWorker is a workaround, I suppose.

Contributor

jeffposnick commented Dec 10, 2015

After thinking about this more, I realized that what I initially proposed, with the assumptions I was making, would lead to a deadlock.

Let me rephrase this feature request: it would be useful if there were a promise exposed on navigator.serviceWorker that only resolved once the page was controlled by (any) service worker. I had incorrectly assumed that navigator.serviceWorker.ready was such a promise, and I had written a bunch of (flakey!) unit tests based on that assumption, but that's wrong, so I guess I'm asking for a new promise in addition to .ready.

Listening for controllerchange on navigator.serviceWorker is a workaround, I suppose.

@jeffposnick jeffposnick changed the title from Have clients.claim() wait until controllers have changed before resolving to Promise on navigator.serviceWorker that resolves when page is controlled Dec 10, 2015

@jungkees jungkees added this to the Version 2 milestone Dec 11, 2015

@jeffposnick

This comment has been minimized.

Show comment
Hide comment
@jeffposnick

jeffposnick Dec 11, 2015

Contributor

FWIW, lacking formal support for such a promise in the platform, I'm adopting https://github.com/PolymerElements/platinum-sw/blob/fix-flaky-test/test/controlled-promise.js in some unit tests that need to wait until the page is controlled before they execute.

Contributor

jeffposnick commented Dec 11, 2015

FWIW, lacking formal support for such a promise in the platform, I'm adopting https://github.com/PolymerElements/platinum-sw/blob/fix-flaky-test/test/controlled-promise.js in some unit tests that need to wait until the page is controlled before they execute.

@delapuente

This comment has been minimized.

Show comment
Hide comment
@delapuente

delapuente Dec 13, 2015

@wanderview are there technical impediments for instead of resolving .ready when the sw gets to activating, make it resolve when getting to activated?

delapuente commented Dec 13, 2015

@wanderview are there technical impediments for instead of resolving .ready when the sw gets to activating, make it resolve when getting to activated?

@jungkees

This comment has been minimized.

Show comment
Hide comment
@jungkees

jungkees Dec 14, 2015

Collaborator

If .ready were designed to resolve when the client got a controller, we couldn't use it for the initial client loading triggered w/o a registration and the shift + reload case (as that client will live w/o a controller for its lifetime). controllerchange is exactly the event to catch the client's controller (active worker) change.

I think changing the behavior of .ready is not a good idea. Let's discuss whether we really want to add such an API.

/cc @jakearchibald @slightlyoff

Collaborator

jungkees commented Dec 14, 2015

If .ready were designed to resolve when the client got a controller, we couldn't use it for the initial client loading triggered w/o a registration and the shift + reload case (as that client will live w/o a controller for its lifetime). controllerchange is exactly the event to catch the client's controller (active worker) change.

I think changing the behavior of .ready is not a good idea. Let's discuss whether we really want to add such an API.

/cc @jakearchibald @slightlyoff

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Dec 14, 2015

Collaborator

Agree with @jungkees. Reg .ready means you can use APIs that depend on an active worker, such as push & BG sync.

Collaborator

jakearchibald commented Dec 14, 2015

Agree with @jungkees. Reg .ready means you can use APIs that depend on an active worker, such as push & BG sync.

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 14, 2015

Member

@jakearchibald wouldn't it make more sense to resolve .ready on activated, though? I believe we delay functional events fired at the SW until the activate event completes.

Member

wanderview commented Dec 14, 2015

@jakearchibald wouldn't it make more sense to resolve .ready on activated, though? I believe we delay functional events fired at the SW until the activate event completes.

@delapuente

This comment has been minimized.

Show comment
Hide comment
@delapuente

delapuente Dec 15, 2015

@jakearchibald , I agree but you can not use them because there is a small windows between activating and activated where you don't receive functional events (like @jeffposnick discovered). It only can when it is fully activated. So, as @wanderview suggest we should resolve .ready when activated. The other option is to stall functional events until activation completion but this could be harmful if someone is extending the event for a long time.

delapuente commented Dec 15, 2015

@jakearchibald , I agree but you can not use them because there is a small windows between activating and activated where you don't receive functional events (like @jeffposnick discovered). It only can when it is fully activated. So, as @wanderview suggest we should resolve .ready when activated. The other option is to stall functional events until activation completion but this could be harmful if someone is extending the event for a long time.

@jeffposnick

This comment has been minimized.

Show comment
Hide comment
@jeffposnick

jeffposnick Dec 15, 2015

Contributor

.ready resolution could be left as-is, and there could be a new promise, perhaps named .controlled, that resolves when there's a controller for the current page.

I see this as being useful primarily for writing unit tests, when the "progressive enhancement/I don't care if there's a controller or not" aspects of service worker isn't relevant, and you really need to delay execution until there's a controller. If you're writing tests that already use promises (because they're using fetch, for instance) then having to mix in controllerchanged event listeners as well ends up feeling messy.

Contributor

jeffposnick commented Dec 15, 2015

.ready resolution could be left as-is, and there could be a new promise, perhaps named .controlled, that resolves when there's a controller for the current page.

I see this as being useful primarily for writing unit tests, when the "progressive enhancement/I don't care if there's a controller or not" aspects of service worker isn't relevant, and you really need to delay execution until there's a controller. If you're writing tests that already use promises (because they're using fetch, for instance) then having to mix in controllerchanged event listeners as well ends up feeling messy.

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 15, 2015

Member

The other option is to stall functional events until activation completion but this could be harmful if someone is extending the event for a long time.

To repeat, this is in the spec today. And we implement it.

Member

wanderview commented Dec 15, 2015

The other option is to stall functional events until activation completion but this could be harmful if someone is extending the event for a long time.

To repeat, this is in the spec today. And we implement it.

@delapuente

This comment has been minimized.

Show comment
Hide comment
@delapuente

delapuente Dec 17, 2015

Roger but please, consider the other option (resolving .ready when activated, which is more aligned with what we want to communicate) and be aware about the potential problems of stall until activation which could lead to noticeable delays in responses due to perfectly valid onactivate tasks.

@wanderview just a clarification, are all functional events stalled until activation or only fetch events?

delapuente commented Dec 17, 2015

Roger but please, consider the other option (resolving .ready when activated, which is more aligned with what we want to communicate) and be aware about the potential problems of stall until activation which could lead to noticeable delays in responses due to perfectly valid onactivate tasks.

@wanderview just a clarification, are all functional events stalled until activation or only fetch events?

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 17, 2015

Member

@wanderview just a clarification, are all functional events stalled until activation or only fetch events?

All functional events. I recently got step 4 added here:

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#handle-functional-event-algorithm

The fetch event waiting for activate event to complete was previously in the spec.

We delay for all functional events in gecko. I don't know what chrome does at the moment.

Member

wanderview commented Dec 17, 2015

@wanderview just a clarification, are all functional events stalled until activation or only fetch events?

All functional events. I recently got step 4 added here:

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#handle-functional-event-algorithm

The fetch event waiting for activate event to complete was previously in the spec.

We delay for all functional events in gecko. I don't know what chrome does at the moment.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Dec 17, 2015

Collaborator

@delapuente

I agree but you can not use them because there is a small windows between activating and activated where you don't receive functional events

I'm not sure what you mean by "can not use". Functional events are queued. The APIs are fully functional. Can you show me a failure case?

I'm not against making .ready resolve on activation, but we need a better argument.

Collaborator

jakearchibald commented Dec 17, 2015

@delapuente

I agree but you can not use them because there is a small windows between activating and activated where you don't receive functional events

I'm not sure what you mean by "can not use". Functional events are queued. The APIs are fully functional. Can you show me a failure case?

I'm not against making .ready resolve on activation, but we need a better argument.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Dec 17, 2015

Collaborator

@jeffposnick

there could be a new promise, perhaps named .controlled

This could be implemented as:

const p = new Promise(r => {
  if (navigator.serviceWorker.controller) return r();
  navigator.serviceWorker.addEventListener('controllerchange', e => r());
});

I think we need more evidence that this is a common enough pattern to add to the platform.

Collaborator

jakearchibald commented Dec 17, 2015

@jeffposnick

there could be a new promise, perhaps named .controlled

This could be implemented as:

const p = new Promise(r => {
  if (navigator.serviceWorker.controller) return r();
  navigator.serviceWorker.addEventListener('controllerchange', e => r());
});

I think we need more evidence that this is a common enough pattern to add to the platform.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Dec 17, 2015

Collaborator

@delapuente

be aware about the potential problems of stall until activation which could lead to noticeable delays in responses due to perfectly valid onactivate tasks

Can you show me some code that would be delayed in this way?

Collaborator

jakearchibald commented Dec 17, 2015

@delapuente

be aware about the potential problems of stall until activation which could lead to noticeable delays in responses due to perfectly valid onactivate tasks

Can you show me some code that would be delayed in this way?

@jeffposnick

This comment has been minimized.

Show comment
Hide comment
@jeffposnick

jeffposnick Dec 17, 2015

Contributor

The "gotcha"/failure came up in the context of writing a unit test, where the code to execute really had to wait until the page was controlled, or it wouldn't end up testing the right thing. I had originally written the test to wait on .ready, and then got confused when the test ended up being flaky.

I now understand why the test was flaky, and am using a synthetic .controlled promise instead.

So that's the main use case I have in mind, and I think that it's a common enough pattern for anyone writing tests. Whether we ask everyone writing tests that depend on the page being controlled to roll their own controllerchange listener, or whether it gets added natively to the spec is obviously a matter for debate.

Contributor

jeffposnick commented Dec 17, 2015

The "gotcha"/failure came up in the context of writing a unit test, where the code to execute really had to wait until the page was controlled, or it wouldn't end up testing the right thing. I had originally written the test to wait on .ready, and then got confused when the test ended up being flaky.

I now understand why the test was flaky, and am using a synthetic .controlled promise instead.

So that's the main use case I have in mind, and I think that it's a common enough pattern for anyone writing tests. Whether we ask everyone writing tests that depend on the page being controlled to roll their own controllerchange listener, or whether it gets added natively to the spec is obviously a matter for debate.

@delapuente

This comment has been minimized.

Show comment
Hide comment
@delapuente

delapuente Dec 17, 2015

All functional events. I recently got step 4 added here:

Thank you @wanderview , that step is what I was missing!

I'm not sure what you mean by "can not use". Functional events are queued. The APIs are fully functional. Can you show me a failure case?

@jakearchibald , my comment was added before knowing about the fact that functional events are delayed until activation. So, no problem here.

@jeffposnick can you point to the specific test? If the spec says that events are delayed and it's well implemente, I'm not able to imagine the specific scenario where activated is truly needed.

delapuente commented Dec 17, 2015

All functional events. I recently got step 4 added here:

Thank you @wanderview , that step is what I was missing!

I'm not sure what you mean by "can not use". Functional events are queued. The APIs are fully functional. Can you show me a failure case?

@jakearchibald , my comment was added before knowing about the fact that functional events are delayed until activation. So, no problem here.

@jeffposnick can you point to the specific test? If the spec says that events are delayed and it's well implemente, I'm not able to imagine the specific scenario where activated is truly needed.

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 17, 2015

Member

Personally I think some of the issues here stem from the name "ready" and sticking it on the ServiceWorkerContainer.

The current .ready promise is really when is the oldest ServiceWorker ready to begin receiving (maybe queueing) functional events.

It seems reasonable, though, for a page to think navigator.serviceWorker.ready is more about "when is my page's controlling service worker ready". Being on the ServiceWorkerContainer implies some relationship with the current page.

Member

wanderview commented Dec 17, 2015

Personally I think some of the issues here stem from the name "ready" and sticking it on the ServiceWorkerContainer.

The current .ready promise is really when is the oldest ServiceWorker ready to begin receiving (maybe queueing) functional events.

It seems reasonable, though, for a page to think navigator.serviceWorker.ready is more about "when is my page's controlling service worker ready". Being on the ServiceWorkerContainer implies some relationship with the current page.

@jeffposnick

This comment has been minimized.

Show comment
Hide comment
@jeffposnick

jeffposnick Dec 17, 2015

Contributor

@delapuente: The test I'm referring to that was flaky when waiting on .ready: https://github.com/PolymerElements/platinum-sw/blob/4cfa76aae4aa9926ace3041fecfafb25c4da2265/test/platinum-sw-fetch/index.html#L37

It's no longer flaky since switching over to explicitly wait on the page being controlled: https://github.com/PolymerElements/platinum-sw/blob/fb5a32dd77a7ee9e4240a05e0d00a570b176fb25/test/platinum-sw-fetch/index.html#L38

It's testing behavior that relies on the service worker's fetch handler being triggered, and that will only happen if the page is controlled when it makes the network request. To reiterate my confusion, I thought that .ready's resolution implied that the page was controlled, but it doesn't actually imply that.

Contributor

jeffposnick commented Dec 17, 2015

@delapuente: The test I'm referring to that was flaky when waiting on .ready: https://github.com/PolymerElements/platinum-sw/blob/4cfa76aae4aa9926ace3041fecfafb25c4da2265/test/platinum-sw-fetch/index.html#L37

It's no longer flaky since switching over to explicitly wait on the page being controlled: https://github.com/PolymerElements/platinum-sw/blob/fb5a32dd77a7ee9e4240a05e0d00a570b176fb25/test/platinum-sw-fetch/index.html#L38

It's testing behavior that relies on the service worker's fetch handler being triggered, and that will only happen if the page is controlled when it makes the network request. To reiterate my confusion, I thought that .ready's resolution implied that the page was controlled, but it doesn't actually imply that.

@delapuente

This comment has been minimized.

Show comment
Hide comment
@delapuente

delapuente Dec 20, 2015

@delapuente: The test I'm referring to that was flaky when waiting on .ready: https://github.com/PolymerElements/platinum-sw/blob/4cfa76aae4aa9926ace3041fecfafb25c4da2265/test/platinum-sw-fetch/index.html#L37

@jeffposnick , was the problem that your test was timing out? How long was the time out? Because, per step 4 of https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#handle-functional-event-algorithm that test should pass as your fetch should be delayed until the SW is activated.

So perhaps your test was revealing a bug in Chrome.

delapuente commented Dec 20, 2015

@delapuente: The test I'm referring to that was flaky when waiting on .ready: https://github.com/PolymerElements/platinum-sw/blob/4cfa76aae4aa9926ace3041fecfafb25c4da2265/test/platinum-sw-fetch/index.html#L37

@jeffposnick , was the problem that your test was timing out? How long was the time out? Because, per step 4 of https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#handle-functional-event-algorithm that test should pass as your fetch should be delayed until the SW is activated.

So perhaps your test was revealing a bug in Chrome.

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 20, 2015

Member

@delapuente That page self registers, so the initial page is not controlled. If the test fetch happens before the clients.claim() finishes then there is no fetch event to be delayed. It really does need to wait until being controlled before starting the test.

Member

wanderview commented Dec 20, 2015

@delapuente That page self registers, so the initial page is not controlled. If the test fetch happens before the clients.claim() finishes then there is no fetch event to be delayed. It really does need to wait until being controlled before starting the test.

@delapuente

This comment has been minimized.

Show comment
Hide comment
@delapuente

delapuente Dec 21, 2015

So that fetch is not controlled because it's dispatched between steps 10 and 13 of https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#activation-algorithm so, after ready is resolved (step 10) but before claim finishes (step 13). Isn't it, @wanderview ?

delapuente commented Dec 21, 2015

So that fetch is not controlled because it's dispatched between steps 10 and 13 of https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#activation-algorithm so, after ready is resolved (step 10) but before claim finishes (step 13). Isn't it, @wanderview ?

@jungkees

This comment has been minimized.

Show comment
Hide comment
@jungkees

jungkees Dec 21, 2015

Collaborator

That page self registers, so the initial page is not controlled.

The page will have its controller only after clients.claim() step 3.4.2 is executed, which is after the Activate algorithm completed. If the test fetch has already happened before that, it falls back to the network in Handle Fetch algorithm before dispatching the fetch event.

Collaborator

jungkees commented Dec 21, 2015

That page self registers, so the initial page is not controlled.

The page will have its controller only after clients.claim() step 3.4.2 is executed, which is after the Activate algorithm completed. If the test fetch has already happened before that, it falls back to the network in Handle Fetch algorithm before dispatching the fetch event.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Apr 10, 2016

Collaborator

Given how easy it is to do this already (#799 (comment)), I don't think we need a specific API for it.

Collaborator

jakearchibald commented Apr 10, 2016

Given how easy it is to do this already (#799 (comment)), I don't think we need a specific API for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment