w3c / beacon

Beacon
https://w3c.github.io/beacon/
Other
46 stars 22 forks source link

Ambiguity on "maximum data size" limit and enforcement #38

Closed igrigorik closed 7 years ago

igrigorik commented 7 years ago

The user agent should restrict the maximum data size to ensure that beacon requests are able to complete quickly and in a timely manner.

The sendBeacon method returns true if the user agent is able to successfully queue the data for transfer. Otherwise it returns false.

(note) If the user agent limits the amount of data that can be queued to be sent using this API and the size of data causes that limit to be exceeded, this method returns false. A return value of true implies the browser has queued the data for transfer.

As of today, the spec does not specify a specific maximum data size limit. This was an intentional decision when we were iterating on the early drafts; we wanted to allow UA's to experiment and adjust the limits in the future as needed. Fast forward a few years...

Technically, all implementations are "spec compliant", because we did not spell out the limit or exact conditions for how it should be enforced. However, the differences are also causing failures in Chrome for the proposed web-platform-tests: https://github.com/w3c/web-platform-tests/pull/4024#discussion_r84789110. Some thoughts and options...

a) I don't think we should spec a hard limit; I think it still makes sense to allow UAs to adjust this if and when needed. However, as a matter of general guidance to developers, I think we probably should add a non-normative note in the doc indicating that multiple browsers picked 64KB for their initial implementation.

b) Given that we don't spec a hard limit, I don't think web-platform-tests should hardcode 64KB either. It's good to have tests for "super large payloads are not allowed", but perhaps we can just change that number to something much larger in our tests.. e.g. >=1MB. That way, if an existing implementation decides to increase their limit, they're not immediately greeted with a bunch of test failures.

c) sendBeacon adoption has been growing steadily in Chrome, and our telemetry for quota exceeded shows that very few pages hit the 64KB quota; I've not heard developer complaints about it, so far, at least.. As such, I'm inclined to say that this behavior shouldn't be treated as a test failure either. It may be the case that it's a bit too restrictive (e.g. SPA app that sticks around for hours~days and uses sendBeacon under the hood), and we might want to revisit this behavior in the future (e.g. by raising the quota limit; only apply the quota requirement for beacons that fire when queued when the page is onloading; switch to same per-beacon enforcement as Edge/FF; ...), but I think it's a reasonable implementation under current spec language and should be allowed.. unless we can point to specific cases where it breaks. My proposal for this one is:

Additional notes and related discussions:

/cc @toddreifsteck @yoavweiss @nicjansma @plehegar

toddreifsteck commented 7 years ago

This type of thing is exactly the reason that we should all be submitting tests and discussing the outcome.

@igrigorik I don't agree that we should simply add a note. The current spec allows for an implementation that WILL harm SPAs that use sendBeacon to send 1k per minute in ~1 hour. I assert that we should tighten the specifications language around quotas to ensure that sendBeacon continues working and that quotas must either be "for the active queue" or "per beacon" or something similar.

If very few sites are hitting this behavior, I assert that is because very few SPAs are utilizing sendBeacon even with its rapid adoption.

igrigorik commented 7 years ago

That's fair. I think there are two threads here, let me try to unpack..

Should we specify a hard limit int he spec?

My position is that we should not, but we should add a note indicating what existing implementations have settled on. I don't hear any loud disagreements on this one?

Specify enforcement model?

@toddreifsteck I think your points are fair. That said, the primary reason why Chrome went with the quota approach is to provide some guarantee that we won't be saddled with a large POST payload as the page is being unloaded -- e.g. "please upload this 3MB trace of my app while you unload, kthnx". Enforcing a per beacon limit helps, but does not address this entirely, as one could just queue a few dozen of those. Perhaps we can find a happy medium here...

toddreifsteck commented 7 years ago
  1. I agree that we should not specific the specific hard limit.
  2. I think we should clean up the spec with regard to the enforcement model.

I can only think of one requirement which led to the current quotas:

@igrigorik Can you think of any other requirements? After we've locked down requirements, we can take a shot at some rough spec language to lock in this requirement.

For what its worth, I agree that all of our quotas fail to be elegant solutions to that requirement and believe if we can lock this down for Beacon which is in heavy use today, we will have helped push the concept forward.

igrigorik commented 7 years ago

Protect user from large network requests being kept open after the page has navigated away

Yep, as far as quota is concerned, I think that's it.

toddreifsteck commented 7 years ago

However... I think that specifying that the outstanding sendBeacon request queue has a rolling quota that is incremented on request and decremented when the request is completed. I also assert that we should note that 64k is the recommended size of this quota. Firefox, Chrome and Microsoft Edge have all chosen the same number.

I think this model is also a bit cleaner with regard to Service Worker and protects users more than the per-request 64k quotas implemented in Firefox/Microsoft Edge. It also improves upon Chrome's 64k limit for a context by allowing SPAs to continue working after sending 64k of payload over time.

I'm certain that Microsoft Edge could implement this quickly and we could submit updated tests.

@igrigorik What do you think?

igrigorik commented 7 years ago

@toddreifsteck I like that.

@cbentzel @tyoshino curious to hear your thoughts on this one.

tyoshino commented 7 years ago

I agree that the decision on choosing the limit should be left to each implementation.

I think we should have a histogram of the size of payload used for sendBeacon() instead of getting the bool of "quota exceeded or not". If developers (even third-party contents providers) have been adjusting the payload size appropriately (otherwise, they'll lose the signal), we shouldn't see increase in the graph.

That said, as we're not hearing any complaint on it, I agree that 64k should be enough currently.

Re: enforcement model, I agree that applying the limit to the queue (not per-request) is the right thing to do given the requirement is that we protect the user from consumption of the network resource with unloaded page. So, the proposal is that we spec that sendBeacon's queue would fail when its size is exceeding the limit when the page unloads?

It probably doesn't make sense to enforce a lifetime quota while the page is active.

It makes sense with regard to the purpose of the limit, but I guess people would just limit the total amount of data they queue as it's hard to observe which sendBeacon has finished (and no longer occupying the queue). I don't object to it.

tyoshino commented 7 years ago

I'm fine with adding a note that most of UAs have chosen 64k.

tyoshino commented 7 years ago

but I guess people would just limit the total amount of data they queue as it's hard to observe which sendBeacon has finished (and no longer occupying the queue).

Regarding this comment of mine, Ilya, is it already common that sendBeacon() is used for periodical beacon-ing than using async XHR / Fetch API until unload happens?

igrigorik commented 7 years ago

So, the proposal is that we spec that sendBeacon's queue would fail when its size is exceeding the limit when the page unloads?

I believe the proposal is: limit sendBeacon to 64kb of in-flight data, regardless of the point in the lifecycle of the page. As part of the queue algorithm: increment in-flight data counter, once request has finished, decrement it by the request account. If attempting to queue > in-flight max, reject and return false.

Regarding this comment of mine, Ilya, is it already common that sendBeacon() is used for periodical beacon-ing than using async XHR / Fetch API until unload happens?

It doesn't appear so.. as I would have expected to hear from those developers about our 64kb max. That said, I do think that this is a pattern we want to encourage, so it is something we should fix.

toddreifsteck commented 7 years ago

I've done a lot of reviewing of XHR/sendBeacon resource timings on top 100 in the past week. Although sendBeacon is being adopted, a lot of telemetry is still on top of XHR. Also... all sample sendBeacon code I've seen has fallback code for using XHR when sendBeacon returns false. This means it is unlikely for sites to complain. (Said another way, we should fix this.)

tyoshino commented 7 years ago

I believe the proposal is: limit sendBeacon to 64kb of in-flight data,

OK. I was misunderstanding the timing of enforcement. Thanks, Ilya.

As part of the queue algorithm: ...

Seems I was understanding this part correctly.

I rethought the issue of lack of way to know the queue size I mentioned in https://github.com/w3c/beacon/issues/38#issuecomment-259598393. As we're going to implement the keepalive feature for the Fetch API, I think we'll suggest that developers who want to precisely control the queue size use the Fetch API with keepalive feature, and we just align the algorithm of sendBeacon with it. So, I now think your proposal of cap on in-flight makes sense.

Thanks Todd for the analysis.

igrigorik commented 7 years ago

Thanks everyone, sounds like we have consensus on the high-level approach.

Let's continue in https://github.com/w3c/beacon/pull/39. There is some layering bits that we need to figure out there..

andrewwakeling commented 7 years ago

Apologies if this is the wrong place to raise this concern.

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 commented 7 years ago

@andrewwakeling the quota is only enforced on the body payload. We don't restrict the number of requests in flight.

igrigorik commented 7 years ago

Resolved via https://github.com/w3c/beacon/pull/39.

collimarco commented 6 months ago

Are there any updates on this? What are the current browser limits?

I would like to use sendBeacon to autosave user documents (during a visibilitychange event), however I am not sure that it is a good idea...

This "max data size" on the POST request is really strange (and wrong in my opinion) and makes sendBeacon useless and unreliable for saving user documents...

clelland commented 6 months ago

I'm not sure what the current cross-browser state is here, (and I don't know if there has been any substantial update in ~7 years)

However, FYI, fetchLater is being proposed as a more reliable method of sending data back to a server on visiblitychange or even before actual unload, without requiring that you use an unload handler. There are still data size limits, but they are larger and scoped to the (source-document, destination-origin) pair.

If that fits your use case, and you're able to wait for browsers to catch up (and certainly even proposing a change to sendBeacon would require waiting for browser support) then that might be useful for you.