stream: adjust src hwm when pipelining #40751
Conversation
|
@nodejs/startup @addaleax |
There was a problem hiding this comment.
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.
| const cb = common.mustCall((err) => { | ||
| assert.strictEqual(err.name, 'AbortError'); | ||
| assert.strictEqual(res, '012345'); | ||
| assert.strictEqual(res, '01234'); |
There was a problem hiding this comment.
why this?
4d68da2 to
3d0d042
Compare
|
@nodejs/streams |
|
What happens if the source is already piped to another destination via |
|
Combining pipe and read is not a good idea... so no |
It will work but the consumption will be driven by pipeline as the @ronag could you add a test for this case? |
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 Unfortuunately this is the only way |
I don't really see what a test here contributes? It uses an existing api. |
|
Fwiw, I marked this semver-major because it’s a big breaking change to stop using
The latter sounds a lot safer to me. |
|
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 @mcollina wdyt? |
|
This needs a rebase. |
|
This needs a rebase, if we want to still merge it. |
No description provided.