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

Add .bytesRead to ReadableByteStream.*Reader #367

Closed
tyoshino opened this issue Jun 18, 2015 · 14 comments
Closed

Add .bytesRead to ReadableByteStream.*Reader #367

tyoshino opened this issue Jun 18, 2015 · 14 comments
Labels

Comments

@tyoshino
Copy link
Member

@tyoshino tyoshino commented Jun 18, 2015

Putting together @domenic's reader.bytesRead idea to address yutakahirano/fetch-with-streams#37:

Request/Response constructor saves the current value of .bytesRead #362 (comment)

const rbs = ...;  // BodyInit as-is, or created from ArrayBuffer, etc.
this._bodySize = ...;  // undefined for RBS. The length of BodyInit for the others
const reader = rbs.getReader();
this._bytesReadAtConstruction = reader.bytesRead;
reader.releaseLock();

cache.put(), etc. checks if req._bytesReadAtConstruction has been changed or not.

const reader = req.body.getReader();
if (reader.bytesRead !== this._bytesReadAtConstruction) {
  throw new TypeError();
}
// Proceed with consuming body

fetch() computes the number of bytes remaining to generate the up-to-date value to be used for the Content-Length header.

const reader = req.body.getReader();
if (req._bodySize !== undefined) {
  const currentSize = req._bodySize - reader.bytesRead;
  // Generate Content-Length header
}
@tyoshino

This comment has been minimized.

Copy link
Member Author

@tyoshino tyoshino commented Jun 18, 2015

This proposal satisfies (1) (detecting partial consumption), (2), (3) (visible only to one who has lock), (4) (as long as the user-authored RBS conforms to the spec) of yutakahirano/fetch-with-streams#37 (comment).

Domenic said ok to give up on (6) at yutakahirano/fetch-with-streams#37 (comment).

(5) and (7) are not satisfied. They can be satisfied only when a wrapper is used.

@tyoshino tyoshino changed the title Add `.bytesRead` to ReadableByteStream.*Reader Add .bytesRead to ReadableByteStream.*Reader Jun 18, 2015
@domenic

This comment has been minimized.

Copy link
Member

@domenic domenic commented Jun 18, 2015

I'd ideally like to generalize this to ReadableStream as well, using the size() measuring functionality from the queuing strategy. In which case it needs a better name. consumedSize? consumed? readCount? readSize? offset? (I think I like offset the most.)

Also, I would really appreciate if anyone else has scenarios where this property is useful, besides HTTP streams and Content-Length. I guess in general it can be useful for progress meter type things. What else?

@tyoshino

This comment has been minimized.

Copy link
Member Author

@tyoshino tyoshino commented Jun 24, 2015

I'm fine with offset.

  • The good point of offset and fooCount is that it sounds like holding an accumulated historical data.
  • The bad point of count is that it sounds like counting objects, not the result of size()
  • offset may sound like it's settable to perform seeking, but it doesn't matter much.
@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Jun 24, 2015

I don't see how this addresses Content-Length as I tried to explain in yutakahirano/fetch-with-streams#37 (comment).

@tyoshino

This comment has been minimized.

Copy link
Member Author

@tyoshino tyoshino commented Jun 25, 2015

Like this?

Switch on BodyInit's type:

  • Blob, BufferSource, FormData...:
    • Save the original length as originalLength. When passed to fetch(), calculate currentLength by originalLength - rbs.offset and generate Content-Length header using currentLength.
  • ReadableByteStream obtained from onfetch event w/ original length info:
    • Same as above
  • ReadableByteStream obtained from cache (assuming the final length is known):
    • Same as above
  • ReadableByteStream obtained from onfetch event w/o original length info:
  • User authored ReadableByteStream:
    • Same as above
@tyoshino

This comment has been minimized.

Copy link
Member Author

@tyoshino tyoshino commented Jun 25, 2015

Oh, wait... We're likely to just prohibit fetch(req2) in yutakahirano/fetch-with-streams#37 (comment)?

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Jun 25, 2015

So yeah, it depends on whether we prohibit a partially read Response/Request from being transmitted. It does not make much sense to me to transmit them in part. A Response/Request is a body + metadata. It's not a partial body + metadata. Is there any precedent for this? (Other than HTTP range, but that does not seem to be what this is about.)

@domenic

This comment has been minimized.

Copy link
Member

@domenic domenic commented Jun 25, 2015

I think it is a matter of convenience. E.g. it could be convenient, if we allow it, to request a body from one URL, read off the first 200 bytes (the "header" portion), then upload the rest elsewhere. Another scenario is e.g. in a service worker intercepting an incoming response and stripping off a header or similar before calling respondWith.

If we disallow such things, then it becomes more manual work for authors, and possibly more memcpys. (Especially since we backed away from writable streams for uploads.)

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Jun 25, 2015

That can cause attacks too though. E.g. tricking the server into cutting off some set of bytes that contain an important script. Per whatwg/fetch#61 (comment) I think we want offset, but I'm not sure we should do this, at least not initially. Note also whatwg/fetch#67 which this would make even worse.

@domenic

This comment has been minimized.

Copy link
Member

@domenic domenic commented Jun 25, 2015

Can you explain the attack model more? As I said, it's literally just convenience; you could replicate the same thing with more manual work.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Jun 25, 2015

Say the document's CSP allows resources from "https://example.com/test". Now the service worker modifies the resource by reading the part from it that comments out a script or some such, but we don't make it clear it's now a synthetic response (or is that the plan?). Hole.

@domenic

This comment has been minimized.

Copy link
Member

@domenic domenic commented Jun 25, 2015

Ah I see, yeah, we'd want to mark anything with a nonzero offset as a synthetic response I think.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Jun 25, 2015

That might be okay then, but it makes the synthetic response check a bit more complicated since you'll have to check the combination of the response and the stream rather than just a bit on the response. And remember to do that. Rejecting seems easier for now...

@domenic

This comment has been minimized.

Copy link
Member

@domenic domenic commented Aug 3, 2015

Let's close this as it's no longer necessary given the plan to expose IsReadableStreamDisturbed (#378).

@domenic domenic closed this Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.