Merge ReadableByteStream into ReadableStream

[tyoshino]

(This post was initially empty, but to summarize the changes, added a check list)

• sync the spec text with the new reference implementation
• merge ReadableStreamController and ReadableByteStreamController
• merge ReadableStreamReader and ReadableByteStreamBYOBReader
• Finalize pull/pullInto behavior #423
  • add byobRequest getter to the controller
  • adopt the while-loop based pulling routine for the non-byte source version
  • stop delaying pull based on the return value of the last pull call
• port the asserts in the non-byte source version to the byte source version
• remove auto release feature from the byte source version
• rename Byob to BYOB
• move TransferArrayBuffer to helpers.js
• make the default highWaterMark of the byte source version to 0
• port the functionality that the start method can delay pulling by returning a pending Promise to the byte source version
• add [[disturbed]] feature to the byte source version
• port highWaterMark mechanism to ReadableByteStreamController
• merge ReadableStream and ReadableByteStream
  • now it behaves differently based on whether the given source has pullInto or not
  • bunch of renaming for merge
• merge ReadableStreamReader and ReadableByteStreamReader
• Revisiting what's reasonable return value for each method of the controller #424
  • make controller.close() and controller.enqueue() fail when the stream is not in the readable state
  • make controller.enqueue() throw a predefined TypeError, not the stored error
• Rename ReadableStreamController to ReadableStreamDefaultController
• Rename ReadableStreamReader to ReadableStreamDefaultReader
• Rename ReadableByteStreamController to ReadableStreamBYOBController

[tyoshino]

Since some changes such as auto-release removal are not reflected to RBS yet, they don't looks very close yet. But except for them, they share most of code now.

To make it clear which operation is exposed to controller implementations, I've put some comments.

[tyoshino]

Note

• No spec text change made yet
• Tee is not yet updated yet

to get initial high level review first before working on them

[tyoshino]

Here's a quick explanation of the interfaces between controllers and the readable byte stream.

Readable byte stream controller:

• probes what kind of reader the stream is locked to by ReadableByteStreamHasReader() and ReadableByteStreamHasByobReader()
• probes how many requests there are by GetNumReadRequests() and GetNumReadIntoRequests()
• responds to the requests by RespondToReadRequest() and RespondToReadIntoRequest()
• manipulates the stream's state by CloseReadableByteStream(), ErrorReadableByteStream()
• invokes the pull operation on the underlying byte source appropriately based on the signals from the stream (reader) which are PullFromReadableByteStream() and PullFromReadableByteStreamInto(). Controllers must implement these operations.

[domenic]

Awesome, thanks so much for working on this.

So the readers are still owned by the stream? That is probably good, and simpler than what I was originally thinking of.

Let me try to write a summary to help get my head around it. I will use ReadableStream since it is simpler.

ReadableStream has:

• state, reader, storedError, disturbed, and controller internal slots
• AddReadRequestToReadableStream, GetNumReadRequests, RespondToReadRequest "spec internal methods" to allow the controller to manipulate its controlled readable stream (and its reader) without manipulating internal state directly.

ReadableStreamController has:

• controlledReadableStream, underlyingSource, strategySize, strategyHWM, queue, started, closeRequested, pulling, pullAgain internal slots.
• CancelReadableStreamController, EnqueueInReadableStreamController, ErrorReadableStreamController, GetReadableStreamControllerDesiredSize, ReadableStreamControllerCallPullIfNeeded, ShouldPullReadableStreamController "spec internal methods".

The eventual path forward, then, would be:

• Convert ReadableStreamController's spec-internal methods into actual internal "methods", probably written something like ReadableStreamController@[[Cancel]](...). In source code we can name these _Close() and have them be methods in the class, which will be nice.
• Match up all controller internal methods so that both ReadableStreamController and ReadableByteStreamController have the same set of internal methods.
• Convert ReadableStream to call this@[[controller]]@[[Cancel]](...), thus getting polymorphism for when we allow constructing ReadableStreams with ReadableByteStreamControllers, and merging the two classes.

This sounds like it will work really well. There are probably some more complexities for the controller <-> reader interaction, but what you have so far makes sense.

[tyoshino]

So the readers are still owned by the stream? That is probably good, and simpler than what I was originally thinking of.

Yes. Operations on the reader is hidden to the controller.

ReadableStream has:

• state, reader, storedError, disturbed, and controller internal slots

Right.

• AddReadRequestToReadableStream, GetNumReadRequests, RespondToReadRequest "spec internal methods" to allow the controller to manipulate its controlled readable stream (and its reader) without manipulating internal state directly.

Right. [[state]] and [[storedError]] are also visible to the controller but not manipulated directly by the controller.

ReadableStreamController has:

• controlledReadableStream, underlyingSource, strategySize, strategyHWM, queue, started, closeRequested, pulling, pullAgain internal slots.
• CancelReadableStreamController, EnqueueInReadableStreamController, ErrorReadableStreamController, GetReadableStreamControllerDesiredSize, ReadableStreamControllerCallPullIfNeeded, ShouldPullReadableStreamController "spec internal methods".

and CloseReadableStreamController. Currently they all contains "ReadableStreamController" in their names and the others don't.

Ah, I just noticed that in the last post I forgot to include CancelReadable(Byte)StreamController when listing the interfaces that controllers must implement.

The eventual path forward, then, would be:

• Convert ReadableStreamController's spec-internal methods into actual internal "methods", probably written something like ReadableStreamController@[Cancel]. In source code we can name these _Close() and have them be methods in the class, which will be nice.

It's really nice! It'll be clear which operation belongs to which object.

• Match up all controller internal methods so that both ReadableStreamController and ReadableByteStreamController have the same set of internal methods.
• Convert ReadableStream to call this@[[controller]]@[Cancel], thus getting polymorphism for when we allow constructing ReadableStreams with ReadableByteStreamControllers, and merging the two classes.

Yes!

This sounds like it will work really well. There are probably some more complexities for the controller <-> reader interaction, but what you have so far makes sense.

Yeah. The interface adds complication but modularization would make each class easier to understand, I guess.

[tyoshino]

Up to 7beb159, made some bug fixes, more refactoring, enabled tee

[tyoshino]

Made some more commits. Merged 2e7f87e, added distributed property, and reflected auto release feature removal to the readable byte stream.

[tyoshino]

I don't think this is a good idea, or feasible. That is where the main separation is. RBSC has byobRequest on it, after all.

Agreed! Yeah, given byobRequest addition, it's no longer good idea.

[tyoshino]

As discussed at #423 (comment), I've removed the synchronous repeated pulling mechanism. Pulling criteria checking code has been simplified to be the same as ReadableStream with non-byte source.

Error propagation was and is still done correctly.

[tyoshino]

I'm less sure about this, but it still seems reasonable to me that these be separate types... what do you think?

Hmm, ok. Having read(view) on a reader obtained by getReader() is just confusing. ReadableStream with non-byte source still has getBYOBReader() but should be less confusing.

[tyoshino]

Sorry. The suggestion was not to add read(view) on the reader obtained by getReader(), but make the reader obtained by getBYOBReader() (or could be renamed to getByteReader() or something) provide both read() and read(view).

[domenic]

Oh, I see, I didn't realize. So our two options are:

• non-BYOB streams have getReader() that if you do .read(view) it fails; BYOB streams have getReader() that allows .read(view)
• non-BYOB streams have getReader() that if you do .read(view) it fails; BYOB streams have .getReader() that also disallows .read(view), but they have .getByobReader() that allows .read(view) (and disallows .read()?).

Now that you lay this out, I think option 1 is better. But! I still don't think we need to merge the two types, unless that is convenient for spec/implementation. We can just have .getReader() return a normal reader for non-BYOB streams, and a BYOB reader for BYOB streams. Or we can merge them into one type; up to you :)

[tyoshino]

Sorry for delay!

Yeah, the option 1 is the plan I imagined. Maybe to make it less confusing, we need to rename .read(view) to .readInto(view).

One good thing is that we won't have .getBYOBReader() on a ReadableStream which is constructed with the byob option set to false.

In terms of code cleanness, it's ok not to make the merge happen since we've already simplified it by merging the operations behind the methods enough.

[tyoshino]

Maybe it's time to proceed to make this ready for commit. I'll create another branch not to lose the review interaction details, and add text changes to it.

[tyoshino]

Created a new PR: #430

[tyoshino]

It's time to do section reorganization previously suggested/investigated by @domenic at #418 (comment).

In addition to the idea by Domenic, can we separate internal operations and ones can be used directly by clients of each class? Too much?

For example, InitializeReadableStreamReaderGeneric() is just common code factored out from the two readers. Not indented for use by clients like ReadFromReadableStreamReader().

[tyoshino]

Regarding spec writing, I'd like to get agreement on the following items. Maybe it's good time to establish consistent rules.

• id for headings: Keep using abbreviations like rs- or just write down all phrases?
  • e.g. rs-controller-enqueue for enqueue() in ReadableStreamController, or rename it to readable-stream-controller-enqueue?
• operation naming: We've been giving names that can be read roughly as an English phrase. I think. But This led to long moving of functions on renaming as they've been placed in the lexicographical order. As we're introducing more hierarchical sections for the abstract operations, the problem would no longer matter, but that said, common prefix is simpler and some times readable than the English phrase like style. What do you think? E.g. ReadFromReadableStreamReader would be renamed to just ReadableStreamReaderRead. All the operations on ReadableStream would have the same prefix "ReadableStream".
• Any preference on the ordering of operations implementing the logic of class methods excluding the brand checking? E.g. GetReadableStreamControllerDesiredSize() would be placed after CloseReadableStreamController() when ordered lexicographically, but the corresponding methods are placed in a different order since in the class representation we're placing getters/setters before the other methods.

[tyoshino]

Domenic, in the example at #418 (comment) you've put all the abstract operations in the same section but giving sub sections under it. Does this mean that you think we should use one section parallel to sections explaining each class?

Can we choose to put each operation to the corresponding big section explaining the class the operation operates on?

[domenic]

In addition to the idea by Domenic, can we separate internal operations and ones can be used directly by clients of each class? Too much?

Hmm. My first impression is that it's a bit too much, but I'm not sure. If you want to draft a quick bullet-point version of what the TOC would look like with your idea, we could discuss further.

Domenic, in the example at #418 (comment) you've put all the abstract operations in the same section but giving sub sections under it. Does this mean that you think we should use one section parallel to sections explaining each class?

Can we choose to put each operation to the corresponding big section explaining the class the operation operates on?

What I meant there was that, similar to the current spec, we want a section underneath "3 ReadableStreams" for abstract operations. Call it "3.5 Readable Stream Abstract Operations". Within that there'd be subsections.

If you think the abstract operations divide up cleanly enough that they could be nested under the class sections (so, e.g., "3.2.5 Abstract Operations for ReadableStream", "3.3.5 Abstract Operations for ReadableStreamController", etc.) then maybe that would work. I am a bit skeptical though. It seems like a lot of them are kind of hard to decide which class they are associated with. The organization in my comment seems more likely to be useful for readers.

In general, I'm open to suggestions on proposed organization; the best way to do that is to make concrete proposals with full TOCs.

id for headings

I think we should keep the abbreviations, and we should endeavor to preserve as many links as possible. Nice short and well-curated IDs makes sense to me, for a spec that is of manageable size like this one.

I could imagine moving the abstract ops to a different ID style, like #ao-IsReadableStream. But that would break existing incoming links from e.g. Fetch, and I am not sure it is worth it.

operation naming

I agree that it is probably a good idea to rearrange them with prefixes like you suggest.

Any preference on the ordering of operations implementing the logic of class methods excluding the brand checking?

I think it's OK for them to not correspond perfectly. Let's keep it lexicographic.

[tyoshino]

Done

• Add all new methods, kill all old methods.
• Rename ReadableStreamController to ReadableStreamDefaultController
• Rename ReadableStreamReader to ReadableStreamDefaultReader
• Rename ReadableByteStreamController to ReadableStreamBYOBController
• Reorganize the section based on the idea at Merge ReadableByteStream into ReadableStream #418 (comment) and discussion at Merge ReadableByteStream into ReadableStream #418 (comment)
• IDs: Given consistent IDs. Please review if they comply your principles, Domenic.
• Renamed operations to have a common prefix, and then reordered them
• Made bunch of bug and typo fixes.

[tyoshino]

So, it's done. Sorry but the diff is huge... Please take a look at #430.