Skip to content

stream.PassThrough not draining (from Node 16) #40935

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
davedoesdev opened this issue Nov 23, 2021 · 1 comment
Closed

stream.PassThrough not draining (from Node 16) #40935

davedoesdev opened this issue Nov 23, 2021 · 1 comment
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@davedoesdev
Copy link
Contributor

Version

v16.13.0

Platform

Linux david-Latitude-E6440 5.13.0-21-generic #21-Ubuntu SMP Tue Oct 19 08:59:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

stream

What steps will reproduce the bug?

const { PassThrough } = require('stream');
const pt = new PassThrough({ highWaterMark: 0 });
pt.on('drain', () => console.log('drained'));
console.log(pt.write('hello'));
console.log(pt.writableLength, pt.writableNeedDrain);
console.log(pt.read().toString());
console.log(pt.writableLength, pt.writableNeedDrain);

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

Every time

What is the expected behavior?

On Node 14:

david@david-Latitude-E6440:/tmp$ node --version
v14.18.1
david@david-Latitude-E6440:/tmp$ node repro.js 
false
5 true
drained
hello
0 false

What do you see instead?

On Node 16:

david@david-Latitude-E6440:/tmp$ node --version
v16.13.0
david@david-Latitude-E6440:/tmp$ node repro.js 
false
5 true
hello
5 true

Additional information

No response

@lpinca lpinca added the stream Issues and PRs related to the stream subsystem. label Nov 23, 2021
@davedoesdev
Copy link
Contributor Author

davedoesdev commented Nov 24, 2021

I think this is due to the change to transforming the data on write instead of read.
Therefore the readable length gets to 5 when write() is called, i.e. before read(), and https://github.com/nodejs/node/blame/master/lib/internal/streams/readable.js#L474 doesn't trigger.
Whereas on Node 14, the length is 0 when read() is called so we see length less than watermark.
Perhaps a highWaterMark of 0 isn't going to work going forward?
FYI I'm using it for testing.

ronag added a commit to nxtedition/node that referenced this issue Nov 24, 2021
ronag added a commit to nxtedition/node that referenced this issue Nov 24, 2021
ronag added a commit to nxtedition/node that referenced this issue Nov 24, 2021
@ronag ronag closed this as completed in 37f1dd9 Nov 27, 2021
targos pushed a commit that referenced this issue Nov 29, 2021
Fixes: #40935

PR-URL: #40947
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Jan 30, 2022
Fixes: #40935

PR-URL: #40947
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
Fixes: #40935

PR-URL: #40947
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
kanongil added a commit to kanongil/node-1 that referenced this issue Jul 1, 2022
kanongil added a commit to kanongil/node-1 that referenced this issue Jul 1, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants