Skip to content

fs: respect setDefaultHighWaterMark in ReadStream - #66956

Open
hamzakargin wants to merge 1 commit into
nodejs:mainfrom
hamzakargin:fs-readstream-default-hwm
Open

hamzakargin wants to merge 1 commit into
nodejs:mainfrom
hamzakargin:fs-readstream-default-hwm

Conversation

@hamzakargin

Copy link
Copy Markdown
Contributor

fs.createReadStream() and filehandle.createReadStream() ignore
stream.setDefaultHighWaterMark() because ReadStream hardcodes a 64 KiB
default.

ReadStream now uses the value set with setDefaultHighWaterMark() and
falls back to 64 KiB when it was never called. Using
getDefaultHighWaterMark() directly would lower the default to 16 KiB on
Windows, 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.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams. labels Oct 23, 2026
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>
@hamzakargin
hamzakargin force-pushed the fs-readstream-default-hwm branch from e98b952 to dfa7cb1 Compare October 23, 2026 11:39
@codecov

codecov Bot commented Oct 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.59%. Comparing base (1bffd1d) to head (dfa7cb1).
⚠️ Report is 12 commits behind head on main.

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     
Files with missing lines Coverage Δ
lib/internal/fs/streams.js 93.12% <100.00%> (+0.01%) ⬆️
lib/internal/streams/state.js 98.27% <100.00%> (+0.23%) ⬆️

... and 40 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mertcanaltin

Copy link
Copy Markdown
Member

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 42695

Can 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?

@hamzakargin

Copy link
Copy Markdown
Contributor Author

Thanks for testing!

  1. This isn't new with this PR. An explicit highWaterMark: 0 already reads nothing on current Node (v24.22.0):

    fs.createReadStream('README.md', { highWaterMark: 0 });
    // read 0 size 42695

    ReadStream.prototype._read gets n = 0 and hits if (n <= 0) this.push(null), so the stream ends before reading. Falling back to 64 KiB only for the default would leave the explicit case broken, so I think it should be fixed in _read for both, in a separate issue.

  2. I see it as a fix, since setDefaultHighWaterMark() is documented as setting the default for streams. But it does change the documented 64 * 1024 default for fs read streams, so semver-major is fine with me if collaborators prefer it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs: createReadStream disregards setDefaultHighWaterMark

3 participants