should update() during top level script evaluation be ignored?

[wanderview]

The wpt tests we imported from blink has this code at the top level of one of its service worker scripts:

// update() during the script evaluation should be ignored.
self.registration.update();

I don't see anything in the spec that does this. It seems like it might be a good idea, though, to avoid potential infinite async update loops. For example, if the install event fails or something I think you could end up with an endless set of failing update jobs running in the background.

[wanderview]

@mattto, can you clarify exactly what chrome does in this case? Does it truly ignore it silently or does it reject with an InvalidStateError?

[mfalken]

Let me take a look.

As a process note, I'm having trouble staying on top of Github mail. Is it possible to assign me to these issues that are blocked on my feedback, so I can use the Github's issue tracker to keep track of my tasks?

[mfalken]

Ah, I see searching issues for mentions:matto is a pretty good way.

[mfalken]

Ah you mentioned the wrong matto. I'm @mattto with three t's.

[wanderview]

Ah, sorry! Fixed in the comment above.

[ehsan]

I think it makes more sense to reject the promise. Ignoring it silently seems wrong to me.

[mfalken]

Looks like we reject with a InvalidStateError.

The spec probably needs to be revised? In update(), Get Newest Worker is not necessarily null, if there's an active or waiting worker. Then in 9.2 Update, the installing version would be terminated.

[jungkees]

I don't see any problem with the current text. If the install event fails, the worker will get redundant. Rejecting the promise (update() step 4) when the newest worker is null should be enough I guess.

[jungkees]

Get Newest Worker is not necessarily null, if there's an active or waiting worker.

That's right. And even in the example in the OP, update() was called within the installing worker's script, the newest worker, the installing worker itself, is always there. But the Schedule Job will aggregate this update() job in favor of the running installation, so there won't be such an endless set of failing updates.

[wanderview]

That's right. And even in the example in the OP, update() was called within the installing worker's script, the newest worker, the installing worker itself, is always there. But the Schedule Job will aggregate this update() job in favor of the running installation, so there won't be such an endless set of failing updates.

You will still end up with continuous updates running once every 1 second (or whatever update delay the particular browser uses). This seems bad to me. I think we should remove the potential for infinite loops in job scheduling where possible.

I think it would be better to silently resolve the update() promise with success if called:

1. During top level sw script evaluation.
2. Called while install event is being processed.

In both cases we can argue an update is already in progress and its the same kind of short-circuit as Register does in its step 5.3.

[jakearchibald]

You will still end up with continuous updates running once every 1 second

I'm not following this.

Assuming the browser has an active worker that contains only self.registration.update();

1. SW spins up for whatever reason
2. Update triggered
3. SW fetched, and it's byte different! self.registration.update(); // v2
4. Installing worker spun up
5. Update triggered
6. SW fetched, byte-identical, ignored
7. Installing worker activates

Sure, you're going to get a fetch for your SW pretty frequently, but this isn't much worse than putting fetch('/whatever') at the top of your SW.

[wanderview]

Assuming the browser has an active worker that contains only self.registration.update();

This is missing the part about failing the install. The script in question is more like:

self.registration.update();
addEventListener('install', function() { throw 'boom'; });

AFAICT this will update continuously until the script is changed. I don't think we should let update() function in a SW that has not reached the installed state.

[jungkees]

But the Schedule Job will aggregate this update() job in favor of the running installation

@wanderview, I wonder if you considered this point?

So, I mean the job created by self.registration.update() will not be pushed to the queue.

[mfalken]

A note on Chrome's implementation: the comment in the Blink's test is misleading:
// update() during the script evaluation should be ignored.

Chrome actually can perform update() during initial script evaluation. It only ignores rejects it if Get Newest Worker returns null. The comment means to say something like "update() from the installing worker of a new registration is rejected with InvalidStateError".

[wanderview]

@jungkees Where does it do the aggregation now? I seem to remember we only aggregate SoftUpdates, not explicit update() calls. Also, I can't find the steps that do the aggregation since the change to a single job queue.

I'm not sure rejecting if newest worker is null is adequate. The install algorithm sets .installing before executing Run Service Worker. So .installing will be set at top level script evaluation.

I think we should probably reject if the only worker is installing with no waiting or active workers.

[jungkees]

@jungkees Where does it do the aggregation now? I seem to remember we only aggregate SoftUpdates, not explicit update() calls. Also, I can't find the steps that do the aggregation since the change to a single job queue.

The single job queue change made all the equivalent jobs (i.e. jobs from register(), update(), Soft Update) be aggregated in the Schedule Job algorithm step 2.2.

[jakearchibald]

F2F resolution: @wanderview to review and see if no longer an issue.

[jungkees]

By ccfa7e4,

The single job queue change made all the equivalent jobs (i.e. jobs from register(), update(), Soft Update) be aggregated in the Schedule Job algorithm step 2.2.

is not true any more. An equivalent job in the front of the queue whose promise has already settled makes the job passed to Schedule Job be queued. So, more thought and discussion are needed for the issue in this tread.

[wanderview]

I think the easiest way to handle this would be to do something like this in ServiceWorkerRegistration.update():

1. If the entry settings object is a ServiceWorkerGlobalScope (or appropriate text to determine its being called within a service worker).
   1. If the ServiceWorkerGlobalScope's associated service worker's state is 'installing', reject the promise with an InvalidStateError and abort these steps.

Would something like that work?

[jungkees]

Agreed on rejecting the update attempt for an installing worker. I think the suggested algorithm steps are good (with a relevant settings object instead of an entry settings object). @matto, WDYT?

[mfalken]

SGTM.

[jungkees]

I put it in the Update algorithm after a thought. I think the consensus is making the update() reject for an installing worker regardless the context object (ServiceWorkerRegistration) is in a client or in a service worker. Let me know if you didn't expect this.

[wanderview]

Sorry, but I don't think the check can be done in the Update algorithm. With it written that way the check won't be executed until after the current job completes and the Update job begins. This means you will never see a worker in the installing state.

I think we have to do this check in the update() function itself. Also, I think it should use the service worker associated with its global since this is really only a problem for a service worker updating itself.

[jungkees]

With it written that way the check won't be executed until after the current job completes and the Update job begins.

Absolutely. I'll get it done again as you originally suggested. Thanks!

[jungkees]

Addressed: afae5de