node icon indicating copy to clipboard operation
node copied to clipboard

fs: enable chunked reading for large files in readFileHandle

Open mertcanaltin opened this issue 1 year ago β€’ 3 comments

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

#55864

mertcanaltin avatar Nov 27 '24 13:11 mertcanaltin

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

mertcanaltin avatar Nov 27 '24 14:11 mertcanaltin

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 88.54%. Comparing base (5e677d6) to head (2036e4a). :warning: Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56022      +/-   ##
==========================================
+ Coverage   88.52%   88.54%   +0.01%     
==========================================
  Files         703      703              
  Lines      208585   208584       -1     
  Branches    40225    40218       -7     
==========================================
+ Hits       184650   184682      +32     
+ Misses      15953    15912      -41     
- Partials     7982     7990       +8     
Files with missing lines Coverage Ξ”
lib/fs.js 98.15% <ΓΈ> (-0.04%) :arrow_down:
lib/internal/fs/promises.js 98.15% <100.00%> (+0.01%) :arrow_up:

... and 31 files with indirect coverage changes

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

codecov[bot] avatar Nov 27 '24 15:11 codecov[bot]

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

mertcanaltin avatar Nov 30 '24 10:11 mertcanaltin

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.

BridgeAR avatar Dec 09 '24 12:12 BridgeAR

just wondering what is the next action here - @BridgeAR

gireeshpunathil avatar Jan 28 '25 03:01 gireeshpunathil

@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 :)

BridgeAR avatar Jan 28 '25 20:01 BridgeAR

@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

mertcanaltin avatar Jan 28 '25 21:01 mertcanaltin

@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.

gireeshpunathil avatar Jan 29 '25 02:01 gireeshpunathil

hello, I wonder if there is an update here @BridgeAR

mertcanaltin avatar Feb 12 '25 16:02 mertcanaltin

this test was successful on my local linux machine

mertcanaltin avatar Mar 12 '25 05:03 mertcanaltin

Hello everyone, I wonder if there is an update about this place @jasnell @BridgeAR πŸ™

mertcanaltin avatar Apr 08 '25 08:04 mertcanaltin

Added baking for lts label to wait backporting this.

mcollina avatar Dec 26 '25 09:12 mcollina

CI: https://ci.nodejs.org/job/node-test-pull-request/70603/

nodejs-github-bot avatar Dec 26 '25 09:12 nodejs-github-bot