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

Enforce payload limits via Fetch API #39

Merged
merged 2 commits into from
Mar 1, 2017
Merged

Conversation

igrigorik
Copy link
Member

Beacon requests are allowed to continue while/if a fetch group is being
terminated - e.g. as the page unloads. To ensure that such requests do
not negatively affect the consequent navigation, or incur high costs for
the user, we limit the amount of data that can be queued via this
method.

The enforcement is applied within Fetch API, which tracks the amount of
in-flight data queued with the keepalive flag set, and returns a
network error if the limit is exceeded.

Background: #38


^ that's the end goal, ideally... as that would lead to consistent behavior between sendBeacon and plain fetch() with keepalive flag set to true. This also means that size enforcement has to live in Fetch API, as otherwise we'd end up with two different byte counters.

The challenge, however, is how to get an early return code for sendBeacon? Unlike fetch, which returns a promise we need to return true/false back to the developer: if we didn't trigger a network error before request is queued, return true; otherwise return false. @annevk any suggestions for best way to layer this?

Beacon requests are allowed to continue while/if a fetch group is being
terminated - e.g. as the page unloads. To ensure that such requests do
not negatively affect the consequent navigation, or incur high costs for
the user, we limit the amount of data that can be queued via this
method.

The enforcement is applied within Fetch API, which tracks the amount of
in-flight data queued with the `keepalive` flag set, and returns a
network error if the limit is exceeded.

Background: #38
@annevk
Copy link
Member

annevk commented Nov 12, 2016

That's the same question as asked during that PR. Can implementations actually synchronously get that size data? E.g., I don't think all can for FormData.

@igrigorik
Copy link
Member Author

@annevk the fact that we have three existing implementations of sendBeacon (FF, Edge, Chrome) suggests that the practical answer is "yes". That said, resolving this fact against Fetch is gnarly since "extract a body" is buried deep within... Effectively what we have is:

  • For sendBeacon we have a processing step (5) that synchronously checks the size, returns false if check fails, and then later calls Fetch.
  • As specced within Fetch we enforce similar check once again, except by rejecting the response promise if it fails within the "extract a body" step.

Ideally, we wouldn't have to do the check in sendBeacon and defer it all to Fetch, but I don't see any easy way to reconcile the two worlds here. I guess I can keep the existing (sync check) logic in Beacon and then replicate similar checks within Fetch. Less than ideal, but I can't think of a better way. WDYT?

@annevk
Copy link
Member

annevk commented Nov 15, 2016

Do all existing implementations support FormData too? Anyway, if that is true I guess everyone has a way to synchronously get the size of these objects already, despite arguments at TPAC to the contrary by the same implementers... So I guess the primitive we should create is getting the size of these objects and reuse that somehow.

@igrigorik
Copy link
Member Author

I believe so. We have a pending PR for web-plat tests (see https://github.com/w3c/web-platform-tests/pull/4024/files#diff-0c312b518bce4321b6708240f0a48519R38) that tests FormData and those seem to pass just fine in all existing implementations.

So I guess the primitive we should create is getting the size of these objects and reuse that somehow.

Should that live in Fetch?

@annevk
Copy link
Member

annevk commented Nov 15, 2016

Yeah, I guess Fetch is fine. Can refactor later if needed. We might also want to expose the size directly on these objects then…

@igrigorik
Copy link
Member Author

Ok, I reverted some of the earlier logic change: we'll run the synchronous size check (as before) and return false if that fails; the call to fetch happens after that and runs in parallel with us returning from sendBeacon. In effect, the only substantive change here is that we changed "user agent may enforce limit" to "user agent will enforce limit", with actual enforcement happening in Fetch.

Speaking of Fetch, I think we need the following:

  • introduce some byte counter variable in Fetch that keeps track of number of in-flight bytes initiated by requests with keepalive set to true; when any such request finished, we decrement the number of in-flight bytes.
  • update the step that checks the size limit (max 64KB of in-flight data for such requests) and return network error if request exceeds said limit.

With above in place, both Beacon initiated and Fetch initiated requests will share the same enforcement logic and have the same behavior.

@annevk @toddreifsteck does that sound reasoanble? I can put together a Fetch PR for this..

@annevk
Copy link
Member

annevk commented Nov 18, 2016

I think we already keep track of transmitted bytes through https://fetch.spec.whatwg.org/#concept-body-transmitted. This sounds reasonable, but what seems to be missing is a definition of how you calculate the size of the objects.

igrigorik added a commit to igrigorik/fetch that referenced this pull request Nov 23, 2016
Requests with keepalive flag set are allowed to outlive the environment
settings object. We want to make sure that such requests do not
negatively impact the user experience when a page is unloaded, etc.

This limits the amount of (body) bytes that can be inflight at any point
when the request has the keepalive flag set; this flag is set by
sendBeacon.

Background: w3c/beacon#39
@andrewwakeling
Copy link

Duplicating my question from #38:

If a create a series of empty payloads and choose to transmit data via the URL, what is the expected outcome, with regard to the quota and the behaviour of the browser?

@igrigorik
Copy link
Member Author

igrigorik added a commit to igrigorik/fetch that referenced this pull request Feb 24, 2017
Requests with keepalive flag set are allowed to outlive the environment
settings object. We want to make sure that such requests do not
negatively impact the user experience when a page is unloaded, etc.

This limits the amount of (body) bytes that can be inflight at any point
when the request has the keepalive flag set; this flag is set by
sendBeacon.

Background: w3c/beacon#39
@igrigorik igrigorik changed the title (WIP) Enforce payload limits via Fetch API Enforce payload limits via Fetch API Feb 24, 2017
@igrigorik
Copy link
Member Author

igrigorik commented Feb 24, 2017

@toddreifsteck Fetch update is ready to land - see whatwg/fetch#419 (comment). Please review the update here... I believe it should be good to merge.

p.s. also a bump for web-platform-tests/wpt#4024 :-)

annevk pushed a commit to whatwg/fetch that referenced this pull request Feb 25, 2017
Requests with keepalive flag set are allowed to outlive the environment settings object. We want to make sure that such requests do not negatively impact the user experience when a page is unloaded, etc.

This limits the amount of (body) bytes that can be inflight at any point when the request has the keepalive flag set; this flag is also set by sendBeacon().

Background: w3c/beacon#39.

Tests: web-platform-tests/wpt#4878.
@toddreifsteck
Copy link
Member

LGTM! Merging. Tests are coming very soon.

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

Successfully merging this pull request may close these issues.

None yet

4 participants