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

ReadableStream.getReader name is a bit misleading #308

Closed
youennf opened this issue Mar 30, 2015 · 38 comments
Closed

ReadableStream.getReader name is a bit misleading #308

youennf opened this issue Mar 30, 2015 · 38 comments

Comments

@youennf
Copy link
Contributor

youennf commented Mar 30, 2015

getReader name is a little bit counter-intuitive to me.
This method is really a constructor.
Its name seems to imply that it could return the same object if called several times on the same stream.
Why not simply calling it ReadableStream.createReader?

@wanderview
Copy link
Member

I think conceptually the stream can only have one reader at a time. The stream is locked by getReader(), so if you try to call it again you get an exception.

Given those semantics, I think "get" is a bit better than "create". Naming is hard, though.

@domenic
Copy link
Member

domenic commented Mar 30, 2015

Yeah, @wanderview covered my point of view pretty nicely. Even though it does get a new one each time it's (successfully) called, "conceptually" only one "real" reader exists per stream. The mapping of that conceptual reader to JS objects is not one-to-one though.

@youennf
Copy link
Contributor Author

youennf commented Mar 30, 2015

From a dev point of view, getReader is really 'lock the stream and get me
the thing that will allow me to access to the data'. The thing is called a
reader and its lifetime is controlled with releaseLock.

I would really avoid using getXXX in case one can never get the same JS
object through this method. The notion of locking seems more important to
convey.

The single-reader is a nice meta-model but I doubt users will actually have
that in mind.
Y
Le 30 mars 2015 20:30, "Domenic Denicola" notifications@github.com a
écrit :

Yeah, @wanderview https://github.com/wanderview covered my point of
view pretty nicely. Even though it does get a new one each time it's
(successfully) called, "conceptually" only one "real" reader exists per
stream. The mapping of that conceptual reader to JS objects is not
one-to-one though.


Reply to this email directly or view it on GitHub
#308 (comment).

@domenic
Copy link
Member

domenic commented Mar 30, 2015

I mean, suggestions welcome. I don't think it's unprecedented for getX() functions to return differnet objects each time---in fact I think that's the norm, given that if you're going to return the same thing each time, you'd just use a getter property. But I agree there's a design space here with the potential for improvements.

@youennf
Copy link
Contributor Author

youennf commented Mar 30, 2015

What about acquireLock?
With the possibility for releaseLock to return the released stream maybe?
Le 30 mars 2015 22:17, "Domenic Denicola" notifications@github.com a
écrit :

I mean, suggestions welcome. I don't think it's unprecedented for getX()
functions to return differnet objects each time---in fact I think that's
the norm, given that if you're going to return the same thing each time,
you'd just use a getter property. But I agree there's a design space here
with the potential for improvements.


Reply to this email directly or view it on GitHub
#308 (comment).

@domenic domenic modified the milestone: Week of 2015-03-30 Mar 30, 2015
@domenic
Copy link
Member

domenic commented Mar 31, 2015

Why is acquire better than get? It just seems like a fancy synony that takes longer to type and spell.

@tyoshino
Copy link
Member

  • "acquire" sounds like receiving some exclusive right to me more than "get". So, I understand youennf's point. But maybe it's just because I see "acquire"s in texts discussing mutex locks most. I'm not English native so don't know the nuance in general.
  • "lock" is just a needed side-effect (part of) for getting the exclusive read access. The main functionality of the interface from the view point of developers is to read data. So, inclusion of "lock" doesn't sound so good to me.

I agree we should use some better name if possible that implies:

  • start reading
  • acquire read access
  • create, attach and return a reader

But I don't come up with anything that is as short and simple as "getReader".

@wanderview
Copy link
Member

In gecko we use take for this sort of thing since its short and suggests exclusive access to the resource. So takeReader(). I don't have a strong opinion, though. /bikeshed

@tyoshino
Copy link
Member

tyoshino commented Apr 1, 2015

"take" also sounds good to me

@youennf
Copy link
Contributor Author

youennf commented Apr 1, 2015

+1
I can go over updating the tests, ref impl and so on, if that helps.

2015-04-01 6:02 GMT+02:00 Takeshi Yoshino notifications@github.com:

"take" also sounds good to me


Reply to this email directly or view it on GitHub
#308 (comment).

@domenic
Copy link
Member

domenic commented Apr 1, 2015

I definitely don't think takeReader makes sense.

At this point I think we need evidence that other getX() methods on the web platform return the same object each time. My suspicion is the opposite---that getReader() is already consistent with the platform.

@domenic
Copy link
Member

domenic commented Apr 1, 2015

Indeed. Here are other methods that follow the getX() pattern and return a new object every time:

  • From DOM:
    • getElementsByTagName/getElementsByTagNameNS
    • getElementsByClassName
  • From HTML:
    • PropertyNodeList.prototype.getValues
    • Document.prototype.getElementsByName
    • Document.prototype.getItems
    • TextTrackCue.prototype.getCueAsHTML
    • CanvasRenderingContext2D.prototype.getImageData/getImageDataHD
    • CanvasDrawingStyles.prototype.getLineDash
    • DataTransfer.prototype.getData
    • DataTransferItem.prototype.getAsFile

And that's just going through some IDL files... The most clear-cut analogous examples are getAsFile, getCueAsHTML, and getImageData/getImageDataHD.

There are indeed a few instances where the same object could be returned, e.g. getAttribute or getElementById. But there's definitely no consensus. In general, when something is supposed to be stable over time, a getter is used (node.ownerDocument, not node.getOwnerDocument()).

Does this allay your concerns, @youennf?

@youennf
Copy link
Contributor Author

youennf commented Apr 1, 2015

These examples look all good as getters.
I might be wrong but I think all of them do not change the state of the
callee.
You can call them multiple times, you will retrieve the same data, albeit
different containers.

Calling getReader changes the state of the stream in observal ways: calling
getReader again will raise an exception. If getReader is called and the
reference to the returned object is lost, you are screwed, there is no way
you can get access to the data. I would not expect that from a getXX method.

Acquire, take or any other similar name give a hint that you should take
great care of the returned result. They also do not convey the idea that
you are doing something safe.
Oh well... maybe I am too much HTTP biased ;)

2015-04-01 15:12 GMT+02:00 Domenic Denicola notifications@github.com:

Indeed. Here are other methods that follow the getX() pattern and return a
new object every time:

  • From DOM:
    • getElementsByTagName/getElementsByTagNameNS
    • getElementsByClassName
      • From HTML:
    • PropertyNodeList.prototype.getValues
    • Document.prototype.getElementsByName
    • Document.prototype.getItems
    • TextTrackCue.prototype.getCueAsHTML
    • CanvasRenderingContext2D.prototype.getImageData/getImageDataHD
    • CanvasDrawingStyles.prototype.getLineDash
    • DataTransfer.prototype.getData
    • DataTransferItem.prototype.getAsFile

And that's just going through some IDL files...

There are indeed a few instances where the same object could be returned,
e.g. getAttribute or getElementById. But there's definitely no consensus.

Does this allay your concerns, @youennf https://github.com/youennf?


Reply to this email directly or view it on GitHub
#308 (comment).

@tyoshino
Copy link
Member

tyoshino commented Apr 2, 2015

@tyoshino
Copy link
Member

tyoshino commented Apr 2, 2015

Domenic, could you explain the reason why you think "take" doesn't make sense?

(wanna hear it though I'm fine with "get")

@domenic
Copy link
Member

domenic commented Apr 2, 2015

taking something to me means that it's already there, and you're taking it from someone else who has it. That's not really what's going on, or even how it should be conceptualized. I would put takeReader on some class that gets a reader, holds it, but might want to let others have it for a while while it's not using it.

Also my objection to acquire is that it's basically synonymous with "get", at least how I hear the two words used in my life. ("Go acquire me a coffee" is just a more formal/sillier way of saying "go get me a coffee".) It doesn't really imply exclusive access in my experience. So I don't understand @youennf's arguments that "get a reader" feels safe/idempotent whereas "acquire a reader" does not.

I think at one point we called it getExclusiveReader which might give the explicitness desired. But I'd rather it be known that all readers are exclusive, so people don't have to type that reminder every time they want a reader.

@tyoshino
Copy link
Member

tyoshino commented Apr 2, 2015

Thanks. Updated the list.

@sparty02
Copy link

sparty02 commented Apr 3, 2015

What does the reader abstraction give you that couldn't be done via a change in semantics of the read method? In other words, why not just stream.read() ? What else can you call on the reader?

@domenic
Copy link
Member

domenic commented Apr 3, 2015

@sparty02 multiple consumers can have a stream; only one consumer (at a time) can have a reader. See the mega-thread #241.

@sparty02
Copy link

sparty02 commented Apr 3, 2015

@domenic Thanks for the context! Intuitively, I would have thought that one would construct a reader and compose in a stream that the reader would operate on. i.e. :

const reader = new StreamReader(stream);
reader.read();    // the reader would do something like stream.lock() and stream.release() internally

it seems a little off for the stream to have to know how to dole out readers....and if it didn't, the issue of naming 'getReader' would be a moot point 😄 Take this with a grain of salt as I've just started to read your spec.

Just read this:

A given readable stream only has at most one reader at a time. We say in this case the stream is locked to the reader, and that the reader is active.

A reader also has the capability to release its read lock, which makes it no longer active. At this point another reader can be acquired at will. If the stream becomes closed or errored as a result of the behavior of its underlying source or via cancellation, its reader (if one exists) will automatically release its lock.

Which makes my comment a moot point.... 😄

@domenic
Copy link
Member

domenic commented Apr 3, 2015

Yeah, if you manage to get a ReadableStreamReader constructor that will actually work, as long as there is no existing reader. But we decided to hide that, as an implementation detail. See https://streams.spec.whatwg.org/#reader-constructor

In any case, thanks for your attention to the spec!

@SlexAxton
Copy link

It seems like instead of checking the current landscape for APIs that return the same thing or not when calling get, we should look for APIs that can only be called once, or that throw an error like this if you call them again. It definitely feels like a weird pattern (the mega-thread seems to give good reason for it to be this way, at a skim), which is why naming it is probably a little more difficult.

Any other APIs that we can only call once (even for a time)?

@SlexAxton
Copy link

It feels like getReader would make sense if it returned the reader instance the first time, and then after that, returned a 'frozen' reader or something like that instead of throwing an error. An error might be thrown if you try to act upon that reader, rather than simply 'getting' it.

@sparty02
Copy link

sparty02 commented Apr 3, 2015

@SlexAxton Agreed. Along those lines, it would feel natural for read() to throw when calling it on a stream (if it were there vs the reader) the second time.....back to the difficulty in naming this thing.

@sparty02
Copy link

sparty02 commented Apr 3, 2015

What about "access" as in:

const reader = stream.accessReader();

It seems to convey some level of exclusivity (you can only 'access' this under certain conditions).

@SlexAxton
Copy link

I don't find read to be a confusing name for something that might throw if I called it at the wrong time. It doesn't seem like the same naming problem to me.

Alternatively, it could be more like the APIs that require user permissions, in that the access can be blocked:

stream.requestReader(function(err, reader) {});

But then it gets all async and weird, but "request" feels more right.

@sparty02
Copy link

sparty02 commented Apr 3, 2015

stream.secureReader();
stream.reserveReader();

@sparty02
Copy link

sparty02 commented Apr 3, 2015

👍 for "request"

@SlexAxton
Copy link

request would almost certainly need to be async, though, and this doesn't need to be.

@sparty02
Copy link

sparty02 commented Apr 3, 2015

const reader = stream.viaReader();

....since really the intent seems to be that you are 'proxying' access to the stream via the reader object.

@domenic
Copy link
Member

domenic commented Apr 3, 2015

There seems to be some confusion. You can get another reader if you release the original one first.

@sparty02
Copy link

sparty02 commented Apr 3, 2015

....which seems to somewhat fit into:

const reader = stream.reserveReader();
reader.release();
const reader2 = stream.reserveReader();  // ok
const reader3 = stream.reserveReader(); // throw... reader wasn't released. 

@yutakahirano
Copy link
Member

It feels like getReader would make sense if it returned the reader instance the first time, and then after that, returned a 'frozen' reader or something like that instead of throwing an error. An error might be thrown if you try to act upon that reader, rather than simply 'getting' it.

I don't think throwing an error is unsound for getX methods. XMLHttpRequest.responseText can throw an error, for example (I mean, methods are more likely to throw errors than attribute getters).

@kriskowal
Copy link

Per my habit, I will reason by analogy to established patterns from which to project names.

What happens between when you release a reader and get/acquire/take/retain a new one? I am going to assume for the sake of argument that the stream’s buffer accumulates and no information is lost.

I suspect that Stream and Reader correspond to the same conceptual entity, so there’s API funny-business going on. Stream and Reader both correspond to sources of data that can have only one consumer (unicast). They differ in that the Stream provides no opinions about how to consume it and alternately vends out to a Reader and instead guarantees that it will only vend readability out to a single consumer at a given time.

I say "readable" loosely because of the shared distinction between "Iterable" and "Iterator", "Observable" and "Observer", "Readable" and "Reader". The Verbable has a verb, e.g., [Symbol.verbable]() or simply verb() that returns a Verber. A Verbable can be multicast or unicast, and a Verber is strictly unicast. I feel a slight preference toward favoring multicast for -ables. Acceptable semantics for different kinds of Readable would include: broadcast (multiple consumers see a contiguous portion of the total stream and all concurrent consumers push back on the producer together), multicast (multiple consumers each start from the beginning, so the whole stream must be accumulated/cached or recomputable), unicast (multiple consumers greedily consume non-overlapping portions of the whole stream).

Stream and Reader have a funny relationship. The Stream seeks to ensure unicast consumption by vending to a single Reader, but ultimately a single Reader could be shared by multiple consumers and, in the case of distributing work to multiple consumers, is actually a useful feature.

My interpretation is that Stream exists for possibly two reasons. On one hand, it is a convenient object for providing different consumer methods if readers turn out to be insufficient. On the other, it provides a point where the underlying fetch API can vend readability to user space and revoke the capability if the user gives it back to the system, so it can bypass userland piping. Strikes me that this is superfluous since a Reader might have directly revokable use of its read() method, or its forEach, map, reduce, all, done, or destroy methods if any of those come to be. Many of these methods could take a lock out on the underlying reader, but I suspect that is unnecessary. If the system decides to consume the stream, it can just starve userland.

Conclusion: getReader does not need a name. Have Reader assimilate Stream.

Of course, I probably missed something important.

@SlexAxton
Copy link

That was great.

Have Reader assimilate Stream

I'm likely just dense, but I thought the whole thing was leading up to myReadableStream.read - sounds like you're saying the opposite?

@kriskowal
Copy link

I’m proposing that it is sufficient to have a Reader and that all places where Stream was should just be Reader. The responsibilities previously given to a Stream can be implemented by a Reader without worrying the user. Specifically, the responsibility of guaranteeing that only the system or the user can read from the stream at any given time, can be subsumed by a Reader. From the user perspective, the ability to have multiple asynchronous readers can be a feature, and the browser can choose to starve user readers if the user gives the stream back to the browser.

However, if there were a getReader() method, I would probably have called it Symbol.reader since it is (dubiously) analogous to Symbol.iterator, only in the sense that Iterator implements Symbol.iterator as return this. Down that line of reasoning, read() would be analogous to iterate(), but that collides with the notion that read():Promise<Iteration<Chunk>> is analogous to next():Iteration<Value>.

@yutakahirano
Copy link
Member

I like the current Stream/Reader separation. Sharing a reader might be useful, but it is possible with the current model, and the current model enables us to define streams' behavior precisely. I don't like the idea of leaving behaviors unspecified.

However, if there were a getReader() method, I would probably have called it Symbol.reader since it is (dubiously) analogous to Symbol.iterator, only in the sense that Iterator implements Symbol.iterator as return this. Down that line of reasoning, read() would be analogous to iterate(), but that collides with the notion that read():Promise<Iteration> is analogous to next():Iteration.

Such symbol can be useful when we have an async loop syntax, but we can add it later. ES6 set has Symbol.iterator slot but it also has values() method. Having both is not a bad thing.

@domenic
Copy link
Member

domenic commented Apr 3, 2015

So, I'm really not seeing anything here that's materially better than getReader. (There's a lot of off-topic stuff about re-designing the streams API entirely, but this is not the thread for that.) And the arguments against getReader have not been that strong, and have counter-arguments in most cases. As such, I think it's best to just stick with getReader().

Thanks for your thoughts though @youennf; I hope you felt they were given fair consideration.

@domenic domenic closed this as completed Apr 3, 2015
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

8 participants