stream: always defer 'readable' with nextTick #17979

mcollina · 2018-01-04T17:13:51Z

Checklist

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

Affected core subsystem(s)

stream

mcollina · 2018-01-04T17:18:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12407/
CITGM (disable readable-stream): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1186/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1187/

mcollina · 2018-01-04T17:24:28Z

This solves a recursive push() -> _read() - > push() cycle when multiple push() happen asynchronously. I'm not 100% sure this is something we want to do at this point, nor if this is the right way to achieve this.

It's tagged as semver-major because I feel it might break some edge cases. We might want to revert this quickly if that is the case.

cc @nodejs/streams @nodejs/tsc

addaleax

Seems pretty reasonable to me

The dicer CITGM failures appear to be real (or are pretty new at least), /cc @mscdex

addaleax · 2018-01-04T19:48:47Z

test/parallel/test-stream-pipe-await-drain-push-while-write.js

+    if (chunk.length === 32 * 1024) { // first chunk
+      readable.push(Buffer.alloc(34 * 1024)); // above hwm
+      // We should check if awaitDrain counter is increased in the next
+      // tick, because when the 'data' event is fired


This sentence seems like it’s missing something?

addaleax · 2018-01-04T19:49:53Z

test/parallel/test-stream-readable-emittedReadable.js

+  process.nextTick(common.mustCall(() => {
+    readable.push(null);
+  }));
+}), 10);


Isn’t setImmediate() enough here?

addaleax · 2018-01-04T19:52:29Z

test/parallel/test-stream-readable-needReadable.js

  asyncReadable.push(null);
-}));
+  assert.strictEqual(asyncReadable._readableState.needReadable, false);
+}), 10);


Ditto … using setTimeout() tends to introduce race conditions because they might expire before the current event loop turn is over, but setImmediate() always defers (and doesn’t keep test running longer than they need to)

Fixes: nodejs#3203

mcollina · 2018-01-05T11:47:50Z

Fixed nits.
The dicer regression is legit, I'll dig into it.

mcollina · 2018-01-05T16:39:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12420/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1191/

Dicer should be fixed.

benjamingr · 2018-01-05T18:50:42Z

Benchmarks?

addaleax · 2018-01-05T18:59:31Z

Benchmarks?

Benchmarks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/87/

jasnell · 2018-01-05T19:09:32Z

test/parallel/test-stream-readable-object-multi-push-async.js

+        }
+
+        if (data.length === 0) {
+          console.log('pushing null');


would prefer to omit the console.log :-)

jasnell · 2018-01-05T19:10:40Z

test/parallel/test-stream-readable-object-multi-push-async.js

+  });
+
+  readable.on('end', common.mustCall(() => {
+    assert.strictEqual(i, (Math.floor(MAX / BATCH) + 1) * BATCH);


Please add a comment that explains the calculation

jasnell · 2018-01-05T19:11:22Z

Still looks good to me. Couple of nits.

mcollina · 2018-01-05T20:16:18Z

                                                   improvement confidence   p.value

streams/readable-bigread.js n=1000 0.40 % 0.7330756
streams/readable-bigunevenread.js n=1000 -0.31 % 0.7630930
streams/readable-boundaryread.js type="buffer" n=2000 -1.21 % 0.2752480
streams/readable-boundaryread.js type="string" n=2000 -0.85 % 0.4784480
streams/readable-readall.js n=5000 -1.52 % 0.1030397
streams/readable-unevenread.js n=1000 0.57 % 0.1487255
streams/transform-creation.js n=1000000 3.56 % 0.1932558
streams/writable-manywrites.js n=2000000 -2.82 % 0.2757697

No perf regression!

mcollina · 2018-01-09T10:05:58Z

Added docs, PTAL

jasnell · 2018-01-10T00:11:46Z

docs LGTM

mcollina · 2018-01-10T11:07:34Z

Landed as 1e0f331.

Emit 'readable' always in the next tick, resulting in a single call to _read() per microtick. This removes the need for the user to implement buffering if they wanted to call this.push() multiple times in an asynchronous fashion, as this.push() triggers this._read() call. PR-URL: #17979 Fixes: #3203 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Fixes a regression introduced by the once-per-microtick 'readable' event emission. See: #17979 PR-URL: #18372 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Fixes a regression introduced by the once-per-microtick 'readable' event emission. See: nodejs#17979 PR-URL: nodejs#18372 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nevercast · 2018-05-16T22:09:20Z

This landed is Node.js v10.1 yes? That is how I should interpret the linked commits?

mcollina · 2018-05-16T22:13:04Z

Yes this is in 10.1

The behaviour of the `'readable'` event changed, or was tightened up, in nodejs/node#17979 such that it is _always_ called on the next tick. This change appears first in Node.JS v10.0). Since we rely on `'readable' in the multiplexing, it means that we have to be more careful about when we wait for the next event loop to come around in tests. The tests tend to be much more brittle than live code with respect to the order things happen, since they attempt to nail down precise states or orderings (e.g., "the `unpipe` happened exactly between these writes").

nodejs#17979 introduced a change in the doc that was not correct about _read always being called asynchronously. This does not hold true when it is in flowing mode. See: nodejs#17979 Fixes: nodejs#24919

#17979 introduced a change in the doc that was not correct about _read always being called asynchronously. This does not hold true when it is in flowing mode. See: #17979 Fixes: #24919 PR-URL: #25442 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>

nodejs#17979 introduced a change in the doc that was not correct about _read always being called asynchronously. This does not hold true when it is in flowing mode. See: nodejs#17979 Fixes: nodejs#24919 PR-URL: nodejs#25442 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>

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

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

jasnell approved these changes Jan 4, 2018

View reviewed changes

addaleax approved these changes Jan 4, 2018

View reviewed changes

stream: always defer 'readable' with nextTick

102c2f0

Fixes: nodejs#3203

mcollina force-pushed the stream-next-tick-readable branch from 6de1dc6 to 102c2f0 Compare January 5, 2018 11:47

squash

fd964de

jasnell reviewed Jan 5, 2018

View reviewed changes

benjamingr approved these changes Jan 5, 2018

View reviewed changes

mcollina mentioned this pull request Jan 9, 2018

Readable.push calling _read #3203

Closed

squash, docs

a421eda

mcollina closed this Jan 10, 2018

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 10, 2018

mafintosh mentioned this pull request Jan 25, 2018

stream: fix readable being emitted when nothing to do #18372

Closed

4 tasks

mcollina deleted the stream-next-tick-readable branch February 7, 2018 08:01

jorgebay mentioned this pull request May 16, 2018

Add node.js 10 to CI matrices and update tests to account for changes in node.js 10 datastax/nodejs-driver#279

Merged

mcollina mentioned this pull request Jun 2, 2018

[v8.x fix] - stream: fix end not being called when reading some base64 files #21089

Closed

4 tasks

mcollina mentioned this pull request Aug 30, 2018

stream-whatwg: add whatwg streams #22352

Closed

4 tasks

ZYSzys mentioned this pull request Dec 21, 2018

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

Closed

mcollina mentioned this pull request Jan 11, 2019

doc: revert incorrect change on readable._read. #25442

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: always defer 'readable' with nextTick #17979

stream: always defer 'readable' with nextTick #17979

mcollina commented Jan 4, 2018 •

edited

Loading

mcollina commented Jan 4, 2018

mcollina commented Jan 4, 2018

addaleax left a comment

addaleax Jan 4, 2018

addaleax Jan 4, 2018

addaleax Jan 4, 2018

mcollina commented Jan 5, 2018

mcollina commented Jan 5, 2018 •

edited

Loading

benjamingr commented Jan 5, 2018

addaleax commented Jan 5, 2018

jasnell Jan 5, 2018

jasnell Jan 5, 2018

jasnell commented Jan 5, 2018

mcollina commented Jan 5, 2018

mcollina commented Jan 9, 2018

jasnell commented Jan 10, 2018

mcollina commented Jan 10, 2018

nevercast commented May 16, 2018

mcollina commented May 16, 2018

stream: always defer 'readable' with nextTick #17979

stream: always defer 'readable' with nextTick #17979

Conversation

mcollina commented Jan 4, 2018 • edited Loading

Checklist

Affected core subsystem(s)

mcollina commented Jan 4, 2018

mcollina commented Jan 4, 2018

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jan 4, 2018

Choose a reason for hiding this comment

addaleax Jan 4, 2018

Choose a reason for hiding this comment

addaleax Jan 4, 2018

Choose a reason for hiding this comment

mcollina commented Jan 5, 2018

mcollina commented Jan 5, 2018 • edited Loading

benjamingr commented Jan 5, 2018

addaleax commented Jan 5, 2018

jasnell Jan 5, 2018

Choose a reason for hiding this comment

jasnell Jan 5, 2018

Choose a reason for hiding this comment

jasnell commented Jan 5, 2018

mcollina commented Jan 5, 2018

mcollina commented Jan 9, 2018

jasnell commented Jan 10, 2018

mcollina commented Jan 10, 2018

nevercast commented May 16, 2018

mcollina commented May 16, 2018

mcollina commented Jan 4, 2018 •

edited

Loading

mcollina commented Jan 5, 2018 •

edited

Loading