stream: Fixes missing 'unpipe' event for pipes made with {end: false} #11876

zaide-chris · 2017-03-16T06:22:06Z

Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.

Fixes: #11837

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

stream

zaide-chris · 2017-03-16T06:26:17Z

I was unsure if I should mark
make -j4 test (UNIX), or vcbuild test (Windows) passes
Completed as 6 tests fail with vcbuild test when I build the master before my changes and then those same 6 also fail after.

mcollina

Can you please wrap all the single tests into braces? Something like: https://github.com/nodejs/node/blob/master/test/parallel/test-buffer-badhex.js

mcollina · 2017-03-16T07:46:10Z

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

mscdex · 2017-03-16T08:09:11Z

test/parallel/test-stream-unpipe-event.js

+const dest5 = new NullWriteable();
+dest5.on('pipe', common.mustCall(() => {}));
+dest5.on('unpipe', () => {
+  throw new Error('unpipe should not have been emited');


s/emited/emitted/

mscdex · 2017-03-16T08:09:25Z

test/parallel/test-stream-unpipe-event.js

+const dest2 = new NullWriteable();
+dest2.on('pipe', common.mustCall(() => {}));
+dest2.on('unpipe', () => {
+  throw new Error('unpipe should not have been emited');


s/emited/emitted/

cjihrig · 2017-03-16T19:48:44Z

test/parallel/test-stream-unpipe-event.js

+
+const dest2 = new NullWriteable();
+dest2.on('pipe', common.mustCall(() => {}));
+dest2.on('unpipe', () => {


You could also use common.mustNotCall('unpipe should not have been emitted') in these cases.

calvinmetcalf · 2017-03-17T16:25:28Z

test/parallel/test-stream-unpipe-event.js

+  dest.on('unpipe', common.mustCall(() => {}));
+  src.pipe(dest, {end: false});
+  setImmediate(() => {
+    assert.strictEqual(src._readableState.pipesCount, 0);


this is the only test that fails without this pr

mcollina · 2017-03-17T17:27:17Z

This PR needs some more tests related to the actual use case for this to be fixed. What is the actual bug that it is causing to the user?

mcollina · 2017-03-17T17:29:47Z

@MylesBorins I would like to run CITGM with the special READABLE_STREAM=disable env variable. How do I do that?

zaide-chris · 2017-03-19T08:15:26Z

This PR needs some more tests related to the actual use case for this to be fixed.

Would you prefer a less abstract test like: https://gist.github.com/zaide-chris/7051b80cc3f34fd0e21be639e9252f85

What is the actual bug that it is causing to the user?

src.pipe(dest) and src.pipe(dest, {end: false}) act differently in more ways then what is listed in the docs so the tests pushed are based on what I wrote to find out exactly how different they are. I could not find any tests that tested for unpipe events in the case of a source stream ending even with a normal src.pipe(dest) that being the 1st test, with the one that currently fails using src.pipe(dest, {end: false}) being the 4th. So I guess the other 4 could be extraneous other then making sure both src.pipe(dest) and src.pipe(dest, {end: false}) act the same when it comes to emitting unpipe events.

gibfahn · 2017-03-19T18:21:52Z

I would like to run CITGM with the special READABLE_STREAM=disable env variable. How do I do that?

@mcollina you can just type READABLE_STREAM=disable citgm-all in the CITGM_COMMAND field.

mcollina · 2017-03-20T09:32:55Z

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

mcollina · 2017-03-20T10:07:11Z

@gibfahn this is what I'm getting:

+ READABLE_STREAM=disable citgm-all --nodedir=/home/iojs/build/workspace/citgm-smoker/nodes/debian8-64 -v verbose -x /home/iojs/build/workspace/citgm-smoker/nodes/debian8-64/smoker/report.xml -q error
/tmp/hudson5206784139195918878.sh: 10: /tmp/hudson5206784139195918878.sh: READABLE_STREAM=disable: not found
Build step 'Conditional steps (multiple)' marked build as failure
Recording test results
ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
Sending e-mails to: [email protected] [email protected] [email protected]
Notifying upstream projects of job completion
Finished: FAILURE

gibfahn · 2017-03-20T10:16:30Z

@mcollina that's odd, doing that works fine locally. I'll have a look.

gibfahn · 2017-03-22T23:47:44Z

@mcollina for some reason the export command doesn't work through Jenkins, probably because of the way it gets quoted.

I think the only other way to add the variable would be to modify the job before and after running CI. I can do that for you if you'd like.

mcollina · 2017-03-23T10:58:59Z

@gibfahn can we get a separate jobs on Jenkins to run this check?

gibfahn · 2017-03-23T12:02:18Z

@mcollina adding extra jobs isn't normally a good idea if we can avoid it, otherwise the different jobs tend to get out of sync.

I've added a checkbox to the job that will disable readable stream if checked (defaults to doing nothing). Don't actually have time to check it right now, but try it and let me know if it's broken.

mcollina · 2017-03-23T13:02:38Z

Thanks @gibfahn

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/668/
CITGM on master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/669/

mcollina · 2017-04-04T16:37:38Z

This is LGTM from me, and it seems it might take some more time to get this tested with CITGM properly, so we might get going.

cjihrig

LGTM with a nit.

cjihrig · 2017-04-04T16:43:40Z

test/parallel/test-stream-unpipe-event.js

+{
+  const dest = new NullWriteable();
+  const src = new NeverEndReadable();
+  dest.on('pipe', common.mustCall(() => {}));


Since this PR was opened, there were some changes to common. We have common.noop now for this type of callback. common.mustCall() can be called with no function, and it will default to common.noop.

TL;DR - common.mustCall(() => {}) throughout this PR can be replaced with common.mustCall().

@zaide-chris would you mind to update this?

mcollina · 2017-04-05T07:13:07Z

Would you prefer a less abstract test like: https://gist.github.com/zaide-chris/7051b80cc3f34fd0e21be639e9252f85

No, maybe something more easy to ready, just showing that 'unpipe' is not emitted.

Currently when the destination emits an 'error', 'finish' or 'close' event the pipe calls unpipe to emit 'unpipe' and trigger the clean up of all it's listeners. When the source emits an 'end' event without {end: false} it calls end() on the destination leading it to emit a 'close', this will again lead to the pipe calling unpipe. However the source emitting an 'end' event along side {end: false} is the only time the cleanup gets ran directly without unpipe being called. This fixes that so the 'unpipe' event does get emitted and cleanup in turn gets ran by that event. Fixes: nodejs#11837

mcollina · 2017-04-14T08:48:54Z

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

mcollina · 2017-04-14T13:18:28Z

Landed in 9dcf18a

Currently when the destination emits an 'error', 'finish' or 'close' event the pipe calls unpipe to emit 'unpipe' and trigger the clean up of all it's listeners. When the source emits an 'end' event without {end: false} it calls end() on the destination leading it to emit a 'close', this will again lead to the pipe calling unpipe. However the source emitting an 'end' event along side {end: false} is the only time the cleanup gets ran directly without unpipe being called. This fixes that so the 'unpipe' event does get emitted and cleanup in turn gets ran by that event. Fixes: nodejs#11837 PR-URL: nodejs#11876 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

evanlucas · 2017-04-25T17:39:10Z

This is landing but throws an error in v7.x. It depends on #12027 being backported

mcollina · 2017-05-01T08:37:49Z

@evanlucas would you prefer me to backport this manually? #12027 seems quite heavy. We should also backport this to v6-lts IMHO.

evanlucas · 2017-05-01T13:39:41Z

@mcollina that would be great! You can target the v7.x-staging branch. Thanks!

Currently when the destination emits an 'error', 'finish' or 'close' event the pipe calls unpipe to emit 'unpipe' and trigger the clean up of all it's listeners. When the source emits an 'end' event without {end: false} it calls end() on the destination leading it to emit a 'close', this will again lead to the pipe calling unpipe. However the source emitting an 'end' event along side {end: false} is the only time the cleanup gets ran directly without unpipe being called. This fixes that so the 'unpipe' event does get emitted and cleanup in turn gets ran by that event. Fixes: nodejs#11837 PR-URL: nodejs#11876 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

mcollina · 2017-05-02T07:14:18Z

@evanlucas here it is #12783.

Currently when the destination emits an 'error', 'finish' or 'close' event the pipe calls unpipe to emit 'unpipe' and trigger the clean up of all it's listeners. When the source emits an 'end' event without {end: false} it calls end() on the destination leading it to emit a 'close', this will again lead to the pipe calling unpipe. However the source emitting an 'end' event along side {end: false} is the only time the cleanup gets ran directly without unpipe being called. This fixes that so the 'unpipe' event does get emitted and cleanup in turn gets ran by that event. Fixes: #11837 PR-URL: #11876 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

gibfahn · 2017-05-15T17:16:16Z

Marking as dont-land because we're landing the v7.x backport.

MylesBorins · 2017-05-15T18:55:22Z

added backported-to-v6.x label. we should only use dont-land when we don't land things :D

MylesBorins · 2017-05-15T19:46:02Z

So it appears like the original test is broken and requires common.noop. It also appears that #12783 might be needed as well.

Can someone please manually backport to v6.x

Also appears that #12027 needs to land first as well

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Mar 16, 2017

mcollina requested changes Mar 16, 2017

View reviewed changes

mscdex reviewed Mar 16, 2017

View reviewed changes

cjihrig requested changes Mar 16, 2017

View reviewed changes

zaide-chris force-pushed the missing-unpipe-event-fix branch from 8a07770 to 23934e4 Compare March 16, 2017 22:05

mcollina mentioned this pull request Mar 17, 2017

March 2017 WG meeting nodejs/readable-stream#261

Closed

6 tasks

calvinmetcalf reviewed Mar 17, 2017

View reviewed changes

mcollina approved these changes Apr 4, 2017

View reviewed changes

cjihrig approved these changes Apr 4, 2017

View reviewed changes

zaide-chris force-pushed the missing-unpipe-event-fix branch from 23934e4 to a773504 Compare April 7, 2017 03:54

Made requested changes to test-stream-unpipe-event.js

a773504

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

mcollina closed this Apr 14, 2017

evanlucas added the backport-requested-v7.x label Apr 25, 2017

mcollina added the lts-watch-v6.x label May 1, 2017

mcollina mentioned this pull request May 2, 2017

stream: Backport of #11876, fixes missing 'unpipe' event #12783

Closed

3 tasks

gibfahn added dont-land-on-v6.x and removed lts-watch-v6.x labels May 15, 2017

gibfahn added the backported-to-v7.x label May 15, 2017

sam-github removed backport-requested-v7.x labels May 15, 2017

MylesBorins added the backported-to-v6.x label May 15, 2017

gibfahn added lts-watch-v6.x and removed backported-to-v6.x labels May 15, 2017

MylesBorins added the backport-requested-v6.x label May 15, 2017

sam-github mentioned this pull request May 15, 2017

test, tools: add common.noop and common.mustNotCall lint rule #12027

Closed

4 tasks

mcollina mentioned this pull request Jun 1, 2017

Updated to node v7.10.0. nodejs/readable-stream#286

Merged

polomsky mentioned this pull request Nov 21, 2017

No suitable finish event for pipe #17037

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: Fixes missing 'unpipe' event for pipes made with {end: false} #11876

stream: Fixes missing 'unpipe' event for pipes made with {end: false} #11876

zaide-chris commented Mar 16, 2017

zaide-chris commented Mar 16, 2017

mcollina left a comment

mcollina commented Mar 16, 2017

mscdex Mar 16, 2017

mscdex Mar 16, 2017

cjihrig Mar 16, 2017

calvinmetcalf Mar 17, 2017

mcollina commented Mar 17, 2017

mcollina commented Mar 17, 2017

zaide-chris commented Mar 19, 2017 •

edited

Loading

gibfahn commented Mar 19, 2017

mcollina commented Mar 20, 2017

mcollina commented Mar 20, 2017

gibfahn commented Mar 20, 2017

gibfahn commented Mar 22, 2017

mcollina commented Mar 23, 2017 •

edited

Loading

gibfahn commented Mar 23, 2017

mcollina commented Mar 23, 2017 •

edited

Loading

mcollina commented Apr 4, 2017

cjihrig left a comment

cjihrig Apr 4, 2017

mcollina Apr 5, 2017

mcollina commented Apr 5, 2017

mcollina commented Apr 14, 2017

mcollina commented Apr 14, 2017

evanlucas commented Apr 25, 2017

mcollina commented May 1, 2017

evanlucas commented May 1, 2017

mcollina commented May 2, 2017

gibfahn commented May 15, 2017

MylesBorins commented May 15, 2017

MylesBorins commented May 15, 2017 •

edited

Loading

stream: Fixes missing 'unpipe' event for pipes made with {end: false} #11876

stream: Fixes missing 'unpipe' event for pipes made with {end: false} #11876

Conversation

zaide-chris commented Mar 16, 2017

Checklist

Affected core subsystem(s)

zaide-chris commented Mar 16, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Mar 16, 2017

mscdex Mar 16, 2017

Choose a reason for hiding this comment

mscdex Mar 16, 2017

Choose a reason for hiding this comment

cjihrig Mar 16, 2017

Choose a reason for hiding this comment

calvinmetcalf Mar 17, 2017

Choose a reason for hiding this comment

mcollina commented Mar 17, 2017

mcollina commented Mar 17, 2017

zaide-chris commented Mar 19, 2017 • edited Loading

gibfahn commented Mar 19, 2017

mcollina commented Mar 20, 2017

mcollina commented Mar 20, 2017

gibfahn commented Mar 20, 2017

gibfahn commented Mar 22, 2017

mcollina commented Mar 23, 2017 • edited Loading

gibfahn commented Mar 23, 2017

mcollina commented Mar 23, 2017 • edited Loading

mcollina commented Apr 4, 2017

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Apr 4, 2017

Choose a reason for hiding this comment

mcollina Apr 5, 2017

Choose a reason for hiding this comment

mcollina commented Apr 5, 2017

mcollina commented Apr 14, 2017

mcollina commented Apr 14, 2017

evanlucas commented Apr 25, 2017

mcollina commented May 1, 2017

evanlucas commented May 1, 2017

mcollina commented May 2, 2017

gibfahn commented May 15, 2017

MylesBorins commented May 15, 2017

MylesBorins commented May 15, 2017 • edited Loading

zaide-chris commented Mar 19, 2017 •

edited

Loading

mcollina commented Mar 23, 2017 •

edited

Loading

mcollina commented Mar 23, 2017 •

edited

Loading

MylesBorins commented May 15, 2017 •

edited

Loading