Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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