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.

Sign up for GitHub

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

Jump to bottom

[v9.x] stream: always reset awaitDrain when emitting data #18911

Closed
wants to merge 1 commit into from
Closed

[v9.x] stream: always reset awaitDrain when emitting data #18911

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Backport of #18516, only conflict being the added debug() line that is being borrowed from 1e0f331

@nodejs-github-bot nodejs-github-bot added stream Issues and PRs related to the stream subsystem. v9.x labels Feb 21, 2018
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/16420/

@addaleax
Copy link
Member Author

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

Could I get an approval on this?

@MylesBorins
Copy link
Contributor

@addaleax looks like this could use a rebase and another CI run

@addaleax addaleax force-pushed the backport-v9.x-18516 branch 2 times, most recently from 75211b6 to e4c10f7 Compare February 27, 2018 17:49
@addaleax
Copy link
Member Author

@nodejs/streams I’m not sure why this test is failing, but it seems like it’s more complex and has to do with different timing between 9.x and master?

The failure looks like this:

Mismatched <anonymous> function calls. Expected exactly 3, actual 2.
    at Object.exports.mustCall (/home/sqrt/src/node/v9/test/common/index.js:437:10)
    at Object.<anonymous> (/home/sqrt/src/node/v9/test/parallel/test-stream-pipe-await-drain-push-while-write.js:17:17)
    at Module._compile (module.js:662:30)
    at Object.Module._extensions..js (module.js:673:10)
    at Module.load (module.js:575:32)
    at tryModuleLoad (module.js:515:12)
    at Function.Module._load (module.js:507:3)
    at Function.Module.runMain (module.js:703:10)
    at startup (bootstrap_node.js:193:16)

@addaleax
stream: always reset awaitDrain when emitting data
b26d272 
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.

By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).

PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mcollina
Copy link
Member

'readable' is always deferred in master, that can explain it.

@MylesBorins
Copy link
Contributor

@mcollina next steps for this PR?

@addaleax
Copy link
Member Author

addaleax commented Mar 6, 2018

I’m not sure how to fix the issues I was getting here – and it does seem like this is still a valid and necessary bug fix, the test fails without the changes to the streams code on v9.x.

@MylesBorins MylesBorins force-pushed the v9.x-staging branch 2 times, most recently from 759b210 to 4e9ac10 Compare March 6, 2018 23:26
@mcollina
Copy link
Member

mcollina commented Mar 7, 2018

I will need to have a look. The fix for v9.x (and 8.x) might be more complicated. This is probably leveraging #18515 (which is semver-major).

@MylesBorins MylesBorins force-pushed the v9.x-staging branch 2 times, most recently from 630a653 to ef4714c Compare March 7, 2018 21:12
@mcollina
Copy link
Member

mcollina commented Mar 9, 2018

I do not think this is backportable to 9 or 8.
The stream is left in a paused state with this change, as it relies on .push() deferring any activity to the next tick, which is semver major (#18515).

I think we can close this and flag #18516 as dont-land-on-9. It might be possible to do a completely different fix for the issue on 8 or 9, but I don't see this severe enough to be worth the effort (but happy to reconsider if there is a specific need).

@addaleax
Copy link
Member Author

addaleax commented Mar 9, 2018

Ok, I’ll close this then. Thanks for investigating! And I agree, if somebody runs into this issue we can still look into it in more details.

@addaleax addaleax closed this Mar 9, 2018
@addaleax addaleax deleted the backport-v9.x-18516 branch March 9, 2018 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@addaleax @MylesBorins @mcollina @nodejs-github-bot