-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Readable._read is called multiple times synchronously #24919
Labels
stream
Issues and PRs related to the stream subsystem.
Comments
/cc @nodejs/streams |
mcollina
added a commit
to mcollina/node
that referenced
this issue
Jan 11, 2019
nodejs#17979 introduced a change in the doc that was not correct about _read always being called asynchronously. This does not hold true when it is in flowing mode. See: nodejs#17979 Fixes: nodejs#24919
3 tasks
Fixed in 2f1ae9e. |
mcollina
added a commit
that referenced
this issue
Jan 15, 2019
#17979 introduced a change in the doc that was not correct about _read always being called asynchronously. This does not hold true when it is in flowing mode. See: #17979 Fixes: #24919 PR-URL: #25442 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
addaleax
pushed a commit
that referenced
this issue
Jan 15, 2019
#17979 introduced a change in the doc that was not correct about _read always being called asynchronously. This does not hold true when it is in flowing mode. See: #17979 Fixes: #24919 PR-URL: #25442 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this issue
Jan 16, 2019
nodejs#17979 introduced a change in the doc that was not correct about _read always being called asynchronously. This does not hold true when it is in flowing mode. See: nodejs#17979 Fixes: nodejs#24919 PR-URL: nodejs#25442 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The documentation for
readable._read(size)
states the following:However the following example demonstrates starvation if that method is able to push all of its data synchronously. The stream implementation ends up calling the read-function repeatedly and won't yield execution as documented.
I believe the issue is caused by the
flow
function callingstream.read
repeatedly within a while-loop until the stream fails to return data:node/lib/_stream_readable.js
Lines 933 to 937 in 951b1c3
One option for fixing this might be to make
flow
recurse with next tick:Alternatively just updating the documentation might also work. I guess most streams that would encounter the issue would just delay
r.push()
into the next tick as a workaround.Even in my case this was just something I spotted while looking into #24918. I don't personally have a use case for such a stream.
The text was updated successfully, but these errors were encountered: