Skip to content
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

mcollina
Copy link
Member

@mcollina mcollina commented Jan 4, 2018

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 PRs that contain breaking changes and should be released in the next major version. label Jan 4, 2018
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 4, 2018
@mcollina
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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty reasonable to me

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence seems like it’s missing something?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t setImmediate() enough here?

asyncReadable.push(null);
}));
assert.strictEqual(asyncReadable._readableState.needReadable, false);
}), 10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

mcollina commented Jan 5, 2018

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

@mcollina
Copy link
Member Author

mcollina commented Jan 5, 2018

@benjamingr
Copy link
Member

Benchmarks?

@addaleax
Copy link
Member

addaleax commented Jan 5, 2018

Benchmarks?

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer to omit the console.log :-)

});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment that explains the calculation

@jasnell
Copy link
Member

jasnell commented Jan 5, 2018

Still looks good to me. Couple of nits.

@mcollina
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
Copy link
Member Author

mcollina commented Jan 9, 2018

Added docs, PTAL

@jasnell
Copy link
Member

jasnell commented Jan 10, 2018

docs LGTM

@mcollina
Copy link
Member Author

Landed as 1e0f331.

@mcollina mcollina closed this Jan 10, 2018
mcollina added a commit that referenced this pull request Jan 10, 2018
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 10, 2018
mcollina pushed a commit that referenced this pull request Jan 29, 2018
Fixes a regression introduced by the once-per-microtick 'readable'
event emission.

See: #17979
PR-URL: #18372
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@mcollina mcollina deleted the stream-next-tick-readable branch February 7, 2018 08:01
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Fixes a regression introduced by the once-per-microtick 'readable'
event emission.

See: nodejs#17979
PR-URL: nodejs#18372
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@nevercast
Copy link

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

@mcollina
Copy link
Member Author

Yes this is in 10.1

squaremo added a commit to amqp-node/amqplib that referenced this pull request Jul 16, 2018
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").
squaremo added a commit to amqp-node/amqplib that referenced this pull request Aug 3, 2018
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").
@mcollina mcollina mentioned this pull request Aug 30, 2018
4 tasks
mcollina added a commit to mcollina/node that referenced this pull request Jan 11, 2019
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
mcollina added a commit that referenced this pull request Jan 15, 2019
#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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
#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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants