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.

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

stream: emit 'pause' on unpipe #32476

Closed
wants to merge 1 commit into from
Closed

stream: emit 'pause' on unpipe #32476

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 24, 2020

unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: #32470

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

@ronag
stream: emit 'pause' on unpipe
6fce298 
unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: nodejs#32470
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Mar 24, 2020
@ronag ronag requested review from addaleax and mcollina March 25, 2020 12:04
addaleax
addaleax approved these changes Mar 25, 2020
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.

I think this might also fix #32291?

mcollina
mcollina approved these changes Mar 25, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@bricss bricss mentioned this pull request Mar 25, 2020
Merged
@nodejs-github-bot
Copy link
Collaborator

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

@MylesBorins
Copy link
Contributor

Should we fast track this and get it included in v13.12.0? I don't know if it should so this should not count as 1 of the 2 +1s

@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 26, 2020
@ronag
Copy link
Member Author

ronag commented Mar 26, 2020 •
edited
Loading

This doesn't have green CI yet. Maybe flaky test (test.parallel/test-tls-root-certificates)? I'll do a rerun/resume once it's complete (osx always takes time).

I seem to be encountering quite a lot of flakiness nowadays.

@MylesBorins
Copy link
Contributor

test.parallel/test-tls-root-certificates is failing in the js suite for a variety of PRs and seems to be flaky on master right now. Unsure if this is an infrastructure issue or not.

https://github.com/nodejs/build/issues/2252

@ronag
Copy link
Member Author

ronag commented Mar 26, 2020

@MylesBorins Does that mean I can land this on master without green CI or should I keep trying until green CI is achieved?

@MylesBorins
Copy link
Contributor

@ronag kicked off CI again. We should try and get green CI

@nodejs-github-bot
Copy link
Collaborator

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

MylesBorins pushed a commit that referenced this pull request Mar 26, 2020
@ronag @MylesBorins
stream: emit 'pause' on unpipe
0d0f151 
unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: #32470

PR-URL: #32476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins
Copy link
Contributor

We did it!!!

Landed in 0d0f151

MylesBorins pushed a commit that referenced this pull request Mar 26, 2020
@ronag @MylesBorins
stream: emit 'pause' on unpipe
56da8df 
unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: #32470

PR-URL: #32476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hassaanp hassaanp hassaanp approved these changes

@addaleax addaleax addaleax approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. 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.

Readable Pause event not triggered when unpipe causes the pause
7 participants
@ronag @nodejs-github-bot @MylesBorins @mcollina @addaleax @hassaanp @targos