GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: fix pipe deadlock when starting with needDrain by ronag · Pull Request #36563 · nodejs/node

Conversation

@ronag

@ronag ronag commented Dec 18, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Related Issues

Fixes: #36544

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and weedz reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@ronag
stream: fix pipe deadlock when starting with needDrain
0af4317 
Fixes: nodejs#36544
@ronag ronag added dont-land-on-v10.x labels Dec 18, 2020
@ronag

ronag commented Dec 18, 2020

Copy link
Copy Markdown
Member Author

Missing test
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag mentioned this pull request Dec 18, 2020
doc: Piping multiple streams to the same Writable stream might not end #36544
Closed
1 task
@HomerSp

HomerSp commented Dec 18, 2020

Copy link
Copy Markdown

I've updated your test slightly to check for the deadlock: https://gist.github.com/bea83eb0dcbd0aea3b2689c2cd91bb38
Now it will check for the initial pause, which will happen without the fix, and also for end, which won't happen without it. I also set highWaterMark to 1 so it'll happen immediately.
ronag reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup
0996479
@ronag
ronag force-pushed the stream-needDrain branch from 62844f2 to 0996479 Compare December 18, 2020 16:17
@HomerSp

HomerSp commented Dec 18, 2020

Copy link
Copy Markdown

Seems like we may need to use common.mustCallAtLeast(1) instead of common.mustCall() because the latter requires it to be called exactly once, and it looks like the build system calls pause twice.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup
fcc9351
@ronag
ronag requested a review from mcollina December 18, 2020 18:43
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 18, 2020
@ronag

ronag commented Dec 18, 2020

Copy link
Copy Markdown
Member Author

@nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Dec 18, 2020

Copy link
Copy Markdown
Member Author

and it looks like the build system calls pause twice.

This was also a separate bug.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag requested a review from lpinca December 18, 2020 18:51
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 18, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 18, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35000/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup
b37e0df
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 18, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 18, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 18, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35003/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Dec 18, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
HomerSp reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ronag ronag added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 18, 2020
@HomerSp

HomerSp commented Dec 19, 2020

Copy link
Copy Markdown

and it looks like the build system calls pause twice.

This was also a separate bug.

It's because pause is called when it's finishing (in unpipe) as well as in the pipe call - so your latest test code looks good to me.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Dec 19, 2020
View reviewed changes
@ronag

ronag commented Dec 20, 2020

Copy link
Copy Markdown
Member Author

Landed in ab895bd
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag closed this Dec 20, 2020
ronag added a commit that referenced this pull request Dec 20, 2020
@ronag
stream: fix pipe deadlock when starting with needDrain
ab895bd 
Fixes: #36544

PR-URL: #36563
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@ronag ronag removed the dont-land-on-v14.x label Dec 20, 2020
@ronag ronag mentioned this pull request Dec 20, 2020
stream: writableNeedDrain #35348
Closed
4 tasks done
@HomerSp HomerSp mentioned this pull request Dec 20, 2020
writeFileListToStream might hang when the writable stream becomes full (asar does not work between Node 15.2.0 and 15.4.0) electron/asar#210
Closed
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Dec 20, 2020
Create a new pull request javascript-indonesias/node#258
Merged
4 tasks
targos pushed a commit that referenced this pull request Dec 21, 2020
@ronag @targos
stream: fix pipe deadlock when starting with needDrain
3f33d0b 
Fixes: #36544

PR-URL: #36563
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos mentioned this pull request Dec 22, 2020
v15.5.0 release proposal #36597
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@ronag @targos
stream: fix pipe deadlock when starting with needDrain
4793f16 
Fixes: #36544

PR-URL: #36563
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@benjamingr benjamingr mentioned this pull request Jan 31, 2022
Readable.pipe() behaves inconsistently resuming (or not) the source #41785
Closed
@kanongil kanongil mentioned this pull request Jul 5, 2023
Another pipe() deadlock since v14.17 #48666
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@lpinca lpinca lpinca approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

doc: Piping multiple streams to the same Writable stream might not end

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@ronag @HomerSp @nodejs-github-bot @mcollina @lpinca
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.