Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Update docs: read() required before readable event#4720

Closed
wanderview wants to merge 1 commit intonodejs:masterfrom
wanderview:readable-event-doc-fix
Closed

Update docs: read() required before readable event#4720
wanderview wants to merge 1 commit intonodejs:masterfrom
wanderview:readable-event-doc-fix

Conversation

@wanderview
Copy link
Copy Markdown

This is one possible fix for #4695.

@indutny
Copy link
Copy Markdown
Member

indutny commented Feb 7, 2013

@isaacs is it really the way it should be documented? Feels really odd to me.

@wanderview
Copy link
Copy Markdown
Author

I agree its a bit clunky, but that seems how it operates now. @TooTallNate indicated in #4695 that there is a performance issue with an implicit _read() when the stream is constructed.

Perhaps we could default to the more intuitive behavior where the first readable event does not require a read(), but provide a tunable constructor option to disable this behavior. Something like:

var r = new Readable({explicitRead: true});

@indutny
Copy link
Copy Markdown
Member

indutny commented Feb 7, 2013

I guess there is another more complex design problem behind this. If you'll miss 'readable' event - you won't know that it has happened. And this is a situation that was always happening with streams before streams2, so you should explicitly start reading from stream to get first readable event.

@indutny
Copy link
Copy Markdown
Member

indutny commented Feb 7, 2013

And, considering this, this pull request LGTM.

@wanderview
Copy link
Copy Markdown
Author

As of v0.9.11, my original test no longer fails. At least for streams that has push() called asynchronously, you no longer need to call read(0) to kick them off. Closing since these doc changes are no longer relevant.

@wanderview wanderview closed this Mar 2, 2013
isaacs added a commit to isaacs/node-v0.x-archive that referenced this pull request Mar 3, 2013
When a readable listener is added, call read(0) so that data will flow in, up to
the high water mark.

Otherwise, it's somewhat confusing that you have to listen for readable,
and ALSO call read() (when it will certainly return null) just to get some
data out of the stream.

See: nodejs#4720
isaacs added a commit that referenced this pull request Mar 4, 2013
When a readable listener is added, call read(0) so that data will flow in, up to
the high water mark.

Otherwise, it's somewhat confusing that you have to listen for readable,
and ALSO call read() (when it will certainly return null) just to get some
data out of the stream.

See: #4720
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants