fs: improve `readFileSync` with file descriptors by anonrig · Pull Request #49691 · nodejs/node

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 <admin@stephenbelanger.com>

PR-URL: nodejs#49691 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

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: #52101 Fixes: #52079 Refs: #49691 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>

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#52101 Fixes: nodejs#52079 Refs: nodejs#49691 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>

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: #52101 Fixes: #52079 Refs: #49691 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>

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 <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> 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:fs nodejs/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 de56591 Compare 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 resolved Hide 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 a415654 Compare 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-actions bot 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:main Sep 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 <admin@stephenbelanger.com>

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 <admin@stephenbelanger.com>

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 <admin@stephenbelanger.com>

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!

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!

bnoordhuis Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Sep 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

anonrig commented Sep 17, 2023 •

edited

Loading

anonrig commented Sep 24, 2023 •

edited by targos

Loading