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

When response comes from a worker, some of the timing-info should pass through #1208

Closed
noamr opened this issue Apr 1, 2021 · 4 comments · Fixed by #1413
Closed

When response comes from a worker, some of the timing-info should pass through #1208

noamr opened this issue Apr 1, 2021 · 4 comments · Fixed by #1413

Comments

@noamr
Copy link
Contributor

noamr commented Apr 1, 2021

Right now when the response comes from a worker, it comes with timing info, however FETCH later overrides the timing info with the timing info of the fetch, which would lose some information. Some of the info such as the connection info should come from the pass-through response, while other information such as the start time should come from the original request.

@annevk
Copy link
Member

annevk commented Apr 13, 2021

This looks reasonable to me. One tricky aspect might be the time coarsening. If the SW has the capability but we don't, we need to coarsen the SW times here. Otherwise they can be kept as-is.

(That would be in line with what I suggested at w3c/ServiceWorker#1575 (comment) so if that changes we'd need to adjust for that here too.)

@noamr
Copy link
Contributor Author

noamr commented Oct 11, 2021

OK the problem here is that in the case of service workers, there could be two fetches with one response. One fetch is the original fetch from the document, and the other one is the potential fetch() call inside the service worker, which forwards its response to the original fetch. A resource timing entry corresponds to a fetch, rather to a response, so associating it with a response could lead to some confusion.

The timing info of the internal fetch is available in the performance timeline of the service worker itself, so that information is not lost (though maybe it's a bit cumbersome to connect the dots between the two entries).

It should probably be clearer in the spec, that when a response comes from a service worker, it's something like a clone of the response, or some similar construct, and that attaching the timing info to the response is only applicable for this instance of fetch, i.e., attaching fetch timing info to the response in the document fetch does not alter the fetch timing info accessible from within the service worker.

Perhaps also a test to that effect could clarify things.

@noamr
Copy link
Contributor Author

noamr commented Oct 11, 2021

/cc @yoavweiss

noamr added a commit to noamr/fetch that referenced this issue Oct 12, 2021
Use the controller for external timing reporting and aborts.

This allows removing the 1:1 dependency between a Resource Timing
entry and a response, as a response can be shared across fetch
instances (e.g. when restoring a response from cache, or when
forwarding a response through a service worker).

See whatwg#1208
@noamr
Copy link
Contributor Author

noamr commented Nov 8, 2021

As decided in the WG meeting, the info from the SW would remain opaque, but accessible from inside the service worker's performance timeline. The RT entry would stand for a particular fetch rather than a response.
This work is the first step to make this explicit.

annevk pushed a commit that referenced this issue Jun 17, 2022
As long as fetch callers pass in the necessary data through the request concept, they will not have to make additional calls to get timing reported accurately. Note that this does not work if callers want to use useParallelQueue.

Downstream PRs:
* whatwg/html#7722
* whatwg/xhr#347
* w3c/csswg-drafts#7355
* w3c/beacon#75
* w3c/resource-timing#321
* https://github.com/w3c/navigation-timing/pull/1760

Closes #1208 and closes w3c/navigation-timing#131.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants