| 01:40 | <msmith12> | Hi all |
| 01:40 | <msmith12> | Just sumbitted patch, hopefully its the start of something great |
| 06:02 | <Noam Rosenthal> | Good morning annevk, some longish morning thought. I was thinking a lot about the fetch/timing integration. Maybe it was not right to associate the timing info with a response? A response is something that can be shared between fetches (service workers, cache), and would have different timing in each fetch context as some of the timing is measured before the response is created. Perhaps reporting the timing should be a method of the fetch instance rather than the response, and all the "attaching" is not necessary? Contemplating https://github.com/whatwg/fetch/issues/1208 and https://github.com/whatwg/fetch/issues/1215 |
| 06:03 | <sideshowbarker> | Noam Rosenthal: by the way, thanks much for doing that PR for proper Fetch integration into CSS |
| 06:11 | <Noam Rosenthal> | Noam Rosenthal: by the way, thanks much for doing that PR for proper Fetch integration into CSS |
| 06:12 | <sideshowbarker> | yeah it was kind of long overdue |
| 06:13 | <sideshowbarker> | and for any given spec, itโs not trivial to figure how to call into the Fetch algorithms in the right way |
| 06:14 | <sideshowbarker> | *for any given request scenario |
| 06:47 | <Noam Rosenthal> | trying to figure it out for multiple specs... For the CSS one it's a bit of a one-size-fits all because there are no load/error events, but still there are issues with referrers and CORS etc. |
| 07:09 | <annevk> | Noam Rosenthal: I guess conceptually you'd want fetch to return a response and timing info, but since the handshake is fetch(request) -> response it's hard to change that |
| 07:09 | <annevk> | Noam Rosenthal: though maybe it's changeable more easily now that we have callbacks |
| 07:10 | <annevk> | Noam Rosenthal: the other thing we probably still want to do at some point is FetchObserver (there's an issue if you search for this), which would also be a somewhat natural point to expose this on |
| 07:12 | <Noam Rosenthal> | annevk: Fetch doesn't really return anything, but rather takes in callbacks that accept responses. I think that it could return some controller object that represents the fetch instance, which could have things like "report resource timing" |
| 07:13 | <annevk> | Noam Rosenthal: sure, I meant at a high-level, but yeah, at a concrete level I suspect we'd want to return the controller directly |
| 07:14 | <annevk> | Noam Rosenthal: that would also give us a way to do abort more cleanly |
| 07:14 | <annevk> | Noam Rosenthal: that's probably what I'd want to start with |
| 07:14 | <Noam Rosenthal> | it would be easy to hook that return value into the fetch caller for the purposes of resource timing |
| 07:15 | <annevk> | We'd still want to use the timings of the callbacks to expose the information, though |
| 07:15 | <Noam Rosenthal> | I guess the idea of FetchObserver is conceptually that? I'll check the issue |
| 07:15 | <Noam Rosenthal> | yes, the callbacks will be there in the same way, but there will be no "attaching timing info to a response" |
| 07:16 | <annevk> | Noam Rosenthal: yeah, FetchObserver is/was for progress events and HTTP/2 push, but the latter died and there was never enough interest in the former (and it can be polyfilled using streams) |
| 07:16 | <annevk> | Yeah, we'd attach it to the controller |
| 07:16 | <Noam Rosenthal> | I think the controller will have access to the fetch params |
| 07:17 | <Noam Rosenthal> | like a public facade for stuff you can do with things from the fetch params |
| 07:17 | <annevk> | Yeah, I think we'd want a facade of sorts (or asserts) to ensure specs don't do weird things |
| 07:18 | <Noam Rosenthal> | Great, let me jot down something |
| 07:51 | <Noam Rosenthal> | this would also make it possible to update resource timing entries after they're created, e.g. if in the future we would want to spec that a video resource timing entry is created when the response is ready but updated with its responseEnd when it's done |
| 08:19 | <annevk> | Noam Rosenthal: I think in principle that should be possible either way, but this might make it easier |
| 08:19 | <Noam Rosenthal> | yea exactly |
| 08:20 | <annevk> | Noam Rosenthal: now that you mention it though, how do we account for media elements currently? They always make range requests so they get responses quite quickly. The spec for them doesn't quite reflect this (or at least not in sufficient detail) |
| 08:20 | <annevk> | I guess the current setup might end up with an entry per range request potentially? |
| 08:22 | <Noam Rosenthal> | annevk: currently in the spec, that integration is not done yet. I need to research how it's done in implementations. I wanted to have a good infrastructure with the simpler requests first |
| 08:40 | <Noam Rosenthal> | annevk: https://github.com/whatwg/fetch/pull/1329 |
| 08:56 | <Noam Rosenthal> | annevk: btw, I think also cacheState and timing allow passed flag should be associated with a fetch and not with a response, for the same reasons. It's not clear how they're forwarded from service workers or restored from cache |
| 08:58 | <annevk> | By restored from cache you mean restored from the Cache API? That might make sense. There's a general issue with that API not getting as much attention as it deserves. |
| 09:13 | <Noam Rosenthal> | annevk: I meant also from HTTP cache. It's not spec`ed how those responses are stored, and as far as the spec is concerned the response stored in the cache is a reference to the same one that's restored. |
| 09:16 | <annevk> | Noam Rosenthal: well, but we always set this field when we pull something from the cache |
| 09:17 | <Noam Rosenthal> | annevk: right, but we set it on the response, which could mean setting it on the original response also. I think there is a related Chrome bug about exactly this |
| 09:17 | <Noam Rosenthal> | e.g. if you store a response to cache and retrieve it before it's done, both responses would get the same cacheState |
| 09:17 | <annevk> | I think I'm missing how it's observable |
| 09:18 | <annevk> | Aah |
| 09:19 | <annevk> | Presumably we should take a copy when storing it in the cache (or take a copy when forwarding). |
| 09:19 | <annevk> | In theory that also seems problematic for reading the body for instance (which can only happen once). |
| 09:21 | <Noam Rosenthal> | yes, and I added a non-normative note as such |
| 09:21 | <Noam Rosenthal> | Ah actually it was something else, got mixed up |
| 09:22 | <Noam Rosenthal> | It's presumably a copy, but it's not formally a copy, and there is no definition of what a "response copy" is |
| 09:24 | <Noam Rosenthal> | which, I believe, fudges some things in existing implementations when response objects are shared between fetches |
| 09:28 | <annevk> | Hmm, how did https://twitter.com/htmlstandard/status/1447820555760005122 get tweeted? |
| 09:31 | <annevk> | Ah, I forgot to remove a leading space when squashing. Good times. |
| 09:54 | <Noam Rosenthal> | annevk: updated the PR, now timing/TAO/cacheState are all part of fetch params and accessible through a controller. Regarding body I'll open a separate issue, I'm less familiar with it. |
| 10:10 | <annevk> | Noam Rosenthal: note that if you're moving cacheState we'll need to update SW too |
| 10:11 | <annevk> | Noam Rosenthal: doing this in smaller steps would make it a little easier to ensure nothing is missed |
| 10:24 | <Noam Rosenthal> | Noam Rosenthal: doing this in smaller steps would make it a little easier to ensure nothing is missed |
| 11:56 | <Noam Rosenthal> | Actually, this might be more complex... not sure if on some cases RT users rely on resource-timing being per response rather than per-fetch, need to clarify with the working group |
| 12:05 | <annevk> | Noam Rosenthal: you mean when requests are coalesced? There's also a question to what extent we want to expose that |
| 12:07 | <annevk> | TabAtkins: have you considered linting / auto formatting for Bikeshed at some point? That would help a lot with spec PRs potentially. |
| 12:08 | <Noam Rosenthal> | annevk: : I mean, for example, when a service worker forwards a network response. Should the connection info be available only inside the context of the service worker, or should it be part of the resource entry of the document? Same for resources that were fetched early in the document and then accessed from cache. |
| 12:09 | <Noam Rosenthal> | I personally believe the cleanest solution that doesn't lose any info would be that a Resource Timing Entry is mapped 1:1 to a fetch, but I have to see with the WG whether there are other expectations or existing implementations that counter this |
| 12:09 | <annevk> | Noam Rosenthal: I thought we had some forwarding of service worker data already? |
| 12:11 | <Noam Rosenthal> | annevk: the service worker timing info itself is extra information available to navigations. What we forward from the service worker for non-navigation resources is a response. Since that forwarding happens before fetch-finale, that response's timing info gets overwritten by the document's fetch |
| 12:16 | <annevk> | I see, there is some complexity there as some service worker responses might not yet carry timing info, so any change would have to be considered carefully |
| 12:38 | <Noam Rosenthal> | annevk: are "flags" in the fetch spec considered atomic? I didn't see information about this in infra |
| 12:46 | <annevk> | Noam Rosenthal: no, jyasskin has brought this up here and there and Jake Archibald too. Saying it's atomic when using it is probably sufficient for now. |
| 12:46 | <Noam Rosenthal> | Ok, so something like "atomically test and set" this or that. |
| 12:53 | <Noam Rosenthal> | annevk: the integrity case, btw, does not present a race condition. Fetch finale is only called after the integrity is verified |
| 12:54 | <Noam Rosenthal> | I believe there is actually no race condition. The "process response" steps are only called from within fetch finale. The flag setting doesn't need to be atomic |
| 12:57 | <annevk> | Noam Rosenthal: what ends up calling finalize in the integrity case? |
| 12:58 | <Noam Rosenthal> | annevk: the integrity algorithm itself once its done |
| 12:58 | <Noam Rosenthal> | you mean fetch finale |
| 12:58 | <annevk> | No, I meant finalize |
| 12:59 | <Noam Rosenthal> | annevk: right now, processing the body for the sake of integrity will call finalize response when it's done. but that would happen before the fetch finale, not in a race |
| 13:01 | <Noam Rosenthal> | the whole integrity sequence is done before fetch finale, so if the callback is attached in fetch finale, there is no race |
| 13:01 | <annevk> | Noam Rosenthal: if the callback is attached in fetch finale, how could it be invoked before? |
| 13:02 | <Noam Rosenthal> | annevk: it should not be invoked before. The callback is only valid for the actual response handling, not for verifying integrity |
| 13:02 | <annevk> | Noam Rosenthal: agreed, but finalize does need to run |
| 13:03 | <Noam Rosenthal> | annevk: it would run as the response body is read again inside process response or one of those |
| 13:03 | <annevk> | Noam Rosenthal: I don't think so, why? |
| 13:04 | <Noam Rosenthal> | ah I see what I've missed. the body is set by integrity and not read again |
| 13:04 | <Noam Rosenthal> | it's still not a race, fetch finale should finalize if body has already been read |
| 13:05 | <annevk> | That body read wouldn't trigger finalize either though, because finalize is only called by fetch finale and the network stack |
| 13:06 | <annevk> | And process response done only cares about the network stack being done, so finalize can be invoked by fetch finale if the network stack is already done. (Which I think is what I wrote down in my rather lengthy comment.) |
| 13:06 | <Noam Rosenthal> | ok, took me a while to process. got it now hopefully |
| 13:08 | <Noam Rosenthal> | process response done is something like "the latter between fetch finale and network stack is done, which are racy" |
| 13:08 | <annevk> | Yeah, I want additional review for it as well as I noted. This stuff is not fun ๐ |
| 13:09 | <Noam Rosenthal> | I'm actually having fun |
| 13:39 | <Noam Rosenthal> | annevk: ok my new version maybe handles it better. I have two flags, "network read complete" and "ready for clients", and only when both are set we dispatch "process response done" |
| 13:39 | <Noam Rosenthal> | they're not atomic as it's not really a thread issue. Body is read either before fetch finale (in integrity checking) or after (as a result of process response) |
| 14:41 | <annevk> | Noam Rosenthal: well, there are two "threads" |
| 14:53 | <Noam Rosenthal> | annevk: but the second "thread" only starts after we set the "ready for clients" flag, so it's not in a race |
| 14:56 | <annevk> | Noam Rosenthal: there's the fetch thread and there's the "receiving body bytes from the network" thread (and there's the main thread, which would be a third); I'm pretty sure the first two can race |
| 15:17 | <Noam Rosenthal> | annevk: Ah I see, the network bytes keep running even if there is no "process response done". ok |
| 15:17 | <Noam Rosenthal> | more fun |
| 15:30 | <annevk> | Jake Archibald: sideshowbarker: https://github.com/whatwg/fetch/pull/1330 might be of interest |
| 21:03 | <msmith12> | annevk: i will resubmit patch thanks |