stream: flow for 'data' listeners upon removal of last 'readable' listener #21696

lundibundi · 2018-07-07T00:05:24Z

When there is at least one 'data' listener try to flow when
last 'readable' listener gets removed and the stream is not piped.

Currently if we have both 'readable' and 'data' listeners set only
'readable' listener will get called and stream will be stalled without
'data' and 'end' events if 'readable' listener neither read()'s nor
resume()'s the stream.

Fixes: #21398

Checklist

make -j4 test (UNIX) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mscdex · 2018-07-07T00:25:42Z

/cc @nodejs/streams

lundibundi · 2018-07-19T20:43:48Z

ping @mcollina @mafintosh.

mcollina · 2018-07-30T14:43:40Z

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

lundibundi · 2018-07-30T16:27:59Z

@mcollina CI has a seemingly not related failure, can we rerun or am I misunderstanding something? (I also tried --repeat 192 on my linux for those 3 tests and no failures).

CITGM has a lot of build failures, I'm not sure how to interpret the results.

Also, I've rebased on master just in case.

mcollina · 2018-08-06T09:44:40Z

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

mcollina

LGTM

mcollina · 2018-08-06T14:06:53Z

Resume Ci: https://ci.nodejs.org/job/node-test-pull-request/16216

jasnell · 2018-08-06T14:13:14Z

doc/api/stream.md

-only when [`stream.read()`][stream-read] is called.
+only when [`stream.read()`][stream-read] is called. But when last `'readable'`
+was removed stream will try to [`stream.resume()`][stream-resume] to fulfill
+`'data'` listeners. Therefore you can use it like this (bar.txt has 'abc\n'


Please avoid using you in the docs. This last sentence can just be dropped.

lundibundi · 2018-08-06T14:34:53Z

@jasnell done. Though we will probably have to wait until CITGM machines are working properly again.

mcollina · 2018-08-06T14:38:45Z

I think landing this without a CITGM run is ok.

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

mafintosh · 2018-08-06T22:10:42Z

So this basically restores the "magic" behaivor of the data event? Guess that makes sense, LGTM

mafintosh · 2018-08-06T22:13:11Z

lib/_stream_readable.js

+  if (!state.readableListening &&
+      self.listenerCount('data') > 0 &&
+      state.pipesCount === 0) {
+    self.resume();


what if the stream was explicitly paused before? Something like:

stream.pause() stream.on('data', ...) stream.once('readable', ...)

Seems bad that this would auto resume that, if that is the case

@lundibundi can you please add a unit test in that case?

Ok, I've added a test and now spotted a problem. After @mcollina PR to make readable take precedence over data there is no way now to determine whether it was user who called pause or we via (this.resume() in on('data') handler) because we use cf5f986#diff-ba6a0df0f5212f5cba5ca5179e209a17R878 and now state.flowing is false if we add 'data' listener after 'readable' listener even though user didn't explicitly pause the stream.

I'm not sure what's the correct solution here. If I add this.isPaused check in here it will only work if a user called it like this

r.on('data', (chunk) => receivedData += chunk); r.once('readable', common.mustCall());

and won't work for

r.once('readable', common.mustCall()); r.on('data', (chunk) => receivedData += chunk);

which is extremely bizarre.

I'm thinking of not setting flowing at all until we have 0 'readable' listeners at which point check for data listeners and then set it to true/false (resume/pause).

I'm thinking of not setting flowing at all until we have 0 'readable' listeners at which point check for data listeners and then set it to true/false (resume/pause).

I think that would work.

addaleax · 2018-08-06T23:26:08Z

doc/api/stream.md

+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/21696
+    description: >
+      When using both `'readable'` and [`'data'`][] stream will try to


typo: the stream

addaleax · 2018-08-06T23:26:20Z

doc/api/stream.md

@@ -825,7 +831,33 @@ result in increased throughput.

 If both `'readable'` and [`'data'`][]  are used at the same time, `'readable'`
 takes precedence in controlling the flow, i.e. `'data'` will be emitted
-only when [`stream.read()`][stream-read] is called.
+only when [`stream.read()`][stream-read] is called. But when last `'readable'`
+was removed stream will try to [`stream.resume()`][stream-resume] to fulfill


typo: the stream

addaleax · 2018-08-06T23:28:01Z

doc/api/stream.md

+  console.log('readable');
+});
+rr.on('data', (chunk) => {
+  console.log(chunk.toString());


I think we might want to add rr.setEncoding('utf8'), or pass the encoding: 'utf8' option to createReadStream rather than using .toString(), to encourage best practices

(The current code could fail if bar.txt contains UTF-8 characters outside the ASCII range)

addaleax · 2018-08-06T23:31:34Z

lib/_stream_readable.js

+  // (if pipesCount is not 0 then we have already 'flowed')
+  if (!state.readableListening &&
+      self.listenerCount('data') > 0 &&
+      state.pipesCount === 0) {


Do we have a test covering what happens if the last readable listener is removed but there is a pipe?

addaleax · 2018-08-06T23:32:33Z

I’d prefer a CITGM run, btw.

mcollina · 2018-08-07T07:28:02Z

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

Issue fixed :-)

mcollina · 2018-08-08T15:58:00Z

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

lundibundi · 2018-08-09T08:10:22Z

PTAL #21696 (comment)

Also, currently if we have 'readable' listener on a Readable stream, if we pipe it to other stream nothing will happen, try this (uncomment/comment 'readable' listener):

const data = ['foo', 'bar', 'baz'];
const r = new Readable({
  read: () => {},
});

// r.on('readable', () => {});

r.pipe(process.stdout);
r.push(data[0]);
r.push(data[1]);
r.push(data[2]);
r.push(null);

mcollina · 2018-08-09T08:21:34Z

#21696 (comment) is as documented.

It needs a little bit more work.

lundibundi · 2018-08-09T08:42:37Z

@mcollina Yeah we have that in the documentation of pause, 'readable' and 'data'. But in .pipe documentation it still says that

The readable.pipe() method attaches a Writable stream to the readable, causing it to switch automatically into flowing mode and push all of its data to the attached Writable

Therefore we should either change the doc or the code. And imo the latter will be better as we will restore the ability to tell if user manually called .pause and will not confuse it with us calling this.resume when there are >0 'readable' listeners. Furthermore, it should simplify Streams for users at least a bit.

I have a fix (the one I mentioned here #21696 (comment)) but for some reason test-tls-server-verify fails with 'AssertionError [ERR_ASSERTION]: 5 2 agent3 rejected, but should NOT have been' (and also test-stream-readable-reading-readingMore but it tests that flowing is false explicitly) and I'm trying to understand why.

mcollina · 2018-08-09T08:49:14Z

Yeah we have that in the documentation of pause, 'readable' and 'data'. But in .pipe documentation it still says that

The docs would need changing, as .pipe() relies on on('data').

test-stream-readable-reading-readingMore might need to be changed as a result of this as it's really picky about the internals.

I'm not sure about test-tls-server-verify.

lundibundi · 2018-08-09T09:00:10Z

@mcollina But then the problem with isPaused I mentioned in #21696 (comment) arises. So either we will always flow after last 'readable' was removed and ignore paused state or we will have that bizarre behavior I mentioned.

Also, .pipe() manually calls .resume so it doesn't use .on('data') property of flowing the stream.

Maybe it will be better for me to push (my fix that also restores pipe) so you could take a look?
Edit: I've pushed the changes PTAL. Also for some reason after rebase on master test-tls-server-verify no longer fails. The other one still fails, I'll fix it soon.

When there is at least one 'data' listener try to flow when last 'readable' listener gets removed and the stream is not piped. Currently if we have both 'readable' and 'data' listeners set only 'readable' listener will get called and stream will be stalled without 'data' and 'end' events if 'readable' listener neither read()'s nor resume()'s the stream. Fixes: nodejs#21398

Now state.flowing will be set only after all of the 'readable' listeners are gone and if we have at least one 'data' listener. * on('data') will not flow (flowing === null and not false) if there are 'readable' listeners * pipe() will work regardless of 'readable' listeners * isPause reports only user .pause call (before setting 'data' listener when there is already 'readable' listener also set flowing to false) * resume always sets stream to flowing state

mcollina · 2018-08-09T14:38:42Z

I've added an alternative implementation of the fix: see #22209.

Fixes: nodejs#21398 See: nodejs#21696

Fixes: nodejs#21398 See: nodejs#21696 PR-URL: nodejs#22209 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>

mcollina · 2018-08-22T07:46:14Z

Fixed in 98cf84f.

Fixes: #21398 See: #21696 PR-URL: #22209 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>

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

jasnell requested review from mcollina and mafintosh July 10, 2018 20:02

lundibundi force-pushed the fix-stream-no-data branch from 84c2cfa to 945efbe Compare July 30, 2018 16:23

lundibundi mentioned this pull request Jul 30, 2018

ReadStream paused and can't resume, when listened readable event #22032

Closed

mcollina previously 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 changed the title ~~stream: flow for 'data' listeners upon removeal of last 'readable' listener~~ stream: flow for 'data' listeners upon removal of last 'readable' listener Aug 6, 2018

jasnell previously requested changes Aug 6, 2018

View reviewed changes

lundibundi force-pushed the fix-stream-no-data branch from 945efbe to 95c8e7b Compare August 6, 2018 14:33

mafintosh reviewed Aug 6, 2018

View reviewed changes

addaleax approved these changes Aug 6, 2018

View reviewed changes

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 added 2 commits August 9, 2018 12:21

lundibundi force-pushed the fix-stream-no-data branch from 95c8e7b to c07abd7 Compare August 9, 2018 09:25

fixup! stream: postpone setting flowing for on('readable')

d699f48

mcollina mentioned this pull request Aug 9, 2018

stream: restore flow if there are 'data' handlers after once('readable') #22209

Closed

4 tasks

mcollina added a commit to mcollina/node that referenced this pull request Aug 21, 2018

stream: restore flow if there are 'data' handlers after once('readable')

861124a

Fixes: nodejs#21398 See: nodejs#21696

mcollina closed this Aug 22, 2018

targos pushed a commit that referenced this pull request Aug 24, 2018

stream: restore flow if there are 'data' handlers after once('readable')

a431a15

Fixes: #21398 See: #21696 PR-URL: #22209 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>

targos pushed a commit that referenced this pull request Sep 3, 2018

stream: restore flow if there are 'data' handlers after once('readable')

53fb7af

Fixes: #21398 See: #21696 PR-URL: #22209 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>

snakamura mentioned this pull request Nov 10, 2018

stream.resume() doesn't make the stream flowing after removing readable listener #24281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: flow for 'data' listeners upon removal of last 'readable' listener #21696

stream: flow for 'data' listeners upon removal of last 'readable' listener #21696

lundibundi commented Jul 7, 2018 •

edited

Loading

mscdex commented Jul 7, 2018

lundibundi commented Jul 19, 2018

mcollina commented Jul 30, 2018

lundibundi commented Jul 30, 2018

mcollina commented Aug 6, 2018

mcollina left a comment

mcollina commented Aug 6, 2018 •

edited

Loading

jasnell Aug 6, 2018

lundibundi commented Aug 6, 2018

mcollina commented Aug 6, 2018

mafintosh commented Aug 6, 2018

mafintosh Aug 6, 2018 •

edited

Loading

mcollina Aug 7, 2018

lundibundi Aug 8, 2018

mcollina Aug 9, 2018

addaleax Aug 6, 2018

addaleax Aug 6, 2018

lundibundi Aug 8, 2018

addaleax Aug 6, 2018

addaleax Aug 6, 2018

lundibundi Aug 9, 2018

addaleax commented Aug 6, 2018

mcollina commented Aug 7, 2018

mcollina commented Aug 8, 2018

lundibundi commented Aug 9, 2018

mcollina commented Aug 9, 2018

lundibundi commented Aug 9, 2018 •

edited

Loading

mcollina commented Aug 9, 2018

lundibundi commented Aug 9, 2018 •

edited

Loading

mcollina commented Aug 9, 2018

mcollina commented Aug 22, 2018

stream: flow for 'data' listeners upon removal of last 'readable' listener #21696

stream: flow for 'data' listeners upon removal of last 'readable' listener #21696

Conversation

lundibundi commented Jul 7, 2018 • edited Loading

Checklist

mscdex commented Jul 7, 2018

lundibundi commented Jul 19, 2018

mcollina commented Jul 30, 2018

lundibundi commented Jul 30, 2018

mcollina commented Aug 6, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

lundibundi commented Aug 6, 2018

mcollina commented Aug 6, 2018

mafintosh commented Aug 6, 2018

mafintosh Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Aug 6, 2018

mcollina commented Aug 7, 2018

mcollina commented Aug 8, 2018

lundibundi commented Aug 9, 2018

mcollina commented Aug 9, 2018

lundibundi commented Aug 9, 2018 • edited Loading

mcollina commented Aug 9, 2018

lundibundi commented Aug 9, 2018 • edited Loading

mcollina commented Aug 9, 2018

mcollina commented Aug 22, 2018

lundibundi commented Jul 7, 2018 •

edited

Loading

mcollina commented Aug 6, 2018 •

edited

Loading

mafintosh Aug 6, 2018 •

edited

Loading

lundibundi commented Aug 9, 2018 •

edited

Loading

lundibundi commented Aug 9, 2018 •

edited

Loading