★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21690Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@mcollina mcollina

@mafintosh mafintosh

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
author ready stream
Projects
None yet
Milestone
No milestone
6 participants
@lundibundi @mscdex @mcollina @mafintosh @BridgeAR @nodejs-github-bot
@lundibundi
Copy link
Member

lundibundi commented Jul 6, 2018 •
edited

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 label Jul 6, 2018

@mscdex

This comment has been minimized.

Sign in to view
Copy link
Contributor

mscdex commented Jul 6, 2018

/cc @nodejs/streams

@lundibundi lundibundi force-pushed the lundibundi:fix-stream-stdin branch from ab7ddcd to d14752c Jul 6, 2018

@addaleax addaleax requested a review from mcollina Jul 10, 2018

@lundibundi

This comment has been minimized.

Sign in to view
Copy link
Member Author

lundibundi commented Jul 19, 2018

ping @mcollina.
@mcollina

@lundibundi lundibundi force-pushed the lundibundi:fix-stream-stdin branch from d14752c to 4df321c Jul 30, 2018

@lundibundi

This comment has been minimized.

Sign in to view
Copy link
Member Author

lundibundi commented Jul 30, 2018 •
edited

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

BridgeAR reviewed Jul 30, 2018
View changes

test/parallel/test-readable-single-end.js Outdated
r.push(null);

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

This comment has been minimized.

Sign in to view
Copy link
@BridgeAR

BridgeAR Jul 30, 2018

Member

When reading the description of the test it seems like these should be .on instead of once?
test/sequential/test-readable-tty-keepalive.js Outdated
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

This comment has been minimized.

Sign in to view
Copy link
@BridgeAR

BridgeAR Jul 30, 2018

Member

Typo in the.
@BridgeAR

BridgeAR reviewed Jul 30, 2018
View changes

Copy link
Member

BridgeAR left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

lundibundi commented Jul 30, 2018

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

BridgeAR reviewed Jul 30, 2018
View changes

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

r.push(null);

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

This comment has been minimized.

Sign in to view
Copy link
@BridgeAR

BridgeAR Jul 30, 2018

Member

Should this not also just be on?

This comment has been minimized.

Sign in to view
Copy link
@lundibundi

lundibundi Jul 30, 2018

Author Member

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

@BridgeAR BridgeAR requested a review from mafintosh Jul 30, 2018

@mcollina
@mcollina

mcollina approved these changes Aug 6, 2018
View changes

Copy link
Member

mcollina left a comment

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

@mcollina mcollina added the author ready label Aug 6, 2018

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Aug 6, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

mafintosh commented Aug 6, 2018

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 label Aug 8, 2018

@lundibundi

This comment has been minimized.

Sign in to view
Copy link
Member Author

lundibundi commented Aug 9, 2018

@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 force-pushed the lundibundi:fix-stream-stdin branch from ce3551e to e874f79 Aug 9, 2018

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

@lundibundi

This comment has been minimized.

Sign in to view
Copy link
Member Author

lundibundi commented Aug 9, 2018

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

mcollina requested changes Aug 10, 2018
View changes

Copy link
Member

mcollina left a comment

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)
lib/_stream_readable.js Outdated
@@ -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.

This comment has been minimized.

Sign in to view
Copy link
@mcollina

mcollina Aug 10, 2018

Member

We'll need to adjust this comment.
lib/_stream_readable.js
@@ -383,7 +385,8 @@ Readable.prototype.read = function(n) {
// the 'readable' event and move on.

This comment has been minimized.

Sign in to view
Copy link
@mcollina

mcollina Aug 10, 2018

Member

this comment will need an update.

@mcollina mcollina added the semver-major label Aug 10, 2018

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Aug 10, 2018

It's not really clear what it is changing here. Can you describe it the change to streams a bit more?
@lundibundi
stream: fix readable behavior for highWaterMark === 0 
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
Verified
This commit was signed with a verified signature.
@lundibundi lundibundi Denys Otrishko
GPG key ID: B78E64B755B79383 Learn about signing commits
Loading status checks…
e5a6527

@lundibundi lundibundi force-pushed the lundibundi:fix-stream-stdin branch from e874f79 to e5a6527 Aug 10, 2018

@lundibundi

This comment has been minimized.

Sign in to view
Copy link
Member Author

lundibundi commented Aug 10, 2018 •
edited

@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 label Aug 10, 2018

@mcollina
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Aug 10, 2018

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

Thanks for taking these issue, they are indeed!
@mcollina

mcollina approved these changes Aug 10, 2018
View changes

@mafintosh

This comment has been minimized.

Sign in to view
Copy link
Member

mafintosh commented Aug 10, 2018

Very nice fix in the end
@mafintosh

mafintosh approved these changes Aug 10, 2018
View changes

@BridgeAR

BridgeAR approved these changes Aug 10, 2018
View changes

@BridgeAR BridgeAR added the author ready label Aug 10, 2018

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Aug 10, 2018

Landed in fe47b8b

@mcollina mcollina closed this Aug 10, 2018

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Aug 10, 2018

Thanks @lundibundi for tackling this one!

mcollina added a commit that referenced this pull request Aug 10, 2018

@lundibundi @mcollina
stream: fix readable behavior for highWaterMark === 0 
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>
Loading status checks…
fe47b8b

targos added a commit that referenced this pull request Aug 11, 2018

@lundibundi @targos
stream: fix readable behavior for highWaterMark === 0 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
3e7ffd4

targos added a commit that referenced this pull request Aug 11, 2018

@lundibundi @targos
stream: fix readable behavior for highWaterMark === 0 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
1c61205

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

This was referenced Aug 16, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed

@ZYSzys ZYSzys referenced this pull request Dec 21, 2018

Open

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.