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

lundibundi · 2018-07-06T17:33:35Z

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.

mscdex · 2018-07-06T20:03:45Z

/cc @nodejs/streams

lundibundi · 2018-07-19T20:44:14Z

ping @mcollina.

mcollina · 2018-07-30T14:42:14Z

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/

lundibundi · 2018-07-30T17:47:44Z

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.

BridgeAR · 2018-07-30T18:52:13Z

test/parallel/test-readable-single-end.js

+r.push(null);
+
+r.once('readable', common.mustCall());
+r.once('end', common.mustCall());


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

BridgeAR · 2018-07-30T18:52:59Z

test/sequential/test-readable-tty-keepalive.js

+const assert = require('assert');
+
+// This test ensures that Node.js will not ignore tty 'readable' subscribers
+// when it's the only tty subscriber and te only thing keeping event loop alive


Typo in the.

BridgeAR

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.

lundibundi · 2018-07-30T21:06:29Z

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

BridgeAR · 2018-07-30T21:13:32Z

test/parallel/test-readable-single-end.js

+
+r.push(null);
+
+r.once('readable', common.mustCall());


Should this not also just be on?

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

mcollina · 2018-08-06T09:47:17Z

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/

mcollina

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

mcollina · 2018-08-06T14:04:05Z

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

mafintosh · 2018-08-06T22:07:56Z

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?

lundibundi · 2018-08-09T15:13:34Z

@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?

lundibundi · 2018-08-09T18:16:49Z

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

mcollina

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)

mcollina · 2018-08-10T07:22:26Z

lib/_stream_readable.js

@@ -274,7 +274,9 @@ function readableAddChunk(stream, chunk, encoding, addToFront, skipChunkCheck) {
  // Also, if we have no data yet, we can stand some more bytes.
  // This is to work around cases where hwm=0, such as the repl.


We'll need to adjust this comment.

mcollina · 2018-08-10T07:23:34Z

lib/_stream_readable.js

@@ -383,7 +385,8 @@ Readable.prototype.read = function(n) {
  // the 'readable' event and move on.


this comment will need an update.

mcollina · 2018-08-10T07:26:25Z

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

lundibundi · 2018-08-10T10:59:57Z

@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.

mcollina · 2018-08-10T11:05:13Z

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/

mcollina · 2018-08-10T13:25:38Z

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

Thanks for taking these issue, they are indeed!

mafintosh · 2018-08-10T14:14:33Z

Very nice fix in the end

mcollina · 2018-08-10T15:17:36Z

Landed in fe47b8b

mcollina · 2018-08-10T15:18:02Z

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>

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 pending requests for changes, and a CI started. label Aug 6, 2018

mcollina removed the author ready PRs that have at least one approval, no pending requests for changes, 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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lundibundi commented Jul 6, 2018 •

edited

mscdex commented Jul 6, 2018

lundibundi commented Jul 19, 2018

mcollina commented Jul 30, 2018

lundibundi commented Jul 30, 2018 •

edited

BridgeAR Jul 30, 2018

BridgeAR Jul 30, 2018

BridgeAR left a comment

lundibundi commented Jul 30, 2018

BridgeAR Jul 30, 2018

lundibundi Jul 30, 2018

mcollina commented Aug 6, 2018

mcollina left a comment

mcollina commented Aug 6, 2018

mafintosh commented Aug 6, 2018

lundibundi commented Aug 9, 2018

lundibundi commented Aug 9, 2018

mcollina left a comment

mcollina Aug 10, 2018

mcollina Aug 10, 2018

mcollina commented Aug 10, 2018

lundibundi commented Aug 10, 2018 •

edited

mcollina commented Aug 10, 2018

mcollina commented Aug 10, 2018

mafintosh commented Aug 10, 2018

mcollina commented Aug 10, 2018

mcollina commented Aug 10, 2018

		@@ -274,7 +274,9 @@ function readableAddChunk(stream, chunk, encoding, addToFront, skipChunkCheck) {
		// Also, if we have no data yet, we can stand some more bytes.
		// This is to work around cases where hwm=0, such as the repl.

		@@ -383,7 +385,8 @@ Readable.prototype.read = function(n) {
		// the 'readable' event and move on.


		r.push(null);

		r.once('readable', common.mustCall());

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

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

Conversation

lundibundi commented Jul 6, 2018 • edited

Checklist

mscdex commented Jul 6, 2018

lundibundi commented Jul 19, 2018

mcollina commented Jul 30, 2018

lundibundi commented Jul 30, 2018 • edited

BridgeAR Jul 30, 2018

Choose a reason for hiding this comment

BridgeAR Jul 30, 2018

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

lundibundi commented Jul 30, 2018

BridgeAR Jul 30, 2018

Choose a reason for hiding this comment

lundibundi Jul 30, 2018

Choose a reason for hiding this comment

mcollina commented Aug 6, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Aug 6, 2018

mafintosh commented Aug 6, 2018

lundibundi commented Aug 9, 2018

lundibundi commented Aug 9, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina Aug 10, 2018

Choose a reason for hiding this comment

mcollina Aug 10, 2018

Choose a reason for hiding this comment

mcollina commented Aug 10, 2018

lundibundi commented Aug 10, 2018 • edited

mcollina commented Aug 10, 2018

mcollina commented Aug 10, 2018

mafintosh commented Aug 10, 2018

mcollina commented Aug 10, 2018

mcollina commented Aug 10, 2018

lundibundi commented Jul 6, 2018 •

edited

lundibundi commented Jul 30, 2018 •

edited

lundibundi commented Aug 10, 2018 •

edited