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

provide a way to execute work after browser has consumed a response #1397

Closed
wanderview opened this issue Mar 28, 2019 · 21 comments
Closed

provide a way to execute work after browser has consumed a response #1397

wanderview opened this issue Mar 28, 2019 · 21 comments

Comments

@wanderview
Copy link
Member

Recently I've been looking at some code that does something like this:

addEventListener('fetch', evt => {
  evt.respondWith(async function() {
    let response = await fancyResponseLoader(evt.request);
    doCompletionWork(response);
    return response;
  }());
});

After it has a Response it does some amount of completion work. This could book keeping, recording metrics, opportunistic caching, etc.

What I have seen in these cases is that the completion work ends up delaying the browser from processing the response.

It might be nice to provide some way for the code to execute something after the browser has processed the response. A couple options:

  • Make respondWith() return a promise for once it has completed its work on the service worker thread.
  • Add a FetchEvent.complete promise for once it has completed its work on the SW thread. This could also support fallback cases where respondWith() is not called at all.

With something like this we would rewrite the example above as:

addEventListener('fetch', evt => {
  evt.respondWith(async function() {
    let response = await fancyResponseLoader(evt.request);
    evt.waitUntil(evt.complete.then(_ => doCompletionWork(response)));
    return response;
  }());
});

The work around for this issue is to try to use a microtask or task to get your completion work to execute after the browser is done. This is pretty much guesswork, though, since I don't think we clearly define how browsers process the response on the SW thread. For example, in chrome we end up internally queuing a microtask after the respondWith promise completes. So if you want to run your completion work after you need two microtasks to get behind the browser. And of course other browsers might be different. Providing an explicit API would avoid this sort of confusion.

@asutherland
Copy link

So the goal is just to avoid just interrupting the single fetch response consumption, rather than wanting something like https://w3c.github.io/requestidlecallback/ to allow multiple fetches' processing to be deferred until the ServiceWorker reaches an idle point?

(Note that although https://www.chromestatus.com/features/5572795866021888 indicates public support from Mozilla for that API, the link dates to Firefox OS times and there's nothing at https://github.com/mozilla/standards-positions/issues right now so I don't know what the current Mozilla/Firefox position might be, although in principle I think we're on-board with APIs to help sites improve time-to-interactive.)

@wanderview
Copy link
Member Author

Right, the completion work may be somewhat timing dependent so we want to avoid using a full task via setTimeout(f,0) or an idle callback. We'd like to still perform the work as close to completion as possible, but after the browser has processed the Response.

@asutherland
Copy link

Okay. I think it's undeniable that sites will want to/already do this and that it's better to spec this than have a cargo-culted double-then(), so sounds good for Firefox/Gecko. Spec-wise it'd be good to have an info-box that explains how this interacts with streams even though it already flows from the specs.

@wanderview
Copy link
Member Author

wanderview commented Mar 29, 2019

Note, the double-then turned out to not even be adequate in my original case. We ended up having to use setTimeout(f,0) which was worse.

@wanderview
Copy link
Member Author

@youennf @jakearchibald Do either of you have opinions on this proposal?

@wanderview
Copy link
Member Author

wanderview commented Apr 2, 2019

Another thought. Maybe something like FetchEvent.responded would be better than FetchEvent.complete. The "complete" word might suggest all waitUntil promises have settled, where really we only are trying to represent the fact that a response-or-fallback has been sent to back to the outer context.

@jakearchibald
Copy link
Contributor

Feels like this is a problem worth solving. I prefer .responded (or whatever we call it) to respondWith returning a promise, as it plays nicer with waitUntil.

Another option is:

evt.afterResponse(async () => {
  await doCompletionWork(response);
});

This would call waitUntil under the hood. My thinking is that using .responded without waitUntil is an antipattern that we might want to prevent.

My feelings aren't particularly strong though, so if no one's excited about this I'm happy with .responded.

@asakusuma
Copy link

I also would prefer a method like evt.afterResponse, rather than a new evt property that is only designed to be passed in to waitUntil.

Using the word "responded" might be confusing if it's also meant to be a fallback for a response.

@wanderview what exactly do you mean here? Is this meant to catch a user code bug where within the fetch event handler, they literally never invoke evt.respondWith?

This could also support fallback cases where respondWith() is not called at all.

@wanderview
Copy link
Member Author

wanderview commented Apr 2, 2019

This could also support fallback cases where respondWith() is not called at all.

@wanderview what exactly do you mean here? Is this meant to catch a user code bug where within the fetch event handler, they literally never invoke evt.respondWith?

If you want to perform some completion work but you don't call respondWith() at all to fallback to network. You want to let the browser get back to the main thread as fast as possible, but then do some work. So something like:

addEventListener('fetch', evt => {
  evt.waitUntil(evt.responded.then(doCompletionWork));
  if (ShouldIgnore(evt.request)) {
    // doCompletionWork() is called after the fallback to network is started
    return;
  }
  evt.respondWith(fancyResponseLoader(evt.request));
});

Personally it feels a bit weird to add a callback-based function like evt.afterResponse() when most of the API in use is promise based. For example, if we do this callback interface you can no longer use it easily with async/await like:

evt.waitUntil(async function() {
  await doAsyncStuff();
  await doMoreAsyncStuff();
  await evt.responded;
  return doFinalStuff();
}());

It also seems like someone could easily make their own afterResponse() wrapper built on top of the promise attribute primitive if they wanted to. I'd rather see the browser expose the primitive than a higher level wrapper that restricts its use if you want to use it for different use cases.

@asakusuma
Copy link

asakusuma commented Apr 2, 2019

I'd rather see the browser expose the primitive than a higher level wrapper that restricts its use if you want to use it for different use cases.

I agree with that. And I would be fine with evt.responded. I just don't see a scenario in which having evt.responded is more useful than afterResponse. In what kind of scenario would you want to call evt.waitUntil rather than evt.afterResponse for a fetch event? In other words, what kind of operation isn't directly related to responding, but you want started before the response is complete?

If we do want to better support the case where responded operations are dependent on waitUntil operations, then I do agree it makes sense to have evt.responded instead of afterResponse.

@youennf
Copy link

youennf commented Apr 3, 2019

The use case makes sense to me too.

respondWith is calling waitUntil under the hood so that waitUntil for fetch events is probably not used much. Also, the tasks done in doCompletionWork are probably often fast. The need to call waitUntil will not be as evident as for other service worker events so it seems best to automate it.
I would be leaning towards afterResponse.

As of the exact details, listing some existing/envisioned use cases might help.
Caching a response being fetched might be one example.

Some related questions:

  • Is it useful for doCompletionWork to access the response given to respondWith?
  • Is the work done by doCompletionWork the same if respondWith is called or not?
  • Should doCompletionWork be called only if respondWith is called?

@wanderview
Copy link
Member Author

wanderview commented Apr 3, 2019

respondWith is calling waitUntil under the hood so that waitUntil for fetch events is probably not used much.

I don't think the statement "waitUntil for fetch events is probably not used much" is necessarily true. The sites I have looked at lately do use FetchEvent.waitUntil() for things that may last beyond the respondWith() resolution.

I agree that waitUntil is a repeated issue that needs educating people, but I don't think we should give up on the extensible web and stop exposing primitives in favor of wrappers because of it. I feel we should expose the attribute promise even if we also expose a wrapper helper.

Would people be ok with both FetchEvent.responded and FetchEvent.afterResponse()?

Also, the tasks done in doCompletionWork are probably often fast.

I think this has been our assumption, but I've seen tracing that says there is a small significant drag from even simple code (at least in chrome). For example, a function that recorded timing measurements in a js object and then issued a fetch() to log the result to the server added a 5ms to 10ms drag on FetchEvents. Looking at the completion work code you would not expect it to really be noticeable, but it was.

Also, this code can be a further drag right after the worker thread starts up and the js jit is cold, etc. That often means the navigation FetchEvent is going to be the slowest which is one we really care about.

Is it useful for doCompletionWork to access the response given to respondWith?

I think we could have the responded promise resolve with the consumed Response. If they want an unconsumed Response the SW script would have to do their own clone() and pass the duplicate into their completion work handler via a closure.

Is the work done by doCompletionWork the same if respondWith is called or not?

I think it would be useful for the promise to resolve whether a promise settles with a promise to respondWith() or respondWith() is not called triggering fallback. Of course whether those are treated the same by the site is up to how they write their script.

Should doCompletionWork be called only if respondWith is called?

I think it should be called for fallback cases as well. If sites don't want that they can avoid registering their completion work in those cases.

Edit: I guess if everyone else wants an afterResponse() callback style function I could live with it. If my concerns turn out to be real we can always expose the underyling promise as well in the future.

@youennf
Copy link

youennf commented Apr 3, 2019

I think it should be called for fallback cases as well. If sites don't want that they can avoid registering their completion work in those cases.

I agree. That said, both afterResponse and responded names seem to convey that this only happens in case the fetch event is responded. Your initial proposed name 'complete' seems somehow more accurate since we are talking of completion of a fetch task and completion handlers.

With regards to responded vs. afterResponse, I am not intimate enough with service worker scripts to have strong feelings either way.

I think we could have the responded promise resolve with the consumed Response. If they want an unconsumed Response the SW script would have to do their own clone() and pass the duplicate into their completion work handler via a closure.

The completion handler might take benefit of knowing whether:

  • respondWith is not called
  • respondWith is called with a response, or a promise resolving to a response
  • respondWith is called but no response is given, thus leading to a network error

@wanderview
Copy link
Member Author

I agree. That said, both afterResponse and responded names seem to convey that this only happens in case the fetch event is responded. Your initial proposed name 'complete' seems somehow more accurate since we are talking of completion of a fetch task and completion handlers.

Other naming ideas:

  • FetchEvent.handled
  • FetchEvent.fulfilled
  • FetchEvent.requestHandled
  • FetchEvent.complete (confusing with outstanding waitUntil, etc?)

respondWith is not called
respondWith is called with a response, or a promise resolving to a response

If we fulfill the promise with the consumed response then they can tell if respondWith() was called or not.

I don't think we can differentiate between a direct Response being passed vs a Promise-to-Response. I believe WebIDL automatically coerces the direct Response to a Promise-to-Response.

respondWith is called but no response is given, thus leading to a network error

If we expose a promise attribute this can be exposed as a promise rejection. If we did a callback function like afterResponse() we would need a separate error callback I think.

@youennf
Copy link

youennf commented Apr 3, 2019

  • FetchEvent.handled

handled has the advantage to relate to the "Handle Fetch" algorithm.
This seems to map well with the idea to do some JS processing at the end of this algorithm.

I don't think we can differentiate between a direct Response being passed vs a Promise-to-Response.

Agreed and I do not see a need to separate this case in smaller cases.
The 3 cases (valid response, error, go to network) seem ok.
Note that the 'error' case would also include the case of not calling respondWith but cancelling the event.

@jakearchibald
Copy link
Contributor

Pre TPAC notes:

  • Everyone seems happy to add this.
  • Just need to agree naming & promise vs callback.

@jakearchibald
Copy link
Contributor

Resolution:

  • fetchEvent.handled sounds good.
  • Resolves with undefined.

@asakusuma
Copy link

@wanderview would using fetchEvent.handled for cache writes also help alleviate the disk contention you talk about here? https://twitter.com/wanderview/status/1177207289691942912

@wanderview
Copy link
Member Author

wanderview commented Sep 27, 2019

It depends on the site, but in the cases I'm thinking of probably not. Its very likely you would need more of a delay to wait for the current page to finish loading. One site I know of uses a 10 second timeout before writing to cache. You could also envisage something that keeps the responses in memory in the SW and then the page postMessages the SW to persist them after page load is complete.

Edit: The real answer is sites need to measure with real data to determine if this is a problem at all and then the best mitigation.

tingshao added a commit to tingshao/ServiceWorker that referenced this issue Jan 10, 2020
tingshao added a commit to tingshao/ServiceWorker that referenced this issue Jan 10, 2020
tingshao added a commit to tingshao/ServiceWorker that referenced this issue Jan 13, 2020
tingshao added a commit to tingshao/ServiceWorker that referenced this issue Jan 15, 2020
tingshao added a commit to tingshao/ServiceWorker that referenced this issue Jan 22, 2020
@makotoshimazu
Copy link

As FetchEvent.handled is implemented in the spec, can we close this issue?

@jakearchibald
Copy link
Contributor

Agreed!

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

6 participants