Repository navigation
fs: respect setDefaultHighWaterMark in ReadStream - #66956
hamzakargin wants to merge 1 commit into
Conversation
|
Review requested:
|
ReadStream ignored stream.setDefaultHighWaterMark() and always used 64 KiB. Use the user-set value when there is one, and keep 64 KiB otherwise so Windows does not drop to the 16 KiB stream default. Fixes: nodejs#66937 Signed-off-by: Hamza Kargın <hamzakargin@outlook.com>
e98b952 to
dfa7cb1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66956 +/- ##
==========================================
- Coverage 90.60% 90.59% -0.02%
==========================================
Files 797 797
Lines 278276 278334 +58
Branches 53498 53516 +18
==========================================
+ Hits 252133 252148 +15
- Misses 16506 16555 +49
+ Partials 9637 9631 -6
🚀 New features to boost your workflow:
|
|
Thanks for the pr. I tested it locally, and the issue is fixed. I have two questions: After setDefaultHighWaterMark(false, 0), fs.createReadStream reads nothing: const { setDefaultHighWaterMark } = require('stream');
const fs = require('fs');
setDefaultHighWaterMark(false, 0);
let n = 0;
fs.createReadStream('README.md')
.on('data', (d) => n += d.length)
.on('end', () => console.log('read', n, 'size', fs.statSync('README.md').size));
// read 0 size 42695Can this PR fall back to 64 KiB for 0? Or should we open a new issue? This PR changes fs read streams for code that uses setDefaultHighWaterMark. Should it be semver-major? |
|
Thanks for testing!
|
fs.createReadStream()andfilehandle.createReadStream()ignorestream.setDefaultHighWaterMark()becauseReadStreamhardcodes a 64 KiBdefault.
ReadStreamnow uses the value set withsetDefaultHighWaterMark()andfalls back to 64 KiB when it was never called. Using
getDefaultHighWaterMark()directly would lower the default to 16 KiB onWindows, so the fallback stays as it is.
Only code that calls
setDefaultHighWaterMark()sees a change.Fixes: #66937
cc @mertcanaltin
I used an AI coding assistant for this change. I verified it myself: reproduced the bug, confirmed the new test fails without the fix, and ran the related tests and linters locally.