fs: enable chunked reading for large files in readFileHandle #56022

mertcanaltin · 2024-11-27T13:49:54Z

Added chunked reading support to readFileHandle to handle files larger than 2 GiB, resolving size limitations while preserving existing functionality.

#55864

mertcanaltin · 2024-11-27T14:00:13Z

I wonder if I should apply this sorting in fs.js, right now I just did it for promises,

in addition there are still places that use ERR_FS_FILE_TOO_LARGE I did not delete this message so as not to break them

codecov · 2024-11-27T15:58:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (8a5a849) to head (2160d30).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56022      +/-   ##
==========================================
+ Coverage   90.22%   90.24%   +0.01%     
==========================================
  Files         629      629              
  Lines      184847   184844       -3     
  Branches    36207    36207              
==========================================
+ Hits       166780   166813      +33     
+ Misses      11015    11011       -4     
+ Partials     7052     7020      -32

Files with missing lines	Coverage Δ
lib/fs.js	`98.27% <ø> (-0.01%)`	⬇️
lib/internal/fs/promises.js	`98.25% <100.00%> (+<0.01%)`	⬆️

... and 27 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 · 2024-11-30T10:20:59Z

I removed the limit test because the limit for reading large files exceeding the GiB limit with fs.readFile has been removed.

lib/internal/fs/promises.js

BridgeAR · 2024-12-09T12:35:57Z

I added the tsc label to discuss, if we want to allow users to read such big files into memory, or if it would be better to try to point out streams instead.

mcollina

lgtm

BridgeAR

The error is removed from the promise version but it's missing the callback readFile implementation. The error itself would not be needed anymore due to that and also has to be removed.
This has to be addressed before we could land this.

We discussed in the TSC meeting that it's not a good idea to read beyond that, while it's acceptable for some cases.
We also discussed around warning when reaching that limit instead. We did not yet have consensus around it, but we'll discuss it again next week to finish the decision for that.

gireeshpunathil · 2025-01-28T03:10:27Z

just wondering what is the next action here - @BridgeAR

BridgeAR · 2025-01-28T20:59:50Z

@gireeshpunathil I believe you wanted to think about the warning again.

I kept my change request since the implementation should also include the callback version next to the warning. That's currently missing :)

mertcanaltin · 2025-01-28T21:47:24Z

@gireeshpunathil I believe you wanted to think about the warning again.

I kept my change request since the implementation should also include the callback version next to the warning. That's currently missing :)

thanks a lot for your comments, unfortunately I saw this place late and now I sent a commit for the callback version

BridgeAR

The original reason for the limit was AFAIK something about some systems on some versions not being able to read that file size. If that's the case, we'd have to handle chunking on our side. I am just not certain if that still applies or if it's a legacy issue.

Please also add the warning instead of the error.

test/parallel/test-fs-readfile.js

test/parallel/test-fs-promises-file-handle-readFile.js

gireeshpunathil · 2025-01-29T02:32:59Z

@gireeshpunathil I believe you wanted to think about the warning again.

I kept my change request since the implementation should also include the callback version next to the warning. That's currently missing :)

I thought we are going to continue the discussion in the TSC on the necessity of the warning, as we didn't converge on that IIRC.

BridgeAR

Please add the warning to all occurrences and test for the warning.

lib/internal/fs/promises.js

test/parallel/test-fs-promises-file-handle-readFile.js

lib/internal/fs/promises.js

mcollina

lgtm

mertcanaltin · 2025-03-12T05:52:28Z

this test was successful on my local linux machine

jasnell · 2025-03-13T19:11:40Z

lib/internal/fs/promises.js

+    if (size > kIoMaxLength) {
+      process.emitWarning(
+        `Warning: Detected \`fs.readFile()\` to read a huge file in memory (${size} bytes). Please consider using \`fs.createReadStream()\` instead to minimize memory overhead and increase the performance.`,
+      );


It would likely be worthwhile to assign a warning code to this warning so that it can be suppressed with the --dsable-warning CLI flag.

Thanks, I try to use the ERR_FS_FILE_TOO_LARGE error code, do you think this is true?

jasnell · 2025-03-13T19:14:06Z

test/parallel/test-fs-promises-file-handle-readFile.js

+
+  function createVirtualLargeFile() {
+    return Buffer.alloc(LARGE_FILE_SIZE, 'A');
+  }


This is quite a large allocation that may fail on some our more resource challenge builders. I believe there's a utility in test/common for skipping if there's not enough available memory? Can't recall exactly what it is but we likely should be safer here.

thanks I find this method enoughTestMem

jasnell · 2025-03-13T19:14:46Z

test/parallel/test-fs-promises-file-handle-readFile.js

+
+  const virtualFile = createVirtualLargeFile();
+
+  await writeFile(FILE_PATH, virtualFile);


It's also possible that the FS on test runners could completely run out of space with this large of a file

I did a check for this place too, thanks for the suggestion

Co-authored-by: Ruben Bridgewater <[email protected]>

mertcanaltin · 2025-04-08T08:11:53Z

Hello everyone, I wonder if there is an update about this place @jasnell @BridgeAR 🙏

mertcanaltin requested a review from joyeecheung November 27, 2024 13:50

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Nov 27, 2024

mertcanaltin requested review from anonrig, JakobJingleheimer and RafaelGSS November 27, 2024 13:52

mertcanaltin force-pushed the dev-55864 branch from 19dc0c4 to 6c85d68 Compare November 30, 2024 10:34

mertcanaltin added the review wanted PRs that need reviews. label Nov 30, 2024

mertcanaltin force-pushed the dev-55864 branch from 6c85d68 to ed82387 Compare November 30, 2024 11:20

pmarchini reviewed Dec 9, 2024

View reviewed changes

lib/internal/fs/promises.js Show resolved Hide resolved

BridgeAR added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Dec 9, 2024

mhdawson mentioned this pull request Dec 9, 2024

Node.js Technical Steering Committee (TSC) Meeting 2024-12-11 nodejs/TSC#1662

Closed

mcollina approved these changes Dec 11, 2024

View reviewed changes

BridgeAR requested changes Dec 11, 2024

View reviewed changes

mhdawson mentioned this pull request Dec 16, 2024

Node.js Technical Steering Committee (TSC) Meeting 2024-12-18 nodejs/TSC#1665

Closed

mhdawson mentioned this pull request Jan 6, 2025

Node.js Technical Steering Committee (TSC) Meeting 2025-01-08 nodejs/TSC#1670

Closed

BridgeAR removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 8, 2025

BridgeAR requested changes Jan 28, 2025

View reviewed changes

test/parallel/test-fs-readfile.js Show resolved Hide resolved

test/parallel/test-fs-promises-file-handle-readFile.js Show resolved Hide resolved

mertcanaltin closed this Jan 29, 2025

mertcanaltin reopened this Jan 29, 2025

mertcanaltin requested a review from BridgeAR January 30, 2025 18:52

mertcanaltin requested a review from addaleax February 12, 2025 16:26

BridgeAR requested changes Mar 11, 2025

View reviewed changes

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

test/parallel/test-fs-promises-file-handle-readFile.js Outdated Show resolved Hide resolved

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

mertcanaltin force-pushed the dev-55864 branch from b685cee to 461bb5b Compare March 11, 2025 12:39

mertcanaltin requested a review from BridgeAR March 11, 2025 12:39

mcollina approved these changes Mar 11, 2025

View reviewed changes

mcollina self-requested a review March 12, 2025 06:12

jasnell reviewed Mar 13, 2025

View reviewed changes

Mert Can Altin and others added 12 commits March 16, 2025 11:51

fs: enable chunked reading for large files in readFileHandle

b1e4ac0

revert lint

4e6fa90

revert lint

fab698d

add large file support to callback-based readFile implementation

9ea02ac

add support for chunked reading in fs/promises

83405a7

undo the link

0675e3b

undo the link

45bf4de

Update lib/internal/fs/promises.js

553af67

Co-authored-by: Ruben Bridgewater <[email protected]>

use emitWarning

14c4288

keep old variable

925c923

update large file limit

c9f73e6

delete console import

2de146f

mertcanaltin force-pushed the dev-55864 branch from 62fea9d to 2de146f Compare March 16, 2025 09:27

mertcanaltin added 4 commits March 16, 2025 12:29

use error code

77f5e86

skips the test if there is no space

7fbb376

lint

ff0137f

use tmpdir

2160d30

mertcanaltin requested a review from jasnell March 16, 2025 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: enable chunked reading for large files in readFileHandle #56022

fs: enable chunked reading for large files in readFileHandle #56022

mertcanaltin commented Nov 27, 2024

mertcanaltin commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

mertcanaltin commented Nov 30, 2024

BridgeAR commented Dec 9, 2024

mcollina left a comment

BridgeAR left a comment

gireeshpunathil commented Jan 28, 2025

BridgeAR commented Jan 28, 2025

mertcanaltin commented Jan 28, 2025

BridgeAR left a comment

gireeshpunathil commented Jan 29, 2025

BridgeAR left a comment

This comment was marked as duplicate.

mcollina left a comment

mertcanaltin commented Mar 12, 2025

jasnell Mar 13, 2025

mertcanaltin Mar 16, 2025 •

edited

Loading

jasnell Mar 13, 2025

mertcanaltin Mar 16, 2025

jasnell Mar 13, 2025

mertcanaltin Mar 16, 2025

mertcanaltin commented Apr 8, 2025


		const virtualFile = createVirtualLargeFile();

		await writeFile(FILE_PATH, virtualFile);

fs: enable chunked reading for large files in readFileHandle #56022

Are you sure you want to change the base?

fs: enable chunked reading for large files in readFileHandle #56022

Conversation

mertcanaltin commented Nov 27, 2024

mertcanaltin commented Nov 27, 2024 • edited Loading

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

mertcanaltin commented Nov 30, 2024

BridgeAR commented Dec 9, 2024

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Jan 28, 2025

BridgeAR commented Jan 28, 2025

mertcanaltin commented Jan 28, 2025

BridgeAR left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Jan 29, 2025

BridgeAR left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

mcollina left a comment

Choose a reason for hiding this comment

mertcanaltin commented Mar 12, 2025

jasnell Mar 13, 2025

Choose a reason for hiding this comment

mertcanaltin Mar 16, 2025 • edited Loading

Choose a reason for hiding this comment

jasnell Mar 13, 2025

Choose a reason for hiding this comment

mertcanaltin Mar 16, 2025

Choose a reason for hiding this comment

jasnell Mar 13, 2025

Choose a reason for hiding this comment

mertcanaltin Mar 16, 2025

Choose a reason for hiding this comment

mertcanaltin commented Apr 8, 2025

mertcanaltin commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

mertcanaltin Mar 16, 2025 •

edited

Loading