stream: fix readable behavior for highWaterMark === 0 by lundibundi · Pull Request #21690 · nodejs/node · GitHub

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

Checklist

make -j4 test (UNIX) passes
tests and/or benchmarks are included
I put one test under sequential as it uses process.stdin to emulate the use case and I'm not sure if that is correct.
documentation is changed or added (not sure what to change)
commit message follows commit guidelines

Edit: I've changed the PR title and description according to changes.

/cc @nodejs/streams

ping @mcollina.

CI: https://ci.nodejs.org/job/node-test-pull-request/16075/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1473/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1474/

Well, CI has failed due to my test... I'm not sure how to handle tty stdin on CI, as it works fine locally. It seems to close the stdin even before the setTimeout fires, can someone help? Should I fork a process for this test and only connect stderr to pass the error?
I've also rebased and decreased delay to 1 ms instead of 1000.

When reading the description of the test it seems like these should be .on instead of once?

Typo in the.

If you want to actually test a tty you have to move the test in the corresponding tty test folder. Otherwise it's not detected as try.

@BridgeAR thanks a lot, I've missed that. Should be fixed now.

Should this not also just be on?

Thanks, idk why did I think it was okay to put once on both of them. 🤔

CI: https://ci.nodejs.org/job/node-test-pull-request/16213/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1481/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1482/

code LGTM. Can land if citgm does not report additional breakage.

Seems good, but CITGM is currently not very healthy: nodejs/build#1429.

I'm not I understand why this is needed for TTYs to work? Can someone clarify this for me? The unneeded readables was breaking of code for me, and as I understand this, it restores that behaivor?

@mafintosh After your comment, I went to double check and found a better way of tackling this problem.

But I also discovered the other thing. Your PR is indeed fixing the issue of empty-readable but the thing is, it suppresses the issue by not emitting the event but there are still multiple calls to emitReadable_ present. I tried to actually resolve the issue but to no avail, unfortunately. Here is the description, maybe someone can figure it out.

The thing is that with the write pattern as in test/parallel/test-stream-readable-no-unneeded-readable.js triggers 'readable' event upon push (as it should) but due to the fact that it only arrives on the nextTick that .once('readable') manages to catch old readable event (because we have already read all of the data of that event in the wrapper) that is now empty and results in empty (and multiple) 'readable' events.
If we look at the debug log (I added a few log statements) we can see that actually after last push (before push(null))

STREAM 15878: do read
wrapper read called
STREAM 15878: read undefined
STREAM 15878: need readable true
STREAM 15878: length less than watermark true
STREAM 15878: do read
wrapper read called
STREAM 15878: read undefined
STREAM 15878: need readable true
STREAM 15878: length less than watermark true
STREAM 15878: do read
STREAM 15878: on readable 0 true
STREAM 15878: read undefined
STREAM 15878: need readable true
STREAM 15878: length less than watermark true
STREAM 15878: reading or ended false
STREAM 15878: emit readable rStream
STREAM 15878: flow null
STREAM 15878: emit readable wrapperStream
STREAM 15878: flow true
STREAM 15878: read undefined
STREAM 15878: need readable false
STREAM 15878: length less than watermark true
STREAM 15878: reading or ended false
STREAM 15878: emit readable rStream
STREAM 15878: flow null
STREAM 15878: emit readable wrapperStream
STREAM 15878: flow true
STREAM 15878: read undefined
STREAM 15878: need readable false
STREAM 15878: length less than watermark true
STREAM 15878: reading or ended false
STREAM 15878: readableAddChunk null
STREAM 15878: emit readable rStream
readable called
STREAM 15878: read undefined
STREAM 15878: endReadable false
STREAM 15878: flow null
STREAM 15878: endReadableNT false 0
STREAM 15878: readableAddChunk null
STREAM 15878: emit readable wrapperStream
STREAM 15878: flow true
STREAM 15878: read undefined
STREAM 15878: endReadable false
STREAM 15878: endReadableNT false 0
STREAM 15878: do read
STREAM 15878: readableAddChunk <Buffer 62 61 72>
STREAM 15878: emitReadable null
STREAM 15878: readableAddChunk <Buffer 62 61 72>
STREAM 15878: emitReadable true
STREAM 15878: read undefined
STREAM 15878: need readable false
STREAM 15878: length less than watermark true
STREAM 15878: do read
wrapper read called
STREAM 15878: read undefined
STREAM 15878: need readable true
STREAM 15878: length less than watermark true
STREAM 15878: do read
STREAM 15878: on readable 0 true
STREAM 15878: read undefined
STREAM 15878: need readable true
STREAM 15878: length less than watermark true
STREAM 15878: reading or ended false
STREAM 15878: emit readable rStream
STREAM 15878: flow null
STREAM 15878: emit readable wrapperStream
STREAM 15878: flow true
STREAM 15878: read undefined
STREAM 15878: need readable false
STREAM 15878: length less than watermark true
STREAM 15878: reading or ended false
STREAM 15878: emit readable rStream
STREAM 15878: flow null
STREAM 15878: emit readable wrapperStream
STREAM 15878: flow true
STREAM 15878: read undefined
STREAM 15878: need readable false
STREAM 15878: length less than watermark true
STREAM 15878: reading or ended false
STREAM 15878: readableAddChunk null
STREAM 15878: emit readable rStream
readable called
STREAM 15878: read undefined
STREAM 15878: endReadable false
STREAM 15878: flow null
STREAM 15878: endReadableNT false 0
STREAM 15878: readableAddChunk null
STREAM 15878: emit readable wrapperStream
STREAM 15878: flow true
STREAM 15878: read undefined
STREAM 15878: endReadable false
STREAM 15878: endReadableNT false 0

there are 2 more 'emit readable rStream' that were suppressed and I think at least the second one shouldn't have happened at all (the first one is from the latest data push).
Though I may be misunderstanding something and this second read/emit is actually needed?

@mcollina @mafintosh @BridgeAR I'd like another review as I changed this PR's idea.

Can you add some tests that shows the changed behavior in streams? I'd like to see a a test that failed in current master but passes here.

I think we would need to add some docs for highWaterMark: 0. There aren't any atm.

(Using the big red cross because I approved it in the past)

We'll need to adjust this comment.

this comment will need an update.

It's not really clear what it is changing here. Can you describe it the change to streams a bit more?

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

@mcollina cleaned up the PR (and removed redundant checks) and added relevant test without the TTY. Yeah, this shouldn't change anything, it's a bug fix, bad wording, sorry. I've updated the description.
Also, test/parallel/test-readable-single-end.js can be dropped but I see no harm in having more tests.

P.s. Streams sure are tough, thanks for bearing with me.

CI: https://ci.nodejs.org/job/node-test-pull-request/16336/

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1497/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1498/

P.s. Streams sure are tough, thanks for bearing with me.

Thanks for taking these issue, they are indeed!

Very nice fix in the end

Landed in fe47b8b

Thanks @lundibundi for tackling this one!

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: nodejs/node#20503 Refs: nodejs/node#18372 PR-URL: nodejs/node#21690 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jul 6, 2018

lundibundi force-pushed the fix-stream-stdin branch from ab7ddcd to d14752c Compare July 6, 2018 21:09

addaleax requested a review from mcollina July 10, 2018 22:37

lundibundi force-pushed the fix-stream-stdin branch from d14752c to 4df321c Compare July 30, 2018 17:45

BridgeAR reviewed Jul 30, 2018

View reviewed changes

BridgeAR requested a review from mafintosh July 30, 2018 22:20

mcollina approved these changes Aug 6, 2018

View reviewed changes

mcollina added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 6, 2018

mcollina removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 8, 2018

lundibundi force-pushed the fix-stream-stdin branch from ce3551e to e874f79 Compare August 9, 2018 15:18

lundibundi changed the title stream: emit empty readable event to lone listener stream: change behavior for highWaterMark === 0 Aug 9, 2018

mcollina requested changes Aug 10, 2018

View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 10, 2018

lundibundi force-pushed the fix-stream-stdin branch from e874f79 to e5a6527 Compare August 10, 2018 10:56

lundibundi changed the title stream: change behavior for highWaterMark === 0 stream: fix readable behavior for highWaterMark === 0 Aug 10, 2018

mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 10, 2018

mcollina approved these changes Aug 10, 2018

View reviewed changes

mafintosh approved these changes Aug 10, 2018

View reviewed changes

BridgeAR approved these changes Aug 10, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 10, 2018

mcollina closed this Aug 10, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

ZYSzys mentioned this pull request Dec 21, 2018

readable event,outputs are different between v9.11 and v10.14.2 #25158

Closed

stream: fix readable behavior for highWaterMark === 0 by lundibundi · Pull Request #21690 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: fix readable behavior for highWaterMark === 0 - #21690

stream: fix readable behavior for highWaterMark === 0#21690lundibundi wants to merge 1 commit intonodejs:masternodejs/node:masterfrom lundibundi:fix-stream-stdinlundibundi/node:fix-stream-stdinCopy head branch name to clipboard

Conversation

lundibundi commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mscdex commented Jul 6, 2018

Uh oh!

lundibundi commented Jul 19, 2018

Uh oh!

mcollina commented Jul 30, 2018

Uh oh!

lundibundi commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Jul 30, 2018

Uh oh!

BridgeAR Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

lundibundi Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 6, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 6, 2018

Uh oh!

mafintosh commented Aug 6, 2018

Uh oh!

lundibundi commented Aug 9, 2018

Uh oh!

lundibundi commented Aug 9, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 10, 2018

Uh oh!

lundibundi commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Aug 10, 2018

Uh oh!

mcollina commented Aug 10, 2018

Uh oh!

mafintosh commented Aug 10, 2018

Uh oh!

mcollina commented Aug 10, 2018

Uh oh!

mcollina commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

stream: fix readable behavior for highWaterMark === 0#21690
lundibundi wants to merge 1 commit into
nodejs:masterfrom
lundibundi:fix-stream-stdin

lundibundi commented Jul 6, 2018 •
edited
Loading

lundibundi commented Jul 30, 2018 •
edited
Loading

lundibundi commented Aug 10, 2018 •
edited
Loading