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: writableNeedDrain #35348

Closed
wants to merge 5 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 25, 2020

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 25, 2020
@ronag
Copy link
Member Author

ronag commented Sep 25, 2020

Needs tests.

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

Fixes: nodejs#35341
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
ronag added a commit to nodejs/undici that referenced this pull request Sep 25, 2020
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

doc/api/stream.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-writable-need-drain branch from 386afdf to 0811886 Compare October 9, 2020 10:27
@ronag ronag marked this pull request as ready for review October 10, 2020 10:59
@ronag
Copy link
Member Author

ronag commented Oct 10, 2020

@nodejs/streams

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020
@nodejs-github-bot
Copy link
Collaborator

if (dest.writableNeedDrain === true) {
if (state.flowing) {
src.pause();
}
Copy link
Member Author

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?

Copy link
Member

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...

Copy link
Member Author

@ronag ronag Oct 12, 2020

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.

Copy link
Member

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.

Copy link

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.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@@ -801,7 +801,12 @@ Readable.prototype.pipe = function(dest, pipeOpts) {
dest.emit('pipe', src);

// Start the flow if it hasn't been started already.
if (!state.flowing) {

if (dest.writableNeedDrain === true) {
Copy link
Member

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) {

if (dest.writableNeedDrain === true) {
if (state.flowing) {
src.pause();
}
Copy link
Member

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...

ronag added a commit to nodejs/undici that referenced this pull request Nov 2, 2020
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 8, 2020
@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2020

@ronag is this ready to land?

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

@aduh95
Copy link
Contributor

aduh95 commented Nov 10, 2020

Landed in dd0f8f1

@aduh95 aduh95 closed this Nov 10, 2020
aduh95 pushed a commit that referenced this pull request Nov 10, 2020
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Nov 10, 2020
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request Nov 10, 2020
ronag added a commit to nodejs/undici that referenced this pull request Nov 12, 2020
ronag added a commit to nodejs/undici that referenced this pull request Nov 12, 2020
ronag added a commit to nodejs/undici that referenced this pull request Nov 13, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 10, 2020
@ronag
Copy link
Member Author

ronag commented Dec 18, 2020

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

@ronag
Copy link
Member Author

ronag commented Dec 20, 2020

This requires bug fix in #36563

targos pushed a commit that referenced this pull request May 1, 2021
Don't write to a stream which already has a full buffer.

Fixes: #35341

PR-URL: #35348
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

streams: Writable expose needDrain
10 participants