Skip to content
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 behavior for highWaterMark === 0 #21690

Closed
wants to merge 1 commit into from

Conversation

lundibundi
Copy link
Member

@lundibundi lundibundi commented Jul 6, 2018

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.

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jul 6, 2018
@mscdex
Copy link
Contributor

mscdex commented Jul 6, 2018

/cc @nodejs/streams

@lundibundi
Copy link
Member Author

ping @mcollina.

@lundibundi
Copy link
Member Author

lundibundi commented Jul 30, 2018

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.

r.push(null);

r.once('readable', common.mustCall());
r.once('end', common.mustCall());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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


r.push(null);

r.once('readable', common.mustCall());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not also just be on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@BridgeAR BridgeAR requested a review from mafintosh July 30, 2018 22:20
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mcollina 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
Copy link
Member

mcollina commented Aug 6, 2018

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

@mafintosh
Copy link
Member

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?

@mcollina 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
Copy link
Member Author

@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 lundibundi changed the title stream: emit empty readable event to lone listener stream: change behavior for highWaterMark === 0 Aug 9, 2018
@lundibundi
Copy link
Member Author

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to adjust this comment.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment will need an update.

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

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
Copy link
Member Author

lundibundi commented Aug 10, 2018

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

@lundibundi lundibundi changed the title stream: change behavior for highWaterMark === 0 stream: fix readable behavior for highWaterMark === 0 Aug 10, 2018
@mcollina mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 10, 2018
@mcollina
Copy link
Member

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

Thanks for taking these issue, they are indeed!

@mafintosh
Copy link
Member

Very nice fix in the end

@BridgeAR 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
Copy link
Member

Landed in fe47b8b

@mcollina mcollina closed this Aug 10, 2018
@mcollina
Copy link
Member

Thanks @lundibundi for tackling this one!

mcollina pushed a commit that referenced this pull request Aug 10, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Aug 11, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants