-
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
stream: fix readable being emitted when nothing to do #18372
Conversation
/cc @mcollina @nodejs/streams |
3bb3f96
to
613b68e
Compare
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1239/ (before landing) |
CITGM seems similar to master, landing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed as 0778f79. |
The original test case hides the underlying cause by using `PassThrough`. This change adds a test case for the underlying cause. This makes it clearer and easier to be understood. Refs: nodejs#18372
Fixes a regression introduced by the once-per-microtick 'readable' event emission. See: nodejs#17979 PR-URL: nodejs#18372 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The original test case hides the underlying cause by using `PassThrough`. This change adds a test case for the underlying cause. This makes it clearer and easier to be understood. Refs: nodejs#18372 PR-URL: nodejs#18575 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Avoid trying to emit 'readable' due to the fact that state.length is always >= state.highWaterMark if highWaterMark is 0. Therefore upon .read(0) call (through .on('readable')) stream assumed that it has enough data to emit 'readable' even though state.length === 0 instead of issuing _read(). Which led to the TTY not recognizing that someone is waiting for the input. Fixes: nodejs#20503 Refs: nodejs#18372
Avoid trying to emit 'readable' due to the fact that state.length is always >= state.highWaterMark if highWaterMark is 0. Therefore upon .read(0) call (through .on('readable')) stream assumed that it has enough data to emit 'readable' even though state.length === 0 instead of issuing _read(). Which led to the TTY not recognizing that someone is waiting for the input. Fixes: #20503 Refs: #18372 PR-URL: #21690 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Avoid trying to emit 'readable' due to the fact that state.length is always >= state.highWaterMark if highWaterMark is 0. Therefore upon .read(0) call (through .on('readable')) stream assumed that it has enough data to emit 'readable' even though state.length === 0 instead of issuing _read(). Which led to the TTY not recognizing that someone is waiting for the input. Fixes: #20503 Refs: #18372 PR-URL: #21690 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
stream
This fixes an issue introduced by #17979 where the readable event are sometimes emitted when the stream is neither readable nor has it ended.