stream: always reset awaitDrain when emitting data #18516
Conversation
nodejs-github-bot
added
the
stream
addaleax
force-pushed
the
stream-pipe-simplify
branch
from
3c9d54f to
8a2f7e3
Compare
|
@addaleax my gut feeling is that this will disable backpressure in some cases when doing multi destination pipes. let me investigate today. i agree we should clean up the awaitDrain stuff tho to fix that read while pipeing issue. |
|
I would really love to get rid to |
|
@addaleax actually this looks really good. i'd still like to merge the other PR as well, to not emit data while paused in flowing mode. |
|
@addaleax we can prob get rid of this as well with your PR https://github.com/addaleax/node/blob/8a2f7e327331cf1b50550292396136a9e25bc78f/lib/_stream_readable.js#L625-L632 |
@mafintosh This PR only refactors on the readable side of the pipe, so I’m not sure it should have any effect on that part … it looks like we didn’t have a test for the condition in that code though? :o I’ve added a test, I’d be glad if you could take a look at that too! |
|
This needs a rebase. Otherwise it could probably land right away. |
addaleax
force-pushed
the
stream-pipe-simplify
branch
from
82b14c9 to
262601f
Compare
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.
By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).
PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#18516 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax
force-pushed
the
stream-pipe-simplify
branch
from
262601f to
ee67dd0
Compare
|
Should this be backported to |
PR-URL: #18516 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.
By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).
PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.
By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).
PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#18516 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
The complicated
awaitDrainmachinery can be made a bitslimmer, and more correct, by just resetting the value
each time
stream.emit('data')is called.By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return
.write() === false,we always end up in a consistent state and don’t need to worry
about odd situations (like
dest.write(chunk)emitting more data).Fixes: #18484
Fixes: #18512
Refs: #18515
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
streams /cc @nodejs/streams