stream: always defer 'readable' with nextTick #17979
Conversation
mcollina
added
the
semver-major
nodejs-github-bot
added
the
stream
|
CI: https://ci.nodejs.org/job/node-test-pull-request/12407/ |
|
This solves a recursive 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 |
| 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 |
There was a problem hiding this comment.
This sentence seems like it’s missing something?
| process.nextTick(common.mustCall(() => { | ||
| readable.push(null); | ||
| })); | ||
| }), 10); |
There was a problem hiding this comment.
Isn’t setImmediate() enough here?
| asyncReadable.push(null); | ||
| })); | ||
| assert.strictEqual(asyncReadable._readableState.needReadable, false); | ||
| }), 10); |
There was a problem hiding this comment.
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)
mcollina
force-pushed
the
stream-next-tick-readable
branch
from
6de1dc6 to
102c2f0
Compare
|
Fixed nits. |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/12420/ Dicer should be fixed. |
|
Benchmarks? |
Benchmarks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/87/ |
| } | ||
|
|
||
| if (data.length === 0) { | ||
| console.log('pushing null'); |
There was a problem hiding this comment.
would prefer to omit the console.log :-)
| }); | ||
|
|
||
| readable.on('end', common.mustCall(() => { | ||
| assert.strictEqual(i, (Math.floor(MAX / BATCH) + 1) * BATCH); |
There was a problem hiding this comment.
Please add a comment that explains the calculation
|
Still looks good to me. Couple of nits. |
streams/readable-bigread.js n=1000 0.40 % 0.7330756 No perf regression! |
|
Added docs, PTAL |
|
docs LGTM |
|
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>
mcollina
added
the
notable-change
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>
|
This landed is Node.js v10.1 yes? That is how I should interpret the linked commits? |
|
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").
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>
#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>
Fixes: #3203
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
stream