Deferred fetching #1647
Deferred fetching #1647
Conversation
There was a problem hiding this comment.
- "inactive timeout" -> "deferred delay". At least that seems clearer to me.
- I think
fetchLaterwas quite nice in that it sorts the same asfetch.fetchDeferredcould also work, though is a bit harder to spell. I don't think we need request in the name. - It's not clear how the name fetch group states get activated. That needs some kind of additional PR against HTML I suppose?
- I think we should describe the API in the same section as the fetch method. Could be called "Fetch methods" then.
- Deferred fetching itself could then precede the "Fetch API" section. Maybe it could even be a subsection of "Fetching" though I don't mind a new top-level section.
fetch.bs
Outdated
| <pre class=idl> | ||
|
|
||
| dictionary DeferredRequestInit : RequestInit { | ||
| DOMHighResTimeStamp? inactiveTimeout; |
There was a problem hiding this comment.
No need for this to be both nullable and optional.
But this is deferred delay specifically in the case of inactivity. "inactivity deferred delay"?
I was thinking about "what are we doing right now?" which is requesting/scheduling a deferred fetch for later. But
Yes, HTML would activate/deactivate in the BFCache code path. Need to prepare a PR for that but wanted to see that I'm on the right track first.
Will do
OK |
|
cc @mingyc @fergald @yoavweiss @clelland latest API shape proposal for PendingBeacon |
fetch.bs
Outdated
| <pre class=idl> | ||
|
|
||
| dictionary DeferredRequestInit : RequestInit { | ||
| DOMHighResTimeStamp backgroundTimeout; |
There was a problem hiding this comment.
Per reason discussions with our OT users, it looks like there is also some need for pageHideTimeout in addition to backgroundTimeout. Should we consider replace this with a general timeout which kicks off after calling fetchLater(), and let user freely decide whichever event to with it with?
There was a problem hiding this comment.
what's the difference? Just a general visibility change, like minimizing the window? They can implement it themselves because the JS in the page is still alive
There was a problem hiding this comment.
This is tricky because the page may be frozen after visibilitychange and no JS timers will fire I think there is a footgun here. Browsers may suspend JS timers after visibilitychange. Explicitly, this happens on the freeze event but I suspect it might happen at other times too (freeze is experimental and not available in WebKit or Mozilla, I think).
So that makes it maybe impossible reliably implement send-after-hidden (if some browsers stop timers). Even in those that send a freeze event before stopping timers, it's complex to get correct, you need to
- set a timer
- listen to freeze
- if either happens, send the beacon immediately and prevent the other event handler from doing so. E.g.
So we may need to give users a way to set a timeout after hidden. Maybe a way to say "timeout after JS timers become unreliable" covers this and BFCache?
There was a problem hiding this comment.
Yea I think this would be the same background timeout (we can find a different name for it).
We can say in the HTML spec, that if the page frozen due to visibility change, start the inactive timer for the fetch group (this doesn't affect this PR!)
There was a problem hiding this comment.
@noamr @fergald A followup question to the definition of our fetchGroup's deactivated state:
In addition to the freeze Page LifeCycle event, should it also include pause event or not?
Per https://html.spec.whatwg.org/C/#pause, no loads are allowed to start/continue in this state, and all background processing is also paused: ... require the user agent to pause while running a task until a condition goal is met
I would assume the answer is no, but would like to confirm.
There was a problem hiding this comment.
This is unfortunate but it seems like we should not send while paused or we risk breaking code like
if (!fetchLaterResult.activated) {
...
abortController.abort();
...
}
So there's not much point in starting a timer on pause. I guess it doesn't matter a whole lot either way. If a page cares about the time the beacon was sent, it will have its own timer running in JS. Either our timer sends it or their timer sends it.
There was a problem hiding this comment.
Pausing is not related to deactivation. The main thread is paused when doing things like fetching module scripts or alert(). As far as we're concerned the page is active during pause.
fetch.bs
Outdated
| @@ -2690,16 +2693,104 @@ functionality. | |||
| <dfn export for="fetch record" id=concept-fetch-record-fetch>controller</dfn> (a | |||
| <a for=/>fetch controller</a> or null). | |||
|
|
|||
| <p>A <dfn export>deferred fetch record</dfn> is a <a for=/>struct</a> used to maintain state needed | |||
| to invoke a fetch at a later time, e.g., when a <code>Document</code> is unloaded or backgrounded. It | |||
There was a problem hiding this comment.
Is this equivalent to being bfcached? (Sorry not sure if this is the standard term of it)
There was a problem hiding this comment.
Yea, it's when it's BFCached
There was a problem hiding this comment.
Maybe refer to BFCache's spec and replace backgrounded with the term non-fully active?
After a user navigates away from a document, the document might be cached in a non-fully active state
also
NOTE: It is possible for a document to become non-fully active for other reasons not related to BFcaching ...
Another note about "origin" of a beacon request: there were some previous discussion about using 3P storage partitioning key (not origin, which is stricter) to decide whether pending beacon requests in a page are sendable or not in terms of privacy concern, see WICG/pending-beacon#30 (comment) and comments there below. I am not sure how this should be spec. |
OK, perhaps the 64kb constraint can be per network partition key rather than origin. |
fetch.bs
Outdated
| }; | ||
|
|
||
| partial interface mixin WindowOrWorkerGlobalScope { | ||
| [NewObject] Promise<Response> fetchLater(RequestInfo input, optional DeferredRequestInit init = {}); |
There was a problem hiding this comment.
I think the Promise model has an unavoidable race caused by getting JS involved in delivering this state change information.
Here's the most direct way I can think of to trigger the issue
let completed = false;
fetchLater(...).then(() => {completed = true});
addEventListener("pagehide", () => {
// This task will run first-thing after `pageshow`
setTimeout(() => {
if (!completed) {
doSomething();
}
}, 0);
});Now let's say the fetch occurs while in BFCache. The first task to run on restore from BFCache will see complete as still false and will call doSomething incorrectly.
Even if we somehow make the tasks that run the promise resolves special so that they run first, you can recreate the same problem using 2 fetchLaters. One of the resolve callbacks must run first and would see the wrong value for the completed of the other fetch.
It might also be that any pageshow event handler will run before these callbacks. I can't find a clear answer to that. Document reactivation is here but I'm not sure where the task queue is unfrozen.
This race was avoided in the original design because isPending() had access to the true state. It was all handled in native code which is allowed to execute while in BFCache - the timer expires, the renderer tells the browser to send and updates the pending state.
I think the only way to avoid this issue is to have an object that gives access to the state and update that object outside of JS.
There was a problem hiding this comment.
The task that resolves the promise (call fetch group's activate) should be called when reactivating the document, before suspended timers are resumed and before pageshow. Both of these are done in specified location so there shouldn't be a race. The race here is perhaps created because of the Userland JS implemetation of pending beacon, it can be solved in user-land in the same way that this kind of race can be avoided with regular fetches.
There was a problem hiding this comment.
Saying that we will run these tasks before everything else is concerning. Up until now, no JS runs between pagehide and visibilitychanged (surprisiglyvisibilitychanged fires before pagehide and before pageshow). Giving these promise resolutions special priority doesn't seem right. What if another feature also requires special priority to run before veverything else?
It also doesn't solve the problem when there are 2+ of them. I would hope that people don't put complex code into these resolve callbacks such that it might look at another pending fetch but any amount of work, including updating logged data, can be hidden behind some framework's function call.
There was a problem hiding this comment.
Hmm I see your point. @annevk how would you feel about having the signature be DeferredFetchResult fetchLater(...), where DeferredFetchResult has a { sent: false } that turns to true synchronously at the appropriate moment? (BFCache restore after send / re-show after timers frozen)
There was a problem hiding this comment.
@annevk my current thinking about it is that when the timeout passes, we post a task to invoke the fetch to the document's event loop, without associating the task with the document. When that task is handled, we only flip the boolean and perform fetch if the document is not fully active. This ensures that the fetch/sent-state nevev races with document activation.
There was a problem hiding this comment.
I don't think we have precedent for tasks that are not associated with a document.
There was a problem hiding this comment.
Hmm you're right, it would also be racy if the reactivate task is already queued. So we need an atomic exchange thingy, something like:
Let sentState be "deferred", "scheduled", or "sent", initially "deferred".
When deactivating (in document event loop, after pagehide):
- Set
sentStateto "scheduled". - In parallel:
- Wait
backgroundTimeoutmillis - Let
currentStatebe the result of atomically exchangingsentStatewith "sent" - If
currentStateisscheduled, then fetch etc.
- Wait
When reactivating (in document event loop, before pageshow):
- Let
currentStatebe the result of atomically exchangingsentStatewith "deferred" - Let fetchLater's
sentbe true ifcurrentStateis "sent", otherwise false.
There was a problem hiding this comment.
I think we should match optional arguments where the boolean is always supposed to default to false.
I agree with this as a principle however I think we're talking about a field in a returned struct, not an optional argument. It's an initial value not a default value. So we don't need to favour false.
fetch.bs
Outdated
| <a for=body>length</a>. | ||
|
|
||
| <li><p>If <var>totalScheduledDeferredBytesForOrigin</var> is greater than 64 kilobytes, then | ||
| throw a {{QuotaExceededError}}. |
There was a problem hiding this comment.
If the discussion at #1647 (comment) is resolved and the API shape is changed to return a DeferredFetchResult instead of Promise, should we consider to return null here in addition to throw an error?
There was a problem hiding this comment.
There's no return value when you throw.
There was a problem hiding this comment.
I understand that the body length case needs to fail but I thought we were going to flush the existing entries in this case. Why are we throwing in that case?
There was a problem hiding this comment.
We've discussed this. The body-length case has a per-target-origin quota, It's up to userland to flush if it's reached, same as with keepalive.
There was a problem hiding this comment.
We have a issue (WICG/pending-beacon#76) tracking this topic, and I don't remember about the decision to hand flushing off to userland implementation.
Also, I thought the quota of keepalive is specified by spec, and implemented by UA (not userland JS code)?
All requests with this flag set share the same in-flight quota restrictions that is enforced within the Fetch API.
There was a problem hiding this comment.
@mingyc anything else here or can we resolve this?
There was a problem hiding this comment.
Thanks for explaining. I added some more questions below
in request’s client ’s fetch group ’s deferred fetch records : ...
if deferredRecord’s request ’s body is not null and deferredRecord’s request ’s URL ’s origin is same origin with request’s URL ’s origin ...
IftotalScheduledDeferredBytesForOriginis greater than 64 kilobytes, then throw a QuotaExceededError .
It means totalScheduledDeferredBytesForOrigin only accumulates request lenth from same-origin deferred fetch within the same fetch group (e.g. same Document).
- You mentioned that the quota is
per-target-origin quota, how will this work with records from different fetch group? each origin manages its own and autoflushesis not specified in above. As Fergal mentioned, it would be difficult for userland code to track request size and flush by themselves. Could we specify auto flushes in FIFO order?
There was a problem hiding this comment.
- per-target-origin-in-document (a fetch group is a document)
- Autoflushing would require rewriting this whole thing as suddenly you could have
committedin a live document. I don't see why tit would be difficult to do this in userland. You can catchQuotaExceededErrorfromfetchLaterand perform a regular fetch.
There was a problem hiding this comment.
The discussions about Promise/autoflushes are resolved.
However, some questions on quota are not yet:
Regarding keepalive, I think this should be a bit different (also chatted about this with @yoavweiss):
Keepalive and fetchLater are mutually exclusive and so are there quotas.Keepalive quota is global, fetchLater quota is per origin
Keepalive is for requests that dispatch immediately
Keepalive survives termination, fetchLater (usually) invokes at terminationSo I think the solution would be that deferred fetches are not keepalive, and instead, they're simply not terminated when the fetch group is terminated,
@noamr We now have fetchLater-specific quota steps, but fetchLater requests are also set keepalive true. Should fetchLater requests share the quota of non-FetchLater keepalive requests, i.e. should the former also be run through the same inflight keepalive checks?
There was a problem hiding this comment.
We've talked about this before in this long issue discussion :)
- These request are implicitly keepalive, because they're never registered as fetch records. we don't actually set their keepalive to true in other ways.
- There is no point in sharing the regular keepalive quota. It would mean that origins would fight for that quota, and it is also unnecessary because the UA can simply delay the deferred fetches until all the keepalive fetches are complete. In addition the caller wouldn't receive an error for this which would make
fetchLaterunreliable.
fetch.bs
Outdated
|
|
||
| <li><p><a for=list>For each</a> <a for="fetch group">deferred fetch record</a> | ||
| <var>deferredRecord</var> in <var>fetchGroup</var>'s | ||
| <a for="fetch group">deferred fetch records</a>: If the result of atomically exchanging the value |
There was a problem hiding this comment.
If the result of atomically exchanging the value of deferredRecord's invoke state with
"terminated" is not "sent"
Could you please help me understand what does atomically exchanging the value of deferredRecord' invoke state with terminated, does it mean the result of calling request a deferred fetch or simply setting deferredRecord.invokeState to something? What is the subject the deferredRecord is exchanging invoke state with?
There was a problem hiding this comment.
It means "atomically exchanging", as in std::exchange - you set a new value and return the previous one in a way that guarantees that nothing can happen in between. I added a comment to the dfn if invokeState to clarify.
There was a problem hiding this comment.
So does that just mean atomically updating invokeState to terminated?
There was a problem hiding this comment.
... atomically updating invokeState to terminated and returning the previous value of invokeState.
There was a problem hiding this comment.
Instead of saying "atomically exchanging" we should just have a list of steps. Within the same thread everything always happens in order. (Or if we really need this primitive we should add it to Infra, but I don't think we do.)
There was a problem hiding this comment.
This is bit tricky. This is fundamentally multi-threaded in that there is the document's thread but we sometimes want the request to be triggered from outside of that (and in some cases while the document's event loop is paused).
Is it possible to spec it so that the document is authoritative for the state of the request and that any other thread can post a request to the document's thread to try update it? This is Chrome's implementation and it's unclear that we want to constrain the implementation to match. It's also unclear to me how the document's thread can handle such a request if its event loop is paused. I guess it could be a different event loop that does not get paused. Maybe we can start off being more proscriptive and if someone ever wants to make an implementation that works differently but achieves the same results, we could reword it to be more flexible.
There was a problem hiding this comment.
Instead of saying "atomically exchanging" we should just have a list of steps. Within the same thread everything always happens in order. (Or if we really need this primitive we should add it to Infra, but I don't think we do.)
Yea I don't get how this can be done without either atomically exchanging or running a task on a paused document's event queue which was in a previous revision of this PR. I don't mind going in either direction (adding atomic exchanges to infra or adding some kind of way to run tasks on a paused document) but seems like @annevk has another alternative in mind?
fetch.bs
Outdated
| <a for=list>For each</a> <a for=/>deferred fetch record</a> <var>deferredRecord</var> in | ||
| <var>fetchGroup</var>'s <a for="fetch group">deferred fetch records</a>: If the result of atomically | ||
| exchanging the value of <var>deferredRecord</var>'s <a for="deferred fetch record">invoke state</a> | ||
| with "<code>deferred</code>" is "<code>sent</code>", then <a for=list>remove</a> |
There was a problem hiding this comment.
When a fetch group fetchGroup is reactivated: If the result of atomically exchanging the value of deferredRecord's invoke state with "
deferred" is "sent", then remove deferredRecord from fetchGroup's deferred fetch records and set deferredRecord's sent to true.
Assuming reactivated means going out of bfcache (background to foreground): I feel that sent (or committed) deferredRecord should not be remove here. Can't this also be done in deactivated just following after the step that performs fetching the request?
There was a problem hiding this comment.
To add more, I think this API's behavior is send (commit) & forget. If a document may be kept in bfcache for an uncertain amount of time thus may not become reactivated. If a deferredRecord is already sent when deactivated, isn't it better to just drop it?
There was a problem hiding this comment.
Doing it anywhere else would be racy. Apart from invokeState, the list of deferred fetch records should not be accessed in parallel, as in, should only be accessed from the main thread.
If we did this in deactivated, there would be a short window where the fetch record is still in the list even though it's committed, which means it would, for example, affect the deferred fetch quota.
Do you see any problem with doing this here?
There was a problem hiding this comment.
If we did this in deactivated, there would be a short window where the fetch record is still in the list even though it's committed, which means it would, for example, affect the deferred fetch quota.
Could you please help me understand why removing record in deactivated will still affect the quota? Couldn't totalScheduledDeferredBytesForOrigin also be updated in deactivated?
I am fine with the current approach if updating introduces unexpected behaviors. The fact that script cannot be executed in between (bfcached document) makes moving this step into deactivated help nothing in terms of quota.
fetch.bs
Outdated
| <a for=body>length</a>. | ||
|
|
||
| <li><p>If <var>totalScheduledDeferredBytesForOrigin</var> is greater than 64 kilobytes, then | ||
| throw a {{QuotaExceededError}}. |
There was a problem hiding this comment.
Thanks for explaining. I added some more questions below
in request’s client ’s fetch group ’s deferred fetch records : ...
if deferredRecord’s request ’s body is not null and deferredRecord’s request ’s URL ’s origin is same origin with request’s URL ’s origin ...
IftotalScheduledDeferredBytesForOriginis greater than 64 kilobytes, then throw a QuotaExceededError .
It means totalScheduledDeferredBytesForOrigin only accumulates request lenth from same-origin deferred fetch within the same fetch group (e.g. same Document).
- You mentioned that the quota is
per-target-origin quota, how will this work with records from different fetch group? each origin manages its own and autoflushesis not specified in above. As Fergal mentioned, it would be difficult for userland code to track request size and flush by themselves. Could we specify auto flushes in FIFO order?
There was a problem hiding this comment.
Thank you noamr@! I've added some last comments. And also added the missing features from the original proposal belows. Please let us know if they are suitable here.
- It is suggested to allow only secure requests for new API in Do we want to enforce HTTPS request? WICG/pending-beacon#27. Should we enforce HTTPS-only requests on
fetchLater()? - Should this spec mention [Permission Policy]https://www.w3.org/TR/permissions-policy/? In [Fetch-Based API] Permissions Policy WICG/pending-beacon#77, the suggestion is to allow the API by default. But we might want to provide a way to manage 3rd party iframe's usage.
- Consider to support retry mechanism WICG/pending-beacon#40 Should this spec mention retry when
fetchLater()fails to send/commit? - The original PendingBeacon proposal also includes Crash recovery WICG/pending-beacon#34, not sure how it can be incorporated into fetch spec.
Probably a good idea, from the point of view of enabling new features only for secure requests.
I don't think we should integrate with permission policy. But we should allow the user agent to deny a
Perhaps consider adding this later?
I don't think that changes anything in the spec. |
@noamr Following up on the sendable beacon discussion: As mentioned in WICG/pending-beacon#30 (comment), there were discussions around whether a beacon (or deferred request) should be sent when network changes. I tried to summarize them in [this PR](WICG/pending-beacon@feb3cf9, but basically to process a beacon request when BackgroundSync is off, we need to see if another open document (tab/frame/etc) with the same storage partitioning key as the current document's one, to avoid unexpected sending the request after network changes. Do you think the above makes sense to be integrated into Fetch spec? |
fetch.bs
Outdated
|
|
||
| <li><p><a for=list>For each</a> <a for="fetch group">deferred fetch record</a> | ||
| <var>deferredRecord</var> in <var>fetchGroup</var>'s | ||
| <a for="fetch group">deferred fetch records</a>: If the result of atomically exchanging the value |
There was a problem hiding this comment.
Instead of saying "atomically exchanging" we should just have a list of steps. Within the same thread everything always happens in order. (Or if we really need this primitive we should add it to Infra, but I don't think we do.)
- Clarify the conditions under which a deferred fetch should be activated early.
Add a JS-exposed function to request a deferred fetch.
A deferred fetch would be invoked in one of two scenarios:
A few constraints:
The JS function is called
requestDeferredFetchbut that's bikesheddable.See WICG/pending-beacon#70
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff