| 08:41 | <Noam Rosenthal> | Jake Archibald: good morning! I have a couple of SW patches waiting for review when you get a chance... https://github.com/w3c/ServiceWorker/pull/1620 and https://github.com/w3c/ServiceWorker/pull/1621 |
| 09:09 | <Noam Rosenthal> | Hi annevk: We found some existing issue with `process response donein this comment: https://github.com/whatwg/fetch/pull/1311/files#r737407947 |
| 09:10 | <Noam Rosenthal> | basically we call "done" when the network is done, but we never call it for http-cache/SW. For http-cache it's simple, but not sure yet how to integrate it with SW |
| 09:15 | <Noam Rosenthal> | annevk: Maybe network read complete flag should be on a response instead of on the fetch, I think that would solve it |
| 09:16 | <annevk> | How would it get set is the problem, no? It's interesting how the lack of progress events for streams is making a lot of this hard |
| 09:16 | <Noam Rosenthal> | yea |
| 09:16 | <Noam Rosenthal> | ideally this should all be related to the streams |
| 09:16 | <annevk> | Perhaps the solution would be to have some kind of transform stream that reports when EOF is pushed and otherwise forwards |
| 09:17 | <Noam Rosenthal> | isn't that equivalent to "teeing" the stream and fully reading one side of the tee? |
| 09:18 | <annevk> | I thought reading had side effects, such as possibly influencing the speed of transfer |
| 09:18 | <Noam Rosenthal> | yea but not observable side effects |
| 09:19 | <Noam Rosenthal> | as far as observable behavior is concerned it should be equivalent, of course implementations can do something "faster" |
| 09:20 | <annevk> | Maybe, I'm not too well-versed in this. It seems rather wasteful, but perhaps with an explanation of what it is doing it could work |
| 09:20 | <Noam Rosenthal> | but maybe a cleaner solution would be to add some "control-flow" reader to a stream like you suggested |
| 09:21 | <annevk> | Mattias Buelens do you have thoughts on any of this? |
| 09:28 | <Noam Rosenthal> | Seems like maybe the cleanest thing would be that FETCH adds a transform stream that implements flush (https://streams.spec.whatwg.org/#dom-transformer-flush) but that is otherwise an identity transform stream |
| 09:55 | <Noam Rosenthal> | annevk: doing ^^^ for only the SW case would solve the problem I think (for HTTP cache we can check cache-state on fetch finale). I'll jot down a revision and see what it looks like |
| 09:57 | <Mattias Buelens> | Teeing and then fully reading one side is not correct, since it doesn't respect backpressure. Piping through an identity transform stream that implements flush() would work. |
| 10:00 | <Mattias Buelens> | You could also use the promise returned by pipeTo() to know when the pipe has completed, and thus when the readable has reached EOF. But a transform stream with flush() is probably easier to reason about. |
| 10:02 | <Noam Rosenthal> | great, thanks Mattias Buelens. Getting to it! |
| 10:26 | <Noam Rosenthal> | Mattias Buelens: how do I define a [=transform stream=] with a custom flush, spec-wise? Seems like {{Transformer}} is a web-exposed class, while I want to stay in the "internal" realm |
| 10:27 | <Mattias Buelens> | See https://streams.spec.whatwg.org/#other-specs-ts |
| 10:28 | <Mattias Buelens> | For an example, see step 5.2 of https://fetch.spec.whatwg.org/#http-fetch |
| 10:28 | <Noam Rosenthal> | perfect, thanks! |
| 10:30 | <Noam Rosenthal> | See https://streams.spec.whatwg.org/#other-specs-ts transformAlgorithm is required though, is there something like an identityTransformAlgorithm I can use here or needs to be exposed? |
| 10:30 | <Noam Rosenthal> | (I only want to customize flush) |
| 10:34 | <Mattias Buelens> | Hmm, doesn't seem like it. It's probably easiest to define your own transformAlgorithm that simply enqueues the given chunk (using https://streams.spec.whatwg.org/#transformstream-enqueue). |
| 10:34 | <Mattias Buelens> | You can copy the phrasing from https://streams.spec.whatwg.org/#create-an-identity-transformstream |
| 10:54 | <Noam Rosenthal> | Done. annevk I think this made https://github.com/whatwg/fetch/pull/1311 a lot better... At fetch finale the response is either done and can be finalized, or we add a TransformStream at that point that reports done on flush. |
| 10:55 | <Noam Rosenthal> | ... no need to deal with the network thread race conditions |
| 13:15 | <Jake Archibald> | Jake Archibald: good morning! I have a couple of SW patches waiting for review when you get a chance... https://github.com/w3c/ServiceWorker/pull/1620 and https://github.com/w3c/ServiceWorker/pull/1621 |
| 13:22 | <Andreu Botella (he/they)> | Noam Rosenthal, annevk, Domenic: https://github.com/whatwg/html/issues/7376 |
| 13:27 | <Noam Rosenthal> | Noam Rosenthal, annevk, Domenic: https://github.com/whatwg/html/issues/7376 |
| 13:28 | <Andreu Botella (he/they)> | I haven't looked at requestIdleCallback in any depth, but this is an issue with the timers |
| 13:28 | <Noam Rosenthal> | Andreu Botella (he/they): from the issue it seems like it's only a rIB issue, as that's the only place this deadline is relevant. so I guess I don't fully understand the issue. |
| 13:29 | <Andreu Botella (he/they)> | setTimeout() timers keeping an entry in the map of active timers even long after their callback has been run |
| 13:29 | <Andreu Botella (he/they)> | thus keeping the deadline in the past |
| 13:30 | <Andreu Botella (he/they)> | even if it's clamped to 0, that's probably not what you want for already-expired timers |
| 13:30 | <Noam Rosenthal> | right, we now remove them from the list. Why were they kept in the list in the first place and how was that observable? |
| 13:32 | <Andreu Botella (he/they)> | The timer task (timer initialization steps, step 9) in the spec will not remove the entry from the map of active timers if repeat is false |
| 13:33 | <Andreu Botella (he/they)> | But see the note in step 13 |
| 13:33 | <Andreu Botella (he/they)> | I suspect Chrome does remove it |
| 13:36 | <Noam Rosenthal> | Andreu Botella (he/they): I see, so for the purpose of repeating intervals, the spec is "lazy" and doesn't actually remove the timers from the map, which would have a strange effect on requestIdleCallback. The safest solution would be that the map value would have a boolean "expired" alongside the timer's deadline to let rIB ignore them |
| 13:37 | <Noam Rosenthal> | Am I getting this correctly now? |
| 13:38 | <Andreu Botella (he/they)> | Or simply remove the entry from the map after the callback |
| 13:38 | <Noam Rosenthal> | Andreu Botella (he/they): we can't do that because of intervals... repeating means that someone actually canceled that ID |
| 13:39 | <Noam Rosenthal> | but yea removing them and making sure intervals are handled correctly by rIB would also solve this problem |
| 13:41 | <Andreu Botella (he/they)> | I noticed this –and other timer issues– while rewriting the implementation of timers in Deno (even though we don't implement requestIdleCallback yet), and what I'm doing is just have step 9.5 run the timer initialization steps if repeat is true or remove the entry otherwise. |
| 13:41 | <Andreu Botella (he/they)> | Would this be an issue for rIC? |
| 13:43 | <Noam Rosenthal> | this would be good for rIC, all that rIC cares about is the "next active timeout to pop" |
| 13:43 | <Andreu Botella (he/they)> | 👍️ |