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.

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

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

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented May 2, 2017

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

stream

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 <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mcollina mcollina added lts-watch-v6.x stream Issues and PRs related to the stream subsystem. labels May 2, 2017
@nodejs-github-bot nodejs-github-bot added stream Issues and PRs related to the stream subsystem. v7.x labels May 2, 2017
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

rubber stamp LGTM if CI is green

@mcollina
Copy link
Member Author

mcollina commented May 2, 2017

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

Thanks!

@mcollina
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Actually, scratch that. Landing now. I'll rebuild. This seems to a be a bug we want fixed.

@evanlucas
Copy link
Contributor

Landed in f86ca8f3bffced9b94f08180f13fb1844a9edb50. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants