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

Clarify allowed behavior when response is "too big" to "fit" in memory/on disk? #244

Open
mkruisselbrink opened this issue Apr 12, 2019 · 6 comments

Comments

@mkruisselbrink
Copy link

In Chrome, as part of a larger XHR-to-blob refactoring, we shipped a change a while ago that changed the behavior when the resulting blob would be too large to fit in memory/on disk. Before we would download as much as we could fit on disk, sending progress events while doing so, and eventually run out of disk space and at that point error out the request.

After the change, we look at the Content-Length header ahead of time (to decide between various strategies of dealing with blobs), and immediately fail the request if we already know we're not going to be able to store the whole request. This seemed safe at the time, since one way or another the request is going to fail anyway, however at the time I wasn't realizing that the difference would be web observable via the (lack of) progress events, and this seem to have actually broken at least one (speed test) website.

This isn't an area where behavior is really fully specified (at least the only hint I can feel around behavior for when the result of an XHR is too big to "fit" is the "Allocating an ArrayBuffer object is not guaranteed to succeed." note, but even that only happens after all the bytes have been fetched and somehow magically stored).

So as written it seems a conformant implementation needs to always download all the bytes, emit progress events along the way, and can only fail after that. Should failing earlier when we know the overall fetch is not going to succeed be allowed?

@annevk
Copy link
Member

annevk commented Apr 15, 2019

I guess I still don't quite see why this applies to XMLHttpRequest, but not say, <img>.

@mkruisselbrink
Copy link
Author

I suppose it applies to anything that needs to load the entire resource in memory or store it on disk. Not sure if anything other than XHR has progress events that make this as visible to the web though. Without progress events the only difference is in the timing of the failure to fetch. But since XHR has progress events there is another difference in those progress events no longer being emitted if we abort the fetch as soon as we know it is ultimately going to fail.

@annevk
Copy link
Member

annevk commented Apr 16, 2019

The img element has progress events in its specification. Depending on how smart you are about it you could also terminate certain fetch()es earlier I suppose. Or a blob() invocation on a clone while letting the streaming variant continue... Hmm, not sure we should go there. 😊

@mkruisselbrink
Copy link
Author

The img element has progress events in its specification.

That's what I get for trusting stack overflow rather than reading the actual spec... Yeah, in that case that seems like it would pretty much be the same situation.

Depending on how smart you are about it you could also terminate certain fetch()es earlier I suppose. Or a blob() invocation on a clone while letting the streaming variant continue... Hmm, not sure we should go there. 😊

Yep... lots of "opportunity" for "optimizations". As currently written the various specs mostly seem to imply that an implementation should always fetch all the bytes, and if the spec says anything about browser resources allowing failure (like for XHR to ArrayBuffer), such failure can only happen after all the bytes are read. It is of course possible to implement that (just throw away the bytes as we're fetching them, but don't report an error until the whole thing is fetched), but I doubt any implementation does that today. Of course what happens when you exhaust system resources isn't really very well specified anywhere, so not sure how important this is, except that apparently websites are relying on certain behavior for operations that would have exhausted resources had the websites not canceled the operation before it got to that point...

@wanderview
Copy link
Member

Also, in theory progress for any request is observable via the Response body stream in a service worker handling the FetchEvent.

@AMorgaut
Copy link

As a Green IT evangelist, I would say

"such implementation is great"
"please fail early and interrupt useless transfert and process costs"

As a JS developper, I would say

"I can be interested to know that the request will eventually fail"
"But my user may be frustrated if I don't at least handle the data I'm capable of..."

That's the whole power of streams and progress events, you can potentially provide services / features as soon as you start receiving the 1st bits of a response

ex: you can start to fill a Grid with useful information for the user

But for sure, for such usage:

  • I would prefer using fetch and its streams than xhr
  • I'd expect the backend to send chunk encoded responses, not ones with a Content-Length - BUT frontend devs don't always have control on backend implementations...

Idem regarding images

What UX would you expect:

  • a partially loaded image
  • an image that is not shown at all

The difference is probably that this choice is usually automatically based on chunked vs identity transfert encoding

So What

My 1st approach would be "don't break the web", and this change may break it at some point. Probably in a very limited way, but to check it seriously, tests should be done in areas people are using limited devices.

What about applying this optimization only when there no progress event listeners?

@whatwg whatwg temporarily blocked greg9762 Nov 22, 2021
@whatwg whatwg deleted a comment from greg9762 Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants