stream: adjust src hwm when pipelining

[ronag]

No description provided.

[ronag]

@nodejs/startup @addaleax

[mcollina]

A doc change is needed. This also change/simplify the pipe logic and I like it.

I'm not sure I would backport it immediately to LTS, but I'm not sure it's strictly semver-major either.

[mcollina]

why this?

[ronag]

There is a timing difference due to how streams are resumed with 'readable' event.

[nodejs-github-bot]

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

[ronag]

@nodejs/streams

[mcollina]

lgtm

[lpinca]

What happens if the source is already piped to another destination via source.pipe(destination)? Does this allow the same source to be piped to multiple destinations?

[ronag]

Combining pipe and read is not a good idea... so no

[mcollina]

What happens if the source is already piped to another destination via source.pipe(destination)? Does this allow the same source to be piped to multiple destinations?

It will work but the consumption will be driven by pipeline as the 'readable' takes precedence.

@ronag could you add a test for this case?

[lpinca]

It will work but the consumption will be driven by pipeline as the 'readable' takes precedence.

If my understanding is correct, then consumption will be driven by the fastest destination, not the slowest, potentially messing up backpressure handling, right?

[mcollina]

If my understanding is correct, then consumption will be driven by the fastest destination, not the slowest, potentially messing up backpressure handling, right?

No, not really. The same happens if you mix async iterators and .pipe(). AsyncIterators use .read() and they drive the consumption of the source data.

Unfortuunately this is the only way .read() can work reliably. See #18994 and #18058.

[ronag]

What happens if the source is already piped to another destination via source.pipe(destination)? Does this allow the same source to be piped to multiple destinations?

It will work but the consumption will be driven by pipeline as the 'readable' takes precedence.

@ronag could you add a test for this case?

I don't really see what a test here contributes? It uses an existing api.

[addaleax]

Fwiw, I marked this semver-major because it’s a big breaking change to stop using .pipe() here – my understanding is that this breaks piping to multiple destinations completely, and even if not, this is missing pipe/unpipe events, it’s breaking manual src.emit('data') calls, etc. (not saying that people should rely on this – but reality is that they do).

either we make pipeline use read or make async gen use pipe

The latter sounds a lot safer to me.

[ronag]

IMO if we merge this we are moving towards some from of deprecation of pipe.

If we don’t merge this we’re in this inconsistent state where sometimes we use pipe and sometimes not and the situation is quite unpredictable for our users.

I think we have to choose which api we recommend and stick with it in core at least.

readable is simpler
pipe has better compat and maybe more feature rich in terms of multiple consumers?

@mcollina wdyt?

[nodejs-github-bot]

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

[ronag]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2793/

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

This needs a rebase.

[aduh95]

This needs a rebase, if we want to still merge it.