fs: improve `readFileSync` with file descriptors#49691

anonrig · 2023-09-17T20:47:19Z

This particular change applies to: fs.readFileSync(fs.openSync(__filename), 'utf8')

My local benchmarks are as follows:

fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='undefined' 0.73 % ±1.65% ±2.20% ±2.86% fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='utf8' 0.05 % ±1.74% ±2.32% ±3.02% fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='undefined' *** 62.27 % ±2.82% ±3.77% ±4.95% fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='utf8' *** 60.93 % ±2.72% ±3.64% ±4.78% fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='undefined' 0.22 % ±1.47% ±1.96% ±2.56% fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='utf8' *** 88.18 % ±2.56% ±3.43% ±4.49% fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='undefined' -3.31 % ±4.32% ±5.80% ±7.65% fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='utf8' -3.18 % ±3.58% ±4.79% ±6.30%

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1413

rluvaton · 2023-09-18T06:34:59Z

Benchmark CI:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1397/

bnoordhuis · 2023-09-18T10:16:45Z

Test failures look related.

Correctness issue: file descriptors are int32, not uint32 (also on Windows, because they go through libuv.)

anonrig · 2023-09-18T14:34:02Z

@bnoordhuis We validate file descriptors on lib/fs.js by checking if it is Uint32 (ref: https://github.com/nodejs/node/blob/main/lib/fs.js#L204).

bnoordhuis · 2023-09-18T19:35:42Z

Apparently that goes back to commit 0803962 from 2015... still wrong though. :-)

(Mostly harmless although technically signed integer overflow is UB in C++.)

anonrig · 2023-09-18T19:41:21Z

still wrong though. :-)

Nice find! I'll update the pull request.

@RafaelGSS I'm getting some weird failed test cases where stdout and stderr are both empty in the failing cases, but returning a status of 1. Any idea?

AssertionError [ERR_ASSERTION] at Object.<anonymous> (/Users/runner/work/node/node/test/parallel/test-permission-fs-read.js:42:10) at Module._compile (node:internal/modules/cjs/loader:1264:14) at Module._extensions..js (node:internal/modules/cjs/loader:1318:10) at Module.load (node:internal/modules/cjs/loader:1113:32) at Module._load (node:internal/modules/cjs/loader:954:12) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:80:12) at node:internal/main/run_main_module:23:47{generatedMessage: true, code: 'ERR_ASSERTION', actual: null, expected: 0, operator: 'strictEqual' }

src/node_file.cc

anonrig · 2023-09-24T20:51:21Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1413

09:17:21 confidence improvement accuracy (*) (**) (***) 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='undefined' -2.80 % ±8.78% ±11.68% ±15.21% 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='utf8' -6.68 % ±9.49% ±12.63% ±16.44% 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='undefined' -3.47 % ±9.74% ±12.97% ±16.88% 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='utf8' -5.20 % ±8.47% ±11.26% ±14.66% 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='undefined' -1.99 % ±4.82% ±6.41% ±8.34% 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='utf8' *** 126.70 % ±16.25% ±21.75% ±28.56% 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='undefined' -1.31 % ±6.23% ±8.29% ±10.79% 09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='utf8' *** 143.95 % ±18.72% ±25.19% ±33.36%

nodejs-github-bot · 2023-09-24T20:54:23Z

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

anonrig · 2023-09-24T20:56:57Z

@nodejs/cpp-reviewers @nodejs/fs Can you review?

bnoordhuis · 2023-09-25T07:02:27Z

src/node_file.cc

+CHECK_EQ(0, uv_fs_close(nullptr, &req, file, nullptr));
+FS_SYNC_TRACE_END(close);
+ }
+uv_fs_req_cleanup(&req);


For correctness reasons this ought to be done after the uv_fs_read() call on line 2590. It works okay now because libuv doesn't allocate for a single buffer (i.e. this cleanup call is a no-op) but that's a lucky accident.

Does that mean that on every while loop iteration (for uv_fs_read), we need to call uv_fs_req_cleanup?

resolved

nodejs-github-bot · 2023-09-25T21:56:05Z

Landed in f16f41c

targos · 2023-09-26T06:44:26Z

@anonrig Please always post the benchmark CI results in the PR (for example by editing the comment with the link like I just did). Jenkins results are not permanent.

PR-URL: #49691 Reviewed-By: Stephen Belanger <[email protected]>

PR-URL: nodejs#49691 Reviewed-By: Stephen Belanger <[email protected]>

Fix a file permissions regression when `fs.readFileSync()` is called in append mode on a file that does not already exist introduced by the fast path for utf8 encoding. PR-URL: #52101Fixes: #52079 Refs: #49691 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

Fix a file permissions regression when `fs.readFileSync()` is called in append mode on a file that does not already exist introduced by the fast path for utf8 encoding. PR-URL: nodejs#52101Fixes: nodejs#52079 Refs: nodejs#49691 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

Fix a file permissions regression when `fs.readFileSync()` is called in append mode on a file that does not already exist introduced by the fast path for utf8 encoding. PR-URL: #52101Fixes: #52079 Refs: #49691 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change. https://github.com/bnoordhuis made the same suggestion based on the PR nodejs#49691. Fixes: nodejs#57800

Free req.file.pathw in fs::ReadFileUtf8(). Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change. https://github.com/bnoordhuis made the same suggestion based on the PR nodejs#49691. Fixes: nodejs#57800

Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change. https://github.com/bnoordhuis made the same suggestion based on the PR nodejs#49691. Fixes: nodejs#57800

Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change. https://github.com/bnoordhuis made the same suggestion based on the PR #49691. Fixes: #57800 PR-URL: #57811 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change. https://github.com/bnoordhuis made the same suggestion based on the PR #49691. Fixes: #57800 PR-URL: #57811 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> CVE-ID: CVE-2025-23165

anonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 17, 2023

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 17, 2023

anonrig mentioned this pull request Sep 17, 2023
Performance of node:fsnodejs/performance#106
Open

rluvaton added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 17, 2023

anonrig force-pushed the fs-read-file-sync-fd branch 2 times, most recently from 0dfcfa4 to de56591Compare September 18, 2023 19:53

anonrig requested a review from RafaelGSS September 20, 2023 15:06

RafaelGSS previously requested changes Sep 21, 2023
View reviewed changes

src/node_file.cc Outdated Show resolvedHide resolved

anonrig requested a review from RafaelGSS September 21, 2023 19:29

anonrig mentioned this pull request Sep 21, 2023
fs: fix file descriptor validator #49752
Merged

anonrig added 2 commits September 24, 2023 16:49

fs: improve readFileSync with file descriptors
28cf7ff

fixup! fs: improve readFileSync with file descriptors
a415654

anonrig force-pushed the fs-read-file-sync-fd branch from 6fea27f to a415654Compare September 24, 2023 20:49

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2023

bnoordhuis reviewed Sep 25, 2023
View reviewed changes

Qard approved these changes Sep 25, 2023
View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 25, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 25, 2023

nodejs-github-bot merged commit f16f41c into nodejs:mainSep 25, 2023

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
fs: improve readFileSync with file descriptors
07d0518
PR-URL: #49691 Reviewed-By: Stephen Belanger <[email protected]>

ruyadorno mentioned this pull request Sep 28, 2023
v20.7.1 proposal #49917
Closed

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
fs: improve readFileSync with file descriptors
31702c9
PR-URL: #49691 Reviewed-By: Stephen Belanger <[email protected]>

ruyadorno mentioned this pull request Sep 28, 2023
v20.8.0 proposal #49932
Merged

This was referenced Oct 2, 2023
binding: mock readFileSync tschaub/mock-fs#378
Closed
Fix tests on Node 20.8.0+ getgauge/gauge-js#603
Merged

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
fs: improve readFileSync with file descriptors
1542939
PR-URL: nodejs#49691 Reviewed-By: Stephen Belanger <[email protected]>

richardlau added the dont-land-on-v18.x label Mar 15, 2024

This was referenced Mar 15, 2024
fs: fix edge case in readFileSync utf8 fast path #52101
Merged
fs.writeFileSync in Node 20 failed to write file in TMPDIR #52079
Closed

tniessen mentioned this pull request Apr 9, 2025
Memory leak in node::fs::ReadFileUtf8(const FunctionCallbackInfo<Value>& args) when args[0] is a string. #57800
Closed

Justin-Nietzel mentioned this pull request Apr 9, 2025
Free req.file.pathw in fs::ReadFileUtf8(). #57811
Closed

Uh oh!

fs: improve readFileSync with file descriptors#49691

fs: improve readFileSync with file descriptors #49691

Uh oh!

Conversation

anonrig commented Sep 17, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rluvaton commented Sep 18, 2023

Uh oh!

bnoordhuis commented Sep 18, 2023

Uh oh!

anonrig commented Sep 18, 2023

Uh oh!

bnoordhuis commented Sep 18, 2023

Uh oh!

anonrig commented Sep 18, 2023

Uh oh!

Uh oh!

anonrig commented Sep 24, 2023• edited by targos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2023

Uh oh!

anonrig commented Sep 24, 2023

Uh oh!

bnoordhuisSep 25, 2023

Choose a reason for hiding this comment

Uh oh!

anonrigSep 25, 2023

Choose a reason for hiding this comment

Uh oh!

bnoordhuisSep 26, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 25, 2023

Uh oh!

targos commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fs: improve `readFileSync` with file descriptors#49691

fs: improve `readFileSync` with file descriptors #49691

anonrig commented Sep 17, 2023•
edited
Loading

anonrig commented Sep 24, 2023•
edited by targos
Loading