Skip to content

stream: fix backpressure when multiple sync push #19613

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
Closed
mcollina wants to merge 1 commit into from
Closed

stream: fix backpressure when multiple sync push #19613

mcollina wants to merge 1 commit into from

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Mar 26, 2018

Closes: #19601

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Mar 26, 2018
@mcollina mcollina changed the title stream: fix backpressure when multiple sync stream: fix backpressure when multiple sync push Mar 26, 2018
@mcollina
Copy link
Member Author

mcollina commented Mar 26, 2018

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

@mcollina mcollina mentioned this pull request Mar 26, 2018
Closed
@mcollina
Copy link
Member Author

mcollina commented Mar 26, 2018

cc @nodejs/streams @mafintosh @addaleax

mscdex
mscdex reviewed Mar 26, 2018
View reviewed changes
lib/_stream_readable.js Outdated
Copy link
Contributor

@mscdex mscdex Mar 26, 2018

Choose a reason for hiding this comment

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

This should be removed completely.

mscdex
mscdex reviewed Mar 26, 2018
View reviewed changes
lib/_stream_readable.js Outdated
Copy link
Contributor

@mscdex mscdex Mar 26, 2018

Choose a reason for hiding this comment

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

s/need/needs/

Copy link
Member

@targos targos Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also please capitalize all sentences.

mscdex
mscdex reviewed Mar 26, 2018
View reviewed changes
lib/_stream_readable.js Outdated
Copy link
Contributor

@mscdex mscdex Mar 26, 2018

Choose a reason for hiding this comment

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

s/neded/needed/

Copy link
Member

@targos targos Mar 26, 2018

Choose a reason for hiding this comment

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

I think it's meant to be "ended"

mscdex
mscdex reviewed Mar 26, 2018
View reviewed changes
lib/_stream_readable.js Outdated
Copy link
Contributor

@mscdex mscdex Mar 26, 2018

Choose a reason for hiding this comment

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

s/some/for some/

@mcollina
Copy link
Member Author

mcollina commented Mar 26, 2018

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1343/

@mcollina mcollina force-pushed the fix-backpressure-needReadable branch from 43be365 to 69e5f3a Compare March 26, 2018 15:07
@mcollina
Copy link
Member Author

mcollina commented Mar 26, 2018

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

mscdex
mscdex reviewed Mar 26, 2018
View reviewed changes
lib/_stream_readable.js Outdated
Copy link
Contributor

@mscdex mscdex Mar 26, 2018

Choose a reason for hiding this comment

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

s/eneded/ended/

Copy link
Member Author

@mcollina mcollina Mar 26, 2018

Choose a reason for hiding this comment

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

I keep doing typos, thanks!

@mcollina mcollina force-pushed the fix-backpressure-needReadable branch from 69e5f3a to 225a693 Compare March 26, 2018 22:27
@mcollina
Copy link
Member Author

mcollina commented Mar 26, 2018

There have been a graceful-fs and spdy failures on citgm on the previous run, but they seem unrelated (just flaky). Running citgm again.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1344/

addaleax
addaleax reviewed Mar 26, 2018
View reviewed changes
test/parallel/test-stream-backpressure.js Outdated
Copy link
Member

@addaleax addaleax Mar 26, 2018

Choose a reason for hiding this comment

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

nit: setImmediate()?

Copy link
Member Author

@mcollina mcollina Mar 27, 2018

Choose a reason for hiding this comment

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

done.

test/parallel/test-stream-backpressure.js Outdated
Copy link
Member

@addaleax addaleax Mar 26, 2018

Choose a reason for hiding this comment

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

Would it make sense to tune this even tighter, and check that length <= HWM? As I understand it that should be the relevant condition for making sure that _read() doesn’t get called

Copy link
Member Author

@mcollina mcollina Mar 27, 2018

Choose a reason for hiding this comment

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

This is not going to be possible, as we are over highWaterMark here in half of the cases even on Node 8. I'll make the check stricter with length <= total, because that's what will happen.

lib/_stream_readable.js Outdated
Copy link
Member

@addaleax addaleax Mar 26, 2018

Choose a reason for hiding this comment

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

Hm … what about this comment? It sounds like it needs to be adjusted, since it explains why we wanted needReadable here in the first place?

Copy link
Member Author

@mcollina mcollina Mar 27, 2018

Choose a reason for hiding this comment

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

I think that comment is actually wrong. It says

if it's past the high water mark, we can push in some more.

However it has a check (simplified) as:

return state.length < state.highWaterMark

I would prefer that we tackle that in another PR, as I would like to get this landed as it solves a potential memory leak situation. If you are ok I'll open an issue about it, as it needs to be investigated and explained better.

Copy link
Member Author

@mcollina mcollina Mar 29, 2018

Choose a reason for hiding this comment

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

@addaleax is this approach good for you? This should be ready to land.

Copy link
Member

@addaleax addaleax Mar 29, 2018

Choose a reason for hiding this comment

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

@mcollina Sure, deferring to another PR sounds fine to me. :)

@mcollina mcollina force-pushed the fix-backpressure-needReadable branch from 225a693 to c4cb320 Compare March 27, 2018 22:20
@mcollina
Copy link
Member Author

mcollina commented Mar 27, 2018

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

@mcollina mcollina force-pushed the fix-backpressure-needReadable branch from c4cb320 to 77f4d29 Compare March 28, 2018 16:13
@mafintosh
Copy link
Member

mafintosh commented Mar 28, 2018

Great to get this fixed! Thanks everyone.

@mcollina
Copy link
Member Author

mcollina commented Mar 29, 2018

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

addaleax
addaleax approved these changes Mar 29, 2018
View reviewed changes
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.

LGTM with the caveat that I, like most people, don’t have a good overview of the internal streams state machine

lib/_stream_readable.js Outdated
Copy link
Member

@addaleax addaleax Mar 29, 2018

Choose a reason for hiding this comment

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

@mcollina Sure, deferring to another PR sounds fine to me. :)

@mcollina
Copy link
Member Author

mcollina commented Mar 30, 2018

Landed as d37e59f.

@mcollina mcollina closed this Mar 30, 2018
mcollina added a commit that referenced this pull request Mar 30, 2018
@mcollina
stream: fix backpressure when multiple sync
d37e59f 
PR-URL: #19613
Fixes: #19601
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mcollina mcollina mentioned this pull request Mar 30, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@trivikr trivikr trivikr approved these changes

+1 more reviewer

@mscdex mscdex mscdex left review comments

Reviewers whose approvals may not affect merge requirements

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.

8 participants

@mcollina @mafintosh @mscdex @jasnell @addaleax @targos @trivikr @nodejs-github-bot