Skip to content
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

event.waitUntil vs return promise #1148

Closed
pemrouz opened this issue May 20, 2017 · 13 comments
Closed

event.waitUntil vs return promise #1148

pemrouz opened this issue May 20, 2017 · 13 comments

Comments

@pemrouz
Copy link

pemrouz commented May 20, 2017

Currently SW event listeners require using event.respondWith and event.waitUntil.

It would be nice if we could return a promise instead of these.

The argument against given here was that because "it lets you keep the SW alive forever". However, you could pass the promise sleep(Infinity) to event.waitUntil too, so there doesn't seem to be anything special about the event.* syntax over using an async function + return value. That is, the browser can kill the SW in both cases if it's taking too long.

@domenic
Copy link
Contributor

domenic commented May 20, 2017

Dupe of #256 and #354.

@pemrouz
Copy link
Author

pemrouz commented May 20, 2017

Sorry, missed those. Seems like discussion went to hooks there. Is there any reason to not keep events + use return value?

@jakearchibald
Copy link
Contributor

It would be nice if we could return a promise instead of these.

I don't understand how a single promise can replace both of these.

@pemrouz
Copy link
Author

pemrouz commented May 21, 2017

In both cases, it waits for the given promise to resolve. For waitUntil, the value the promise resolves to is irrelevant/unused.

@jakearchibald
Copy link
Contributor

But a fetch event has both respondWith and waitUntil. How can one returned promise be used for both?

@jakearchibald
Copy link
Contributor

fwiw the general problem here is event handler return values are already used for preventing default. Making them do something different events seems a bit weird.

Also, if you don't want to handle a fetch, you just avoid calling respondWith. This gets more complicated with return values, as functions always return.

@pemrouz
Copy link
Author

pemrouz commented May 21, 2017

But a fetch event has both respondWith and waitUntil. How can one returned promise be used for both?

return for respondWith. Wait till all the tasks have completed. See:

was thinking cache async, but return response. Can browser not kill SW based on tasks left to complete? e.g. node doesn't kill if setTimeout or other cbs pending, no "waitUntil". cache.put alone should suffice, no kill till completes


fwiw the general problem here is event handler return values are already used for preventing default.

Sure, but it seems like it should be fairly simple to not confuse the two. For example, if the promise return value was spec'ed just for ExtendableEvent, it would just make it work for SW and newer API's whilst leaving the older stuff as is.

Making them do something different events seems a bit weird.

I imagine very few people actually use event return values to prevent default vs .preventDefault() and will be confused by this or complain about it. Or, if you took a poll, I imagine most people would strongly prefer to use async/await throughout vs return values in SW code.

On the other hand, people are pretty used to the event loop running till completion and will find the default behaviour very surprising. There will be lots of confusion and debugging when event handlers silently skip and finish tasks. It will require constantly changing between paradigms.

In fact, now I think about it, event.waitUntil seems like completely redundant information. When would you ever deliberately want to terminate before some other task you've scheduled?

Also, if you don't want to handle a fetch, you just avoid calling respondWith. This gets more complicated with return values, as functions always return.

This could match to not returning a promise, or a promise that resolves to undefined?

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 1, 2017

In fact, now I think about it, event.waitUntil seems like completely redundant information. When would you ever deliberately want to terminate before some other task you've scheduled?

The browser will want to do this if you're using too much background CPU. It can use extendable events to track which events have granted the service worker additional run time, and that run time can be revoked.

Additionally, waitUntil is used to signal eventual success/failure for multiple events.

@pemrouz
Copy link
Author

pemrouz commented Jun 6, 2017

The browser will want to do this if you're using too much background CPU. It can use extendable events to track which events have granted the service worker additional run time, and that run time can be revoked.

The browser can intervene and terminate based on clear thresholds (time, CPU, storage, etc) in both cases, the waitUntil hook doesn't play any role in this. If you passed sleep(Infinity) the browser would intervene. If you returned or asynchronously scheduled sleep(Infinity) the browser could intervene.

Also note that you took a browser-perspective: I mentioned a developer will never not want to waitUntil. If they schedule something but don't also extend the lifetime, that's always a bug. Extra API is useful if it allows developers to do something different. Accordingly, it only has potential downsides (accidentally omitting it), and zero upsides.

Additionally, waitUntil is used to signal eventual success/failure for multiple events.

The suggestion is that you can simply schedule asynchronous tasks that run to completion or if they trip a quota (e.g. CPU usage), and you can use the return value to signal any eventual extra info (success/failure/response/etc). It might be useful to create a gist to demonstrate if there's a certain task/pattern that you think this wouldn't work for?

@jakearchibald
Copy link
Contributor

If you passed sleep(Infinity) the browser would intervene.

Yep, and it could know which events it terminated early, which wouldn't be the case if sleep(Infinity) wasn't passed.

a developer will never not want to waitUntil

It's pretty common to not waitUntil in a fetch event.

It might be useful to create a gist to demonstrate if there's a certain task/pattern that you think this wouldn't work for?

addEventListener('install', event => {
  event.waitUntil(doAsyncStuff());
});

In the above example, if the promise returned by doAsyncStuff() rejects, the service worker does not install.

addEventListener('fetch', event => {
  event.respondWith(
    fetch(whatever).then(response => {
      event.waitUntil(addThisToTheCache(response));
      return response;
    })
  );
});

In the above example, the developer declares they want to handle this request by calling event.respondWith (it isn't handled by the service worker otherwise). The promise passed to respondWith should resolve with a response. waitUntil is used to further extend the event to carry out work that needs to happen beyond the response of the request.

@pemrouz
Copy link
Author

pemrouz commented Aug 11, 2017

(Sorry for the delayed response)

Yep, and it could know which events it terminated early, which wouldn't be the case if sleep(Infinity) wasn't passed.

Are you saying the browser can't know the list of outstanding tasks it terminated early without the waitUntil assist? I imagine the browser should have that knowledge à la _getActiveHandles().

It's pretty common to not waitUntil in a fetch event.

For completeness: "a developer will never not want to waitUntil if they have scheduled something asynchronously". That's always a bug. They might want to respond early, but not skip waiting.

addEventListener('install', event => {
  event.waitUntil(doAsyncStuff());
});
addEventListener('install', event => {
  return doAsyncStuff();
});

Similarly, does not install if promise rejects.

addEventListener('fetch', event => {
  event.respondWith(
    fetch(whatever).then(response => {
      event.waitUntil(addThisToTheCache(response));
      return response;
    })
  );
});
addEventListener('fetch', async ({ respond }) => {
  const response = await fetch(whatever)
  respond(response)
  addThisToTheCache(response)
})

Similarly, the user can respond early and then cache stuff later.

@jakearchibald
Copy link
Contributor

Currently, with fetch, you have to call respondWith during the dispatch of the event to signal that you intend on handling the response. waitUntil is separate, as you may wish to do additional work in the service worker related to the fetch, but not handle the response.

Closing, as this is a dupe of #256 and #354.

@pemrouz pemrouz changed the title event.waitUntil/respondWith vs return promise event.waitUntil vs return promise Dec 7, 2017
@pemrouz
Copy link
Author

pemrouz commented Dec 7, 2017

To be clear, I think separating respondWith is fine and allows different use cases. However, waitUntil still seems strictly unnecessary. I've updated the title accordingly.

Since we don't await or return the result of waitAndMaybeReject(), we don't react to it in any way. Code like this is usually a mistake.

https://jakearchibald.com/2017/await-vs-return-vs-return-await/

You basically need to amend this article with another hazard where await waitAndMaybeReject() is also not sufficient.

waitUntil is separate, as you may wish to do additional work in the service worker related to the fetch, but not handle the response.

This is the case without waitUntil too.

The issue is that waitUntil is an unnecessary footgun. The original motivation of the browser needing to terminate the worker is completely orthogonal. The browser can do that without a waitUntil. It can also do that in the synchronous case (e.g. if a worker does while(1)) where there is no waitUntil today.

To summarise: Keep .respondWith. You also don't need to deprecate .waitUntil. Just correctly .waitUntil the response if a promise is returned. Also, if there are things left to execute, don't ignore them by default. If you need to terminate despite there being things left to execute, that should be handled separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants