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: always defer 'readable' with nextTick #17979

Closed
wants to merge 3 commits into from

Conversation

Reviewers

@jasnell jasnell

@addaleax addaleax

@benjamingr benjamingr

Assignees
No one assigned
Labels
notable-change semver-major stream
Projects
None yet
Milestone
No milestone
6 participants
@mcollina @benjamingr @addaleax @jasnell @nevercast @nodejs-github-bot
@mcollina
Copy link
Member

mcollina commented Jan 4, 2018 •
edited

Fixes: #3203

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 mcollina added the semver-major label Jan 4, 2018

@nodejs-github-bot nodejs-github-bot added the stream label Jan 4, 2018

@mcollina
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Jan 4, 2018

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

jasnell approved these changes Jan 4, 2018
View changes

@addaleax

addaleax approved these changes Jan 4, 2018
View changes

Copy link
Member

addaleax left a comment

Seems pretty reasonable to me

The dicer CITGM failures appear to be real (or are pretty new at least), /cc @mscdex
test/parallel/test-stream-pipe-await-drain-push-while-write.js Outdated
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 comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jan 4, 2018

Member

This sentence seems like it’s missing something?
test/parallel/test-stream-readable-emittedReadable.js Outdated
process.nextTick(common.mustCall(() => {
readable.push(null);
}));
}), 10);

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jan 4, 2018

Member

Isn’t setImmediate() enough here?
test/parallel/test-stream-readable-needReadable.js Outdated
asyncReadable.push(null);
}));
assert.strictEqual(asyncReadable._readableState.needReadable, false);
}), 10);

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jan 4, 2018

Member

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 mcollina force-pushed the mcollina:stream-next-tick-readable branch from 6de1dc6 to 102c2f0 Jan 5, 2018

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Jan 5, 2018

Fixed nits.
The dicer regression is legit, I'll dig into it.
@mcollina
squash
Loading status checks…
fd964de
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Jan 5, 2018 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member

benjamingr commented Jan 5, 2018

Benchmarks?
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Jan 5, 2018

Benchmarks?

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

jasnell reviewed Jan 5, 2018
View changes

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

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

This comment has been minimized.

Sign in to view
Copy link
@jasnell

jasnell Jan 5, 2018

Member

would prefer to omit the console.log :-)
@jasnell

jasnell reviewed Jan 5, 2018
View changes

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

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

This comment has been minimized.

Sign in to view
Copy link
@jasnell

jasnell Jan 5, 2018

Member

Please add a comment that explains the calculation
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Jan 5, 2018

Still looks good to me. Couple of nits.
@benjamingr

benjamingr approved these changes Jan 5, 2018
View changes

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Jan 5, 2018 

                                                   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 mcollina referenced this pull request Jan 9, 2018

Closed

Readable.push calling _read #3203

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Jan 9, 2018

Added docs, PTAL
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Jan 10, 2018

docs LGTM
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented Jan 10, 2018

Landed as 1e0f331.

@mcollina mcollina closed this Jan 10, 2018

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

@mcollina
stream: always defer 'readable' with nextTick 
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>
1e0f331

@mcollina mcollina added the notable-change label Jan 10, 2018

@mafintosh mafintosh referenced this pull request Jan 25, 2018

Closed

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

4 of 4 tasks complete

mcollina added a commit that referenced this pull request Jan 29, 2018

@mafintosh @mcollina
stream: do not emit readable if the stream ended 
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>
0778f79

kcaulfield94 added a commit to kcaulfield94/node that referenced this pull request Feb 2, 2018

@mafintosh @kcaulfield94
stream: do not emit readable if the stream ended 
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>
9b8425e

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

@mcollina @msoechting
stream: always defer 'readable' with nextTick 
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: nodejs#17979
Fixes: nodejs#3203
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
e30118e

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

@mafintosh @msoechting
stream: do not emit readable if the stream ended 
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>
dc4c32d

@mcollina mcollina deleted the mcollina:stream-next-tick-readable branch Feb 7, 2018

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

@mcollina @msoechting
stream: always defer 'readable' with nextTick 
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: nodejs#17979
Fixes: nodejs#3203
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
f1a2810

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

@mafintosh @msoechting
stream: do not emit readable if the stream ended 
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>
182c60b

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

@mafintosh @MayaLekova
stream: do not emit readable if the stream ended 
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>
613978c

@jorgebay jorgebay referenced this pull request May 16, 2018

Merged

Add node.js 10 to CI matrices and update tests to account for changes in node.js 10 #279

@nevercast

This comment has been minimized.

Sign in to view
Copy link

nevercast commented May 16, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

mcollina commented May 16, 2018

Yes this is in 10.1

@mcollina mcollina referenced this pull request Jun 2, 2018

Closed

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

2 of 4 tasks complete

squaremo added a commit to squaremo/amqp.node that referenced this pull request Jul 16, 2018

@squaremo
Reorder mux test to account for async reads 
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").
Loading status checks…
8d6594b

squaremo added a commit to squaremo/amqp.node that referenced this pull request Aug 3, 2018

@squaremo
Reorder mux test because 'readable' event deferred 
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").
42ac03c

@mcollina mcollina referenced this pull request Aug 30, 2018

Closed

stream-whatwg: add whatwg streams #22352

4 of 4 tasks complete

@ZYSzys ZYSzys referenced this pull request Dec 21, 2018

Open

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

mcollina added a commit to mcollina/node that referenced this pull request Jan 11, 2019

@mcollina
doc: revert incorrect change on readable._read. 
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
fe4fd34

mcollina added a commit to mcollina/node that referenced this pull request Jan 11, 2019

@mcollina
doc: revert incorrect change on readable._read 
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
61709ce

@mcollina mcollina referenced this pull request Jan 11, 2019

Closed

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

3 of 3 tasks complete

mcollina added a commit that referenced this pull request Jan 15, 2019

@mcollina
doc: revert incorrect change on readable._read 
#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>
Loading status checks…
2f1ae9e

addaleax added a commit that referenced this pull request Jan 15, 2019

@mcollina @addaleax
doc: revert incorrect change on readable._read 
#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>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
edab2d6

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

@mcollina @BridgeAR
doc: revert incorrect change on readable._read 
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>
Verified
This commit was signed with a verified signature.
@BridgeAR BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
b5d3618
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.