stream: only increase awaitDrain once for each pipe destination #7292

davedoesdev · 2016-06-13T21:58:19Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

stream

Description of change

Guard against the call to write() inside pipe's ondata pushing more data
back onto the Readable, thus causing ondata to be called again.

This is fine but results in awaitDrain being increased more than once.
The problem with that is when the destination does drain, only a single drain
event is emitted, so awaitDrain in this case will never reach zero and we
end up with a permanently paused stream.
#7278

addaleax · 2016-06-13T22:01:00Z

test/parallel/test-stream-pipe-await-drain-push-while-write.js

Just curious, would a Readable suffice here?

Yes, but I'd have to set its _read to a nullop.

I guess I could try doing the pushs in the _read.

You could use new stream.Readable({read: () => {}}), but you can just leave it as a PassThrough if you prefer, I really was just curious. :)

addaleax · 2016-06-13T22:04:18Z

Thanks, generally looking good! If you want, you can add an Fixes: https://github.com/nodejs/node/issues/7278 line to the commit message so the issue will automatically be closed when this is landed.

davedoesdev · 2016-06-13T22:11:01Z

@addaleax thanks for the review. I'll add a commit addressing your comments but it won't be today now.

davedoesdev · 2016-06-13T23:09:25Z

okay I just updated the test

addaleax · 2016-06-14T00:00:01Z

test/parallel/test-stream-pipe-await-drain-push-while-write.js

Is this line actually necessary? I think the test still tests what it’s supposed to test if you leave it out, because after the push() call here awaitDrain would already be 2… right?

I wanted to test the effect (missing third buffer) rather than the cause.
In any case, awaitDrain isn't incremented until _write returns so it won't already be 2.

addaleax · 2016-06-14T00:02:02Z

/cc @nodejs/streams

I’d be kind of interested in knowing whether we generally guarantee that .push() and/or .write() are reentrant and can be called recursively or not.

addaleax · 2016-06-14T00:02:14Z

Also, I’m not sure why, but this branch seems to be based on a master that’s about a month old – rebasing might be nice for testing this out locally.

davedoesdev · 2016-06-14T05:51:14Z

If push and write aren't reentrant then it'd be great to add it to the API docs.

davedoesdev · 2016-06-14T05:51:34Z

No idea why it's a month old - only forked it yesterday.

davedoesdev · 2016-06-14T06:06:22Z

This branch is now rebased on latest master.

davedoesdev · 2016-06-14T06:38:38Z

@addaleax I removed the process.nextTick since resume/flow takes care of that now in the test. Thanks!

mcollina · 2016-06-14T07:13:25Z

lib/_stream_readable.js

can you please add a comment on the reason for this variable?

also we probably wanna move up the declaration of increasedAwaitDrain to before the ondata handler is attached. i'm pretty sure that might fire sync in some cases

mcollina · 2016-06-14T07:14:45Z

Can you please format the commit message and squash the commits?

mcollina · 2016-06-14T07:34:52Z

This bug is "fixed" from [email protected] to [email protected], where 2.0.3 and 2.1.3 are broken.

cc @calvinmetcalf

Guard against the call to write() inside pipe's ondata pushing more data back onto the Readable, thus causing ondata to be called again. This is fine but results in awaitDrain being increased more than once. The problem with that is when the destination does drain, only a single 'drain' event is emitted, so awaitDrain in this case will never reach zero and we end up with a permanently paused stream.

addaleax · 2016-06-14T07:52:25Z

@mcollina I assume the “fixed” range corresponds to #2325 … #6023?

And I’m not sure why you added good first contribution to a PR… I mean, it is a good first contribution, but I think that’s rather used to signal that an issue is up for grabs by newbies? I’m removing it but feel free to re-add.

mcollina · 2016-06-14T08:01:52Z

@addaleax exactly. That means if you are depending on readable-stream@~2.0 it's fixed, and broken in readable-stream@~2.1 and readable-stream@^2.1.

(I thought good first contribution was to tag possible future contributors, maybe I got it wrong)

davedoesdev · 2016-06-15T06:17:00Z

Should be squashed and reformatted now.

mcollina · 2016-06-15T08:10:10Z

LGTM

addaleax · 2016-06-15T11:38:30Z

CI: https://ci.nodejs.org/job/node-test-commit/3753/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/303/ (If I’ve got everything right)

addaleax · 2016-06-15T19:08:54Z

LGTM

addaleax · 2016-06-15T23:34:59Z

Landed in b5175e8

Thanks for fixing this bug, @davedoesdev! I believe this is your first commit to core, if so, welcome on board and we hope you find other ways to contribute!

Guard against the call to write() inside pipe's ondata pushing more data back onto the Readable, thus causing ondata to be called again. This is fine but results in awaitDrain being increased more than once. The problem with that is when the destination does drain, only a single 'drain' event is emitted, so awaitDrain in this case will never reach zero and we end up with a permanently paused stream. Fixes: #7278 PR-URL: #7292 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Fishrock123 · 2016-06-16T15:16:16Z

test/parallel/test-stream-pipe-await-drain-push-while-write.js

+// A readable stream which produces two buffers.
+const bufs = [new Buffer(32 * 1024), new Buffer(33 * 1024)]; // above hwm
+const readable = new stream.Readable({
+  read: function() {


could have been read() { ^_^

Guard against the call to write() inside pipe's ondata pushing more data back onto the Readable, thus causing ondata to be called again. This is fine but results in awaitDrain being increased more than once. The problem with that is when the destination does drain, only a single 'drain' event is emitted, so awaitDrain in this case will never reach zero and we end up with a permanently paused stream. Fixes: #7278 PR-URL: #7292 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jun 13, 2016

addaleax reviewed Jun 13, 2016
View reviewed changes

This was referenced Jun 13, 2016

Change in stream behaviour in Node 4.4.5 #7278

Closed

Streams Regression in v4.x nodejs/Release#112

Closed

addaleax reviewed Jun 14, 2016
View reviewed changes

mcollina reviewed Jun 14, 2016
View reviewed changes

mcollina added the good first issue Issues that are suitable for first-time contributors. label Jun 14, 2016

addaleax removed the good first issue Issues that are suitable for first-time contributors. label Jun 14, 2016

addaleax added the lts-watch-v4.x label Jun 15, 2016

addaleax assigned MylesBorins Jun 15, 2016

addaleax closed this Jun 15, 2016

Fishrock123 reviewed Jun 16, 2016
View reviewed changes

evanlucas mentioned this pull request Jun 16, 2016

v6.2.2 proposal #7323

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 23, 2016

MylesBorins mentioned this pull request Jun 24, 2016

v4.4.7 proposal #7395

Merged

MylesBorins removed their assignment Dec 27, 2016

Uh oh!

stream: only increase awaitDrain once for each pipe destination #7292

stream: only increase awaitDrain once for each pipe destination #7292

Uh oh!

Conversation

davedoesdev commented Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jun 13, 2016

Uh oh!

davedoesdev commented Jun 13, 2016

Uh oh!

davedoesdev commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jun 14, 2016

Uh oh!

addaleax commented Jun 14, 2016

Uh oh!

davedoesdev commented Jun 14, 2016

Uh oh!

davedoesdev commented Jun 14, 2016

Uh oh!

davedoesdev commented Jun 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davedoesdev commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jun 14, 2016

Uh oh!

mcollina commented Jun 14, 2016

Uh oh!

addaleax commented Jun 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jun 14, 2016

Uh oh!

davedoesdev commented Jun 15, 2016

Uh oh!

mcollina commented Jun 15, 2016

Uh oh!

addaleax commented Jun 15, 2016

Uh oh!

addaleax commented Jun 15, 2016

Uh oh!

addaleax commented Jun 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

davedoesdev commented Jun 13, 2016 •

edited

Loading

davedoesdev commented Jun 14, 2016 •

edited

Loading

addaleax commented Jun 14, 2016 •

edited

Loading