stream: Backport of #11876, fixes missing 'unpipe' event#12783
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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 added lts-watch-v6.x stream 
nodejs-github-bot added stream 
MylesBorins left a comment
MylesBorins left a comment There was a problem hiding this comment.
rubber stamp LGTM if CI is green
mcollina commented May 2, 2017
mcollina commented May 2, 2017
who is merging? Should we wait? I think there is a spurious failure: https://ci.nodejs.org/job/node-test-commit-linux/9551/nodes=ubuntu1204-clang341-64/console. |
evanlucas commented May 2, 2017
@mcollina I'll land. I'm working on v7.10.0 right now anyways. I'm pretty sure that build bot is supposed to be skipped on v7.x? |
mcollina commented May 2, 2017
@evanlucas I have no idea, but the error seems some due to some config in the build machine. Definitely not in this code. |
MylesBorins commented May 3, 2017
only failure is https://ci.nodejs.org/job/node-test-commit-linux/9551/nodes=ubuntu1204-clang341-64/ which is a known failure. LGTM @nodejs/build ... can we deal with this? |
evanlucas commented May 3, 2017
Wow, @mcollina I'm really sorry I actually forgot to land this one today. Is it crucial to get out in this release? If so, I can pull it and rebuild the binaries. |
evanlucas commented May 3, 2017
Actually, scratch that. Landing now. I'll rebuild. This seems to a be a bug we want fixed. |
evanlucas commented May 3, 2017
Landed in f86ca8f3bffced9b94f08180f13fb1844a9edb50. Thanks! |
6 participants
This is a backport of #11876, specifically commit mcollina@9dcf18a.
Ideally this commit can should be easily ported to v6 as well after it has been in the wild for a while.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
stream