stream: writableNeedDrain by ronag · Pull Request #35348 · nodejs/node

ronag · 2020-09-25T16:40:46Z

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 · 2020-09-25T16:40:50Z

Review requested:

@nodejs/http
@nodejs/net

ronag · 2020-09-25T16:40:54Z

Needs tests.

Don't write to a stream which already has a full buffer. Fixes: nodejs#35341

Fixes: #441 Refs: nodejs/node#35348 Refs: nodejs/node#35341

mcollina

lgtm

doc/api/stream.md

lib/internal/streams/readable.js

ronag · 2020-10-10T11:00:09Z

@nodejs/streams

nodejs-github-bot · 2020-10-10T11:22:18Z

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

ronag · 2020-10-10T12:34:44Z

lib/internal/streams/readable.js

+  if (dest.writableNeedDrain === true) {
+    if (state.flowing) {
+      src.pause();
+    }


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

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

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

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.

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 · 2020-10-12T10:47:01Z

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

nodejs-github-bot · 2020-10-12T15:58:42Z

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

lundibundi · 2020-10-12T15:58:21Z

lib/internal/streams/readable.js

  // Start the flow if it hasn't been started already.
-  if (!state.flowing) {
+
+  if (dest.writableNeedDrain === true) {


Nit: why not:

Suggested change

if (dest.writableNeedDrain === true) {

if (dest.writableNeedDrain) {

lundibundi · 2020-10-12T16:08:53Z

lib/internal/streams/readable.js

+  if (dest.writableNeedDrain === true) {
+    if (state.flowing) {
+      src.pause();
+    }


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

Fixes: #441 Refs: nodejs/node#35348 Refs: nodejs/node#35341

aduh95 · 2020-11-09T18:06:21Z

@ronag is this ready to land?

mcollina

lgtm

aduh95 · 2020-11-10T09:44:37Z

Landed in dd0f8f1

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>

Fixes: #441 Refs: nodejs/node#35348 Refs: nodejs/node#35341

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>

ronag · 2020-12-18T11:01:57Z

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

ronag · 2020-12-20T09:30:05Z

This requires bug fix in #36563

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>

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 force-pushed the stream-writable-need-drain branch from 8d1c665 to deabf75 Compare September 25, 2020 16:41

ronag mentioned this pull request Sep 25, 2020

writableNeedDrain nodejs/undici#441

Closed

ronag force-pushed the stream-writable-need-drain branch from deabf75 to 3976f6a Compare September 25, 2020 16:46

stream: writableNeedDrain

591616f

Don't write to a stream which already has a full buffer. Fixes: nodejs#35341

ronag force-pushed the stream-writable-need-drain branch from 3976f6a to 591616f Compare September 25, 2020 16:46

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

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

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

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

fix: Client.stream writableNeedDrain

3cb9234

Fixes: #441 Refs: nodejs/node#35348 Refs: nodejs/node#35341

mcollina approved these changes Sep 26, 2020

View reviewed changes

addaleax reviewed Sep 26, 2020

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

mcollina reviewed Sep 26, 2020

View reviewed changes

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

fixup: docs

0811886

ronag force-pushed the stream-writable-need-drain branch from 386afdf to 0811886 Compare October 9, 2020 10:27

fixup

d9d8e86

ronag marked this pull request as ready for review October 10, 2020 10:59

ronag requested review from addaleax, lpinca and mcollina October 10, 2020 11:00

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020

ronag added 2 commits October 10, 2020 14:25

fixup

dfb77b4

fixup

3dd02b3

ronag commented Oct 10, 2020

View reviewed changes

lundibundi reviewed Oct 12, 2020

View reviewed changes

ronag added a commit to nodejs/undici that referenced this pull request Nov 2, 2020

fix: Client.stream writableNeedDrain

76010a2

Fixes: #441 Refs: nodejs/node#35348 Refs: nodejs/node#35341

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

mcollina approved these changes Nov 9, 2020

View reviewed changes

benjamingr approved these changes Nov 9, 2020

View reviewed changes

aduh95 closed this Nov 10, 2020

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

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

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

fix: Client.stream writableNeedDrain

1be85c2

Fixes: #441 Refs: nodejs/node#35348 Refs: nodejs/node#35341

BethGriggs added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 10, 2020

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 mentioned this pull request Dec 18, 2020

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

Closed

1 task

ronag added dont-land-on-v10.x labels Dec 18, 2020

ronag removed the dont-land-on-v14.x label Dec 20, 2020

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

kanongil mentioned this pull request Jul 5, 2023

Another pipe() deadlock since v14.17 #48666

Closed

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

Uh oh!

Conversation

ronag commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Sep 25, 2020

Uh oh!

ronag commented Sep 25, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ronag commented Oct 10, 2020

Uh oh!

nodejs-github-bot commented Oct 10, 2020

Uh oh!

ronag Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

ronag Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

HomerSp Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 12, 2020

Uh oh!

nodejs-github-bot commented Oct 12, 2020

Uh oh!

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Nov 9, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Nov 10, 2020

Uh oh!

ronag commented Dec 18, 2020

Uh oh!

ronag commented Dec 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ronag commented Sep 25, 2020 •

edited

Loading

ronag Oct 12, 2020 •

edited

Loading