nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107k stars 29.28k forks source link

`stream.Readable.toWeb(readable)` pauses the original `readable` stream #45545

Closed cola119 closed 2 months ago

cola119 commented 1 year ago

Version

v19.1.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

// index.mjs
import { Stream } from "node:stream";

function* generate() {
  yield "hello";
  yield "streams";
}
const readable = Stream.Readable.from(generate());

const webReadable = Stream.Readable.toWeb(readable);

readable.on("data", (chunk) => {
  console.log(chunk);
});

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

$ node index.mjs
hello
streams

What do you see instead?

$ node index.mjs
hello

Additional information

This is because the newReadableStreamFromStreamReadable function called during toWeb pauses the original readable stream at the following:

https://github.com/nodejs/node/blob/4bee69a8c4a699c23342977b670ae90b870ac00d/lib/internal/webstreams/adapters.js#L426-L429

Is this a bug or is this an intentional behavior that has a side effect on the original stream?

cola119 commented 1 year ago

Maybe the document mentions something related? https://nodejs.org/api/stream.html#choose-one-api-style

The Readable stream API evolved across multiple Node.js versions and provides multiple methods of consuming stream data. In general, developers should choose one of the methods of consuming data and should never use multiple methods to consume data from a single stream. Specifically, using a combination of on('data'), on('readable'), pipe(), or async iterators could lead to unintuitive behavior.

jakecastelli commented 3 months ago

I think this is a side effect on the origin stream, IMO either we should not see "hello" or we should not pause the original readable stream.

Could clone be a good candidate for it as well? like here Update: I think the original stream should be paused based on my understanding of the whatgw spec. @mcollina 👀

mcollina commented 2 months ago

this is intentional.