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: writableNeedDrain by ronag · Pull Request #35348 · nodejs/node

Conversation

@ronag

@ronag ronag commented Sep 25, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Don't write to a stream which already has a full buffer.

Fixes: #35341

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Sep 25, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Sep 25, 2020
@ronag

ronag commented Sep 25, 2020

Copy link
Copy Markdown
Member Author

Needs tests.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the stream-writable-need-drain branch from 8d1c665 to deabf75 Compare September 25, 2020 16:41
@ronag ronag mentioned this pull request Sep 25, 2020
writableNeedDrain nodejs/undici#441
Closed
@ronag
ronag force-pushed the stream-writable-need-drain branch from deabf75 to 3976f6a Compare September 25, 2020 16:46
@ronag
stream: writableNeedDrain
591616f 
Don't write to a stream which already has a full buffer.

Fixes: nodejs#35341
@ronag
ronag force-pushed the stream-writable-need-drain branch from 3976f6a to 591616f Compare September 25, 2020 16:46
@ronag ronag mentioned this pull request Sep 25, 2020
fix: Client.stream writableNeedDrain nodejs/undici#442
Merged
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
@ronag
fix: Client.stream writableNeedDrain
cd2e352 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
@ronag
fix: Client.stream writableNeedDrain
3d2085f 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
@ronag
fix: Client.stream writableNeedDrain
1d3eed1 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
@ronag
fix: Client.stream writableNeedDrain
3cb9234 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
mcollina
mcollina approved these changes Sep 26, 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.
All reactions
addaleax
addaleax reviewed Sep 26, 2020
View reviewed changes
Comment thread doc/api/stream.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina reviewed Sep 26, 2020
View reviewed changes
Comment thread lib/internal/streams/readable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup: docs
0811886
@ronag
ronag force-pushed the stream-writable-need-drain branch from 386afdf to 0811886 Compare October 9, 2020 10:27
@ronag
fixup
d9d8e86
@ronag
ronag marked this pull request as ready for review October 10, 2020 10:59
@ronag

ronag commented Oct 10, 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 requested review from addaleax, lpinca and mcollina October 10, 2020 11:00
@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 Oct 10, 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 Oct 10, 2020
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag added 2 commits October 10, 2020 14:25
@ronag
fixup
dfb77b4
@ronag
fixup
3dd02b3
ronag
ronag commented Oct 10, 2020
View reviewed changes
Comment thread lib/internal/streams/readable.js
if (dest.writableNeedDrain === true) {
if (state.flowing) {
src.pause();
}

@ronag ronag Oct 10, 2020

Copy link
Copy Markdown
Member Author

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.

I'm a little unsure how this affects the case where src is already piped to other destinations?

Sorry, something went wrong.

Uh oh!

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

@lundibundi lundibundi Oct 12, 2020

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.

🤔 what about adding the stream to the state.awaitDrainWriters (refs pipeOnDrain + ondata(chunk)). Though that may require copying https://github.com/nodejs/node/pull/35348/files#diff-0117344ddd481d021ad96b9c8eea78a5R741-R747 from ondata...

Sorry, something went wrong.

Uh oh!

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

@ronag ronag Oct 12, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.

I have no idea how that works or what it is intended for.

Sorry, something went wrong.

Uh oh!

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

@lundibundi lundibundi Oct 12, 2020

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.

Well, same 😄, based on the initial look it is a list of writables to wait for a drain event so this case seems fitting.
Let's ping @addaleax @BridgeAR @mscdex as most "recent" collaborators according to git-blame.

Sorry, something went wrong.

Uh oh!

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

@HomerSp HomerSp Dec 18, 2020

Copy link
Copy Markdown

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.

Any chance this can be reverted or done some other way? It seems to cause issues with the source getting stuck in a paused state if you previously fed the output buffer with a lot of data, see eg, electron/asar#210 That module basically just appends a bunch of files to a single binary, but after a random amount of files it gets stuck due to the source being in a paused state.
This could, of course, be easily worked around by adding the writableNeedDrain loop as in the issue above, or adding a resume call to the stream, but I feel like that shouldn't really be necessary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Oct 12, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Oct 12, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lundibundi
lundibundi reviewed Oct 12, 2020
View reviewed changes
Comment thread lib/internal/streams/readable.js
// Start the flow if it hasn't been started already.
if (!state.flowing) {

if (dest.writableNeedDrain === true) {

@lundibundi lundibundi Oct 12, 2020

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.

Nit: why not:

Suggested change
if (dest.writableNeedDrain === true) {
if (dest.writableNeedDrain) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/streams/readable.js
if (dest.writableNeedDrain === true) {
if (state.flowing) {
src.pause();
}

@lundibundi lundibundi Oct 12, 2020

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.

🤔 what about adding the stream to the state.awaitDrainWriters (refs pipeOnDrain + ondata(chunk)). Though that may require copying https://github.com/nodejs/node/pull/35348/files#diff-0117344ddd481d021ad96b9c8eea78a5R741-R747 from ondata...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ronag added a commit to nodejs/undici that referenced this pull request Nov 2, 2020
@ronag
fix: Client.stream writableNeedDrain
76010a2 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 8, 2020
@aduh95

aduh95 commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

@ronag is this ready to land?
ronag reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Nov 9, 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.
All reactions
benjamingr
benjamingr approved these changes Nov 9, 2020
View reviewed changes
@aduh95

aduh95 commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

Landed in dd0f8f1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 closed this Nov 10, 2020
aduh95 pushed a commit that referenced this pull request Nov 10, 2020
@ronag @aduh95
stream: writableNeedDrain
dd0f8f1 
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Nov 10, 2020
@ronag @danielleadams
stream: writableNeedDrain
359a659 
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Nov 10, 2020
v15.2.0 proposal #36055
Merged
ronag added a commit to nodejs/undici that referenced this pull request Nov 12, 2020
@ronag
fix: Client.stream writableNeedDrain
d86c660 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
ronag added a commit to nodejs/undici that referenced this pull request Nov 12, 2020
@ronag
fix: Client.stream writableNeedDrain
03c41a5 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
ronag added a commit to nodejs/undici that referenced this pull request Nov 13, 2020
@ronag
fix: Client.stream writableNeedDrain
1be85c2 
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@ronag @BethGriggs
stream: writableNeedDrain
b520470 
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@BethGriggs BethGriggs added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 10, 2020
@weedz weedz mentioned this pull request Dec 15, 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
@HomerSp HomerSp mentioned this pull request Dec 18, 2020
doc: Piping multiple streams to the same Writable stream might not end #36544
Closed
1 task
@ronag ronag added dont-land-on-v10.x labels Dec 18, 2020
@ronag

ronag commented Dec 18, 2020

Copy link
Copy Markdown
Member Author

Adding don't land labels until #36544 is resolved.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag removed the dont-land-on-v14.x label Dec 20, 2020
@ronag

ronag commented Dec 20, 2020

Copy link
Copy Markdown
Member Author

This requires bug fix in #36563
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request May 1, 2021
@ronag @targos
stream: writableNeedDrain
617f2dc 
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@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
@benjamingr benjamingr benjamingr approved these changes
@lpinca lpinca lpinca approved these changes
@addaleax addaleax Awaiting requested review from addaleax
+2 more reviewers
@HomerSp HomerSp HomerSp left review comments
@lundibundi lundibundi lundibundi left review comments
Reviewers whose approvals may not affect merge requirements

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. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

streams: Writable expose needDrain

Uh oh!

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

10 participants

@ronag @nodejs-github-bot @aduh95 @mcollina @HomerSp @addaleax @benjamingr @lpinca @lundibundi @BethGriggs
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.