stream: writableNeedDrain #35348

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:

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-12T16:09:00Z

lib/internal/streams/readable.js

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


Nit: why not:

Suggested change

if (dest.writableNeedDrain === true) {

if (dest.writableNeedDrain) {

lundibundi · 2020-10-12T16:09:00Z

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 label Sep 25, 2020

ronag force-pushed the stream-writable-need-drain branch from 8d1c665 to deabf75 Compare Sep 25, 2020

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 Sep 25, 2020

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 Sep 25, 2020

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 issue 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 issue 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 issue 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 issue 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 changes

addaleax reviewed Sep 26, 2020

View changes

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

mcollina reviewed Sep 26, 2020

View 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 Oct 9, 2020

fixup

d9d8e86

ronag marked this pull request as ready for review Oct 10, 2020

ronag requested review from mcollina, addaleax and lpinca Oct 10, 2020

ronag added the request-ci label Oct 10, 2020

github-actions bot removed the request-ci label Oct 10, 2020

ronag added 2 commits Oct 10, 2020

fixup

dfb77b4

fixup

3dd02b3

ronag reviewed Oct 10, 2020

View changes

github-actions bot removed the request-ci label Oct 12, 2020

lundibundi reviewed Oct 12, 2020

View changes

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

fix: Client.stream writableNeedDrain

76010a2

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

aduh95 added the author ready label Nov 8, 2020

mcollina approved these changes Nov 9, 2020

View changes

benjamingr approved these changes Nov 9, 2020

View 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 issue 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 issue 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 issue Nov 13, 2020

fix: Client.stream writableNeedDrain

1be85c2

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

BethGriggs added the semver-minor 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 dont-land-on-v12.x dont-land-on-v14.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

nodejs / node Public

stream: writableNeedDrain #35348

stream: writableNeedDrain #35348

ronag commented Sep 25, 2020 •

edited

nodejs-github-bot commented Sep 25, 2020

ronag commented Sep 25, 2020

mcollina left a comment

ronag commented Oct 10, 2020

nodejs-github-bot commented Oct 10, 2020

ronag Oct 10, 2020

lundibundi Oct 12, 2020

ronag Oct 12, 2020 •

edited

lundibundi Oct 12, 2020

HomerSp Dec 18, 2020

nodejs-github-bot commented Oct 12, 2020

nodejs-github-bot commented Oct 12, 2020

lundibundi Oct 12, 2020

lundibundi Oct 12, 2020

aduh95 commented Nov 9, 2020

mcollina left a comment

aduh95 commented Nov 10, 2020

ronag commented Dec 18, 2020

ronag commented Dec 20, 2020

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

nodejs / node Public

stream: writableNeedDrain #35348

stream: writableNeedDrain #35348

Conversation

ronag commented Sep 25, 2020 • edited

Checklist

nodejs-github-bot commented Sep 25, 2020

ronag commented Sep 25, 2020

mcollina left a comment

ronag commented Oct 10, 2020

nodejs-github-bot commented Oct 10, 2020

ronag Oct 10, 2020

Choose a reason for hiding this comment

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

ronag Oct 12, 2020 • edited

Choose a reason for hiding this comment

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

HomerSp Dec 18, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 12, 2020

nodejs-github-bot commented Oct 12, 2020

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

lundibundi Oct 12, 2020

Choose a reason for hiding this comment

aduh95 commented Nov 9, 2020

mcollina left a comment

aduh95 commented Nov 10, 2020

ronag commented Dec 18, 2020

ronag commented Dec 20, 2020

ronag commented Sep 25, 2020 •

edited

ronag Oct 12, 2020 •

edited