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

[weedz]

📗 API Reference Docs Problem

• Version: 15.4.0

• Platform: Linux pop-os 5.8.0-7630-generic 20.10~61c3910-Ubuntu SMP Thu Nov 26 00:10:35 UTC x86_64 x86_64 x86_64 GNU/Linux

• Subsystem: stream

Location

Section of the site where the content exists

Affected URL(s):

• https://nodejs.org/api/stream.html#stream_readable_pipe_destination_options

Description

Concise explanation of the problem

The documentation for readable.pipe() reads:

... The flow of data will be automatically managed so that the destination Writable stream is not overwhelmed by a faster Readable stream.

So when we open a Writable stream and then pipe multiple Readable streams into this Writable stream; the stream becomes full/congested and needs to drain but then we pipe more Readable streams into this stream. This seems to sometimes cause a Readable stream to not end and thus does not pipe its data to the Writable stream.

Bug in electron/asar explaining the issue electron/asar#210.

Since electron/asar does this in basically a for await-loop, we get stuck waiting for a Readable stream which never ends.

Is this expected behavior when piping multiple Readable streams into one Writable stream? Should one check writable.writableNeedDrain before piping a Readable stream?

---

• I would like to work on this issue and
  submit a pull request.

[HomerSp]

This isn't really a problem with the documentation I would say, it seems to be caused by a bug introduced in #35348, lib/internal/streams/readable.js, that causes a pipe to be paused initially when the writable buffer needs draining. The problem is that the readable is just never resumed so it gets stuck and fails.
A simple solution would be to add a drain listener on dest after pausing src, like so: https://gist.github.com/HomerSp/07ec63dacf49aac5f55a62b7c09dc313

Maybe this is supposed to be done automatically, but it just isn't? I can't see anything related to this though, so I'm not sure.

Pinging @ronag and @aduh95

[ronag]

Yea. It's a bug. The drain handler is added lazily in the ondata handler which will never get invoked since it's paused.

The fix should probably be something like:

index a004ce20d0..1afa3a2b29 100644
--- a/lib/internal/streams/readable.js
+++ b/lib/internal/streams/readable.js
@@ -794,6 +794,8 @@ Readable.prototype.pipe = function(dest, pipeOpts) {
   if (dest.writableNeedDrain === true) {
     if (state.flowing) {
       src.pause();
+      ondrain = pipeOnDrain(src, dest);
+      dest.on('drain', ondrain);
     }
   } else if (!state.flowing) {
     debug('pipe resume');

[ronag]

I probably won't have time to sort this until Sunday or Monday so if someone can take the above patch, make a test and open a PR I would encourage that.

EDIT: I think I have time tonight.

[ronag]

@weedz do you have a minimal repro we could use as a basis for a test?

[HomerSp]

@ronag Thanks, that also looks to work. I'll write up a small test, though it is a bit annoying since it's pretty random when it'll happen. Piping a bunch of different sources one after the other should trigger it eventually though.

[ronag]

Opened PR. Still lacking tests though. #36563

[ronag]

@HomerSp I think it's enough to bring dst to need drain state and then pipe an additional readable.