whatwg / streams

Streams Standard
https://streams.spec.whatwg.org/
Other
1.34k stars 159 forks source link

What should we call ReadableByteStream.prototype.getBYOBReader()? #294

Closed domenic closed 8 years ago

domenic commented 9 years ago

Per the lengthy discussions in #253 the new shape of ReadableByteStream will be something like:

Here "BYOB" stands for "bring your own buffer," by the way.

Ideas for names:

Are there any precedents from other platforms we can draw on, just for vocabulary if nothing else?

yutakahirano commented 9 years ago

Naming after reader's type name looks natural to me.

domenic commented 9 years ago

Well, but do you have an idea for the reader's type name? :)

yutakahirano commented 9 years ago

[Readable]BytesStreamReader and getBytesReader, perhaps?

domenic commented 9 years ago

Yeah that kind of works, since it's about specialized behavior for byte streams. But then again, rbs.getReader().read() will return bytes too, hmm. I'll add it to the list :D

chrisdickinson commented 9 years ago

What about inverting it, such that getReader is the BYOB version, while the auto-allocating version becomes getAllocatingReader?

domenic commented 9 years ago

@chrisdickinson that's not as good, I think, because ideally getReader() on both ReadableStream and ReadableByteStream should return an auto-allocating readers with a no-argument read().

tyoshino commented 9 years ago

In offline discussion last Thursday, we categorized the readers we've been designing using a matrix.

Alloc / Pull Auto Manual
BYOB - A
Auto B C

(BYOB && Auto pull) doesn't make sense. So, it's marked -.

We're going to at least provide factory methods for A, B.

The "Alloc: auto" row also applies to object streams. Both of B and C make sense. I think C means that pulling one element.

For byte streams, I realized that I don't know what we should do for C well. C is manual pull. We're going to pull some bytes. How many? The user needs to call read() to pull bytes though they cannot specify the amount of data to pull. Domenic, what do you think about C? Is this kinda method to say "please read now, but decide how much to pull by yourself" to the reader?


So, if A, B and C make sense for byte streams, and B and C make sense to object streams, how should we organize the factory methods?

Gave verbose names for the first step.

domenic commented 9 years ago

Domenic, what do you think about C? Is this kinda method to say "please read now, but decide how much to pull by yourself" to the reader?

Yeah, exactly. This would probably be done intelligently on a per-stream basis. IIRC libuv has heuristics for this (since Node streams are not BYOB), @piscisaureus mentioned them to me once.

It also says, "I don't care about reusing buffers, and will let the JavaScript GC take care of freeing the memory".


I am starting to think that an options-object accepting getReader() may be better. In case we add more dimensions of variability in the future, it would avoid a combinatorial explosion in method names. Let us think what that would look like:

Or maybe we want the default to be auto-pulling, so instead of flowing: true it is manualPull: true. (Better name?) Then we have


The interesting thing about object streams B vs. C is that previously we kind of let this be controlled by the strategy instead of by the consumer. (Although even then I think we always would be trying to pull at least some before exerting backpressure.) We could move that all to configuration to getReader?

Wait, but how does C work for object streams that are wrapping push sources? I think the manualPull: true only makes sense for: (a) APIs that are inherently pull-based like read(2)-on-files; (b) APIs that allow us to use the kernel buffer to store data (but that only goes up to a limit before we start losing data...) like recv(2)-on-sockets. But it does not work for APIs like web sockets that are pushing data at you via events and you will lose the data if you don't react...

domenic commented 9 years ago

Upon further reflection I am not sure auto-pulling vs. manual-pulling is a property of the reader. It may be more of a property of the stream. If it is a property of the reader, you should be able to get an auto-pulling reader, then release it and get a manual-pulling one. Or vice versa. That seems strange and potentially hard to implement.

tyoshino commented 9 years ago

OK. Maybe "auto-pull" concept doesn't make sense when discussing a reader interface. What's going on behind a reader is unknown and abstracted. We basically shouldn't mention about anything behind it when naming the methods.

Pulling vs. non-pulling (give me when available) is the right way of comparison?

domenic commented 9 years ago

Tentative plan per afternoon discussions:

domenic commented 9 years ago

@tyoshino I am coming back to this issue several months later and it seems we kind of forgot about a lot of the ideas in https://github.com/whatwg/streams/issues/294#issuecomment-81557090. Given how our thinking has evolved, e.g. in #329 and #361, what are your thoughts on that comment?

tyoshino commented 9 years ago

Thanks for reminder, Domenic. I'd like to begin with responding to https://github.com/whatwg/streams/issues/294#issuecomment-81557090

Use options

This is still true, I think. Less methods sounds better basically. E.g. if we decide to implement various readers as I listed in https://github.com/whatwg/streams/issues/329#issuecomment-106780286, the option takes mode: "borrowing", etc.

manualPull: false/true

I'm gonna investigate if I can integrate the strategy mechanism with the up-to-date RBS (has pull/pullInto) well.

Cases like a ReadableStream wrapping a WebSocket ...

BYOB of the up-to-date RBS should just work (though involves copy).

domenic commented 9 years ago

I have spun off a lot of that stuff into https://github.com/whatwg/streams/issues/375. That leaves this thread for its original purpose, bikeshedding the name :).

So you think it should be

rbs.getReader({ mode: "byob" });
rbs.getReader({ mode: "default" }); // or... "pull"?
rbs.getReader(); // equivalent to default

The other alternative we were thinking was

rbs.getReader({ feedBuffers: true });
rbs.getReader({ feedBuffers: false });
rbs.getReader(); // equivalent to false

but I think that you are right that future expansion is better served by a string enum.

tyoshino commented 9 years ago

Ah, good point. Yeah, explicit parameters (boolean) for each feature is cleaner, but with mode string we can list only meaningful combinations and maybe robust for future extensions.

tyoshino commented 8 years ago

ReadableByteStream has been merged into ReadableStream (https://github.com/whatwg/streams/issues/379). Now ReadableStream has both getReader and getBYOBReader().

It's time to resume discussion on this issue and finalize the BYOB API surface.

domenic commented 8 years ago

I think your arguments for getReader() + getReader({ mode: "byob" }) are still pretty good. Should we move to that?

tyoshino commented 8 years ago

Reviewed the discussion in this thread. Yes, I still like the plan to add options to getReader().

Regarding the name, I think "buffer-provided" and "buffer-fed" are the candidates I'm still considering in addition to "byob".


BTW, the differentiation between "auto pull" and "manual pull" discussed around https://github.com/whatwg/streams/issues/294#issuecomment-81428113 has been well depicted in the new examples written by Domenic.

https://github.com/whatwg/streams/pull/430#discussion_r57444278 is also related.

In the latest spec, we chose to have the controllers expose queuing status via the desiredSize() getter to delegate pulling decision to the underlying source. Auto/manual pull is no longer part of the readable stream (and controller / reader) API but characteristics of underlying sources.

tyoshino commented 8 years ago

Now I feel that ReadableStreamBYOBController is not the best naming for the controller with the BYOB feature. It has facilities for BYOB reading, but can also work with the default reader. Its main and essential characteristic is that it handles bytes (expects bytes, copies and repackages bytes).

So, I propose:

domenic commented 8 years ago

Hmm. I still like keeping BYOB in those names, because BYOB is the new capability on top of the default. I think it would be OK to keep it. I think of the byob: true option to the underlying source to be something like byobCapable: true (just shorter). I agree that its essential characteristic is that it handles bytes, but it's main new capability is a BYOB API.

Among byob, buffer-fed, and buffer-provided I think byob is pretty good. Maybe it is just because we have been staring at that for a few months though. I think buffer-fed is better than buffer-provided probably.

tyoshino commented 8 years ago

Hmm. I still like keeping BYOB in those names, because BYOB is the new capability on top of the default. ...

In addition to https://github.com/whatwg/streams/issues/294#issuecomment-202748216, I'm thinking about the case in which we need to introduce the read() with ack functionality. If we introduce the feature, we would need to also add a new interface to the controller. Then, the controller will gain one more new feature in addition to BYOB. Considering such possibility, I was leaning toward giving more general name to it. Of course, we could just add a new controller. WDYT?

Among byob, buffer-fed, and buffer-provided I think byob is pretty good. Maybe it is just because we have been staring at that for a few months though. I think buffer-fed is better than buffer-provided probably.

OK. Let's go in "byob".

domenic commented 8 years ago

If we introduce the feature, we would need to also add a new interface to the controller. Then, the controller will gain one more new feature in addition to BYOB. Considering such possibility, I was leaning toward giving more general name to it. Of course, we could just add a new controller. WDYT?

Hmm, it is hard to say. Whether we introduce a new controller or not would depend on how different the internal state tracking would be.

I guess I see two options:

I am OK with either.

So the total changes being contemplated here are:

But no renaming for ReadableStreamBYOBReader or ReadableStreamBYOBRequest or for the underlying source option (byob: true). Right?

tyoshino commented 8 years ago

Thanks.

... depend on how different the internal state tracking would be.

Yeah. I can't come up with estimation of the complexity it would add for now.

... it since it is not very web-exposed ...

I see.

  • Merge getBYOBReader into getReader using { mode: "byob" } as an argument

Yes!

  • (Maybe) rename ReadableStreamBYOBController to ReadableStreamBytesController or ReadableByteStreamController But no renaming for ReadableStreamBYOBReader or ReadableStreamBYOBRequest or for the underlying source option (byob: true). Right?

I thought that if we rename ReadableStreamBYOBController, the underlying source option should also be renamed as it's an option telling what type of controller it needs or what set of reading operations the stream should expose.

But yeah, we could just add a new parameter say "bufferLeasing: true" in addition to byob.

So, ok.

domenic commented 8 years ago

I thought that if we rename ReadableStreamBYOBController, the underlying source option should also be renamed as it's an option telling what type of controller it needs or what set of reading operations the stream should expose.

Hmm, I see. I think the real question is: do we expect a 1:1:1 underlying source types <-> controller types <-> reader types. I guess we should not necessarily expect that. Right now we have 1:1 underlying source types <-> controller types, and that will probably stay forever. But the correspondence with reader types is already not 1:1 since you can get both default and byob readers from (what is currently called) a ReadableStreamBYOBController.

So, OK. We should reflect this asymmetry in the API. New plan:

(Maybe there is a better word than type, but I thought it was important to have different terms, so we could say that a given type of controller/stream/underlying source can have different modes for reading from it.)

tyoshino commented 8 years ago

Yeah, in the last comment of mine, I thought we could have the ReadableStream constructor parameters each corresponds to enabled getReader() mode, but I prefer your idea. Yes, 1:1 correspondence between underlying source types and controller types should be stable for long time.

Please review the PR.