lib: fix fs.readdir recursive async by RafaelGSS · Pull Request #56041 · nodejs/node

RafaelGSS · 2024-11-27T20:05:17Z

codecov · 2024-11-27T21:50:08Z

Codecov Report

Attention: Patch coverage is 92.98246% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.01%. Comparing base (ce346b6) to head (16a29cb).
Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/fs.js	92.98%	8 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #56041    +/-   ##
========================================
  Coverage   88.00%   88.01%            
========================================
  Files         653      656     +3     
  Lines      188093   189077   +984     
  Branches    35942    36006    +64     
========================================
+ Hits       165537   166412   +875     
- Misses      15734    15840   +106     
- Partials     6822     6825     +3

Files with missing lines	Coverage Δ
lib/fs.js	`93.26% <92.98%> (-0.09%)`	⬇️

... and 55 files with indirect coverage changes

nodejs-github-bot · 2024-11-27T21:56:38Z

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

anonrig · 2024-11-28T00:00:43Z

+  }
+}
+
+function handleDirents({ result, currentPath, context }) {


Would you mind adding jsdoc to all newly/updated functions?

I haven't added jsdoc to these ones because they are not public, they are used inside readdirRecursive (which has JSDoc). It seems a pattern on this file.

nodejs-github-bot · 2024-11-28T00:56:25Z

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

nodejs-github-bot · 2024-11-28T02:20:32Z

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

LiviaMedeiros

node/lib/fs.js

Line 1533 in 982a50e

options = getOptions(options);

If options gets mutated (for example, to be reused subsequent readdir) after the function call during the directory processing, the results might become unpredictable. We should copyObject() it.

Ethan-Arrowood

lgtm

aduh95 · 2024-11-29T20:20:46Z

CI is failing

Path: parallel/test-fs-readdir-types-symlinks
Error: --- stderr ---
node:internal/errors:540
      throw error;
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received an instance of Array
    at Object.join (node:path:1261:7)
    at handleFilePaths (node:fs:1458:35)
    at processReaddirResult (node:fs:1437:93)
    at read (node:fs:1497:5)
    at readdirSyncRecursive (node:fs:1505:5)
    at Object.readdirSync (node:fs:1576:12)
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-fs-readdir-types-symlinks.js:34:6)
    at Module._compile (node:internal/modules/cjs/loader:1566:14)
    at Object..js (node:internal/modules/cjs/loader:1718:10)
    at Module.load (node:internal/modules/cjs/loader:1305:32) {
  code: 'ERR_INVALID_ARG_TYPE'
}

Node.js v24.0.0-pre
Command: out/Release/node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /home/runner/work/node/node/test/parallel/test-fs-readdir-types-symlinks.js
=== release test-fs-readdir-recursive ===
Path: sequential/test-fs-readdir-recursive
Error: --- stderr ---
node:internal/errors:540
      throw error;
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received an instance of Array
    at Object.join (node:path:1261:7)
    at handleFilePaths (node:fs:1458:35)
    at processReaddirResult (node:fs:1437:93)
    at read (node:fs:1497:5)
    at readdirSyncRecursive (node:fs:1505:5)
    at Object.readdirSync (node:fs:1576:12)
    at Object.<anonymous> (/home/runner/work/node/node/test/sequential/test-fs-readdir-recursive.js:156:21)
    at Module._compile (node:internal/modules/cjs/loader:1566:14)
    at Object..js (node:internal/modules/cjs/loader:1718:10)
    at Module.load (node:internal/modules/cjs/loader:1305:32) {
  code: 'ERR_INVALID_ARG_TYPE'
}

RafaelGSS · 2024-11-29T23:42:18Z

Thanks @aduh95. I forgot the readdir tests runs on sequential. I'll check

juanarbol

sgtm

nodejs-github-bot · 2024-12-03T13:16:26Z

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

BridgeAR · 2024-12-03T14:15:58Z

+  for (let i = 0; i < context.pathsQueue.length; i++) {
+    read(context.pathsQueue[i]);
  }


I know this is somewhat out of scope of the PR, I just wondered if we want to limit the amount of reads triggered to an upper bound of available file descriptors? That way we would not allocate anything before it's needed and it would probably reduce memory and increase performance a tad.

Hmm, that makes sense. But I wouldn't include it on this PR as it just fixes an async behavior. Currently, fs.readdir() - async performs one operation per time:

-> readdir -> cb -> readdir -> cb ...

But at some point, we could read from parallel folders within the same callback, which would improve performance significantly (memory would increase, though).

nodejs-github-bot · 2024-12-04T14:20:15Z

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

nodejs-github-bot · 2024-12-04T19:40:09Z

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

nodejs-github-bot · 2024-12-04T20:46:30Z

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

nodejs-github-bot · 2024-12-05T03:17:40Z

Commit Queue failed

- Loading data for nodejs/node/pull/56041
✔  Done loading data for nodejs/node/pull/56041
----------------------------------- PR info ------------------------------------
Title      lib: fix fs.readdir recursive async (#56041)
Author     Rafael Gonzaga <rafael.nunu@hotmail.com> (@RafaelGSS)
Branch     RafaelGSS:fix-readdir-async-throw -> nodejs:main
Labels     fs, author ready
Commits    3
 - lib: fix fs.readdir recursive async
 - fixup! lib: fix fs.readdir recursive async
 - fixup! fixup! lib: fix fs.readdir recursive async
Committers 1
 - RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: https://github.com/nodejs/node/pull/56041
Fixes: https://github.com/nodejs/node/issues/56006
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56041
Fixes: https://github.com/nodejs/node/issues/56006
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - fixup! fixup! lib: fix fs.readdir recursive async
   ℹ  This PR was created on Wed, 27 Nov 2024 20:05:17 GMT
   ✔  Approvals: 2
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/56041#pullrequestreview-2470295506
   ✔  - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/56041#pullrequestreview-2470693898
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-12-04T20:46:30Z: https://ci.nodejs.org/job/node-test-pull-request/63877/
- Querying data for job/node-test-pull-request/63877/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/12172531267

RafaelGSS · 2024-12-05T19:32:47Z

Could someone re-approve? I pushed 16a29cb after approvals.

aduh95 · 2024-12-05T20:25:27Z

Landed in 53356c3

lib: fix fs.readdir recursive async

982a50e

Fixes: nodejs#56006

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

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2024

anonrig reviewed Nov 28, 2024

View reviewed changes

jazelly reviewed Nov 28, 2024

View reviewed changes

Comment thread lib/fs.js Outdated

LiviaMedeiros reviewed Nov 29, 2024

View reviewed changes

LiviaMedeiros mentioned this pull request Nov 29, 2024

fs: make mutating options in readdir() not affect results #56057

Closed

ShenHongFei reviewed Nov 29, 2024

View reviewed changes

Comment thread lib/fs.js Outdated

aduh95 reviewed Nov 29, 2024

View reviewed changes

Comment thread lib/fs.js Outdated

aduh95 reviewed Nov 29, 2024

View reviewed changes

Comment thread lib/fs.js Outdated

fixup! lib: fix fs.readdir recursive async

6ca4cf4

Ethan-Arrowood approved these changes Nov 29, 2024

View reviewed changes

juanarbol approved these changes Nov 30, 2024

View reviewed changes

fixup! fixup! lib: fix fs.readdir recursive async

16a29cb

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 3, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 3, 2024

BridgeAR reviewed Dec 3, 2024

View reviewed changes

RafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 4, 2024

juanarbol added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Dec 5, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 5, 2024

juanarbol added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 5, 2024

aduh95 added the needs-ci PRs that need a full CI run. label Dec 5, 2024

aduh95 merged commit 53356c3 into nodejs:main Dec 5, 2024

targos mentioned this pull request Dec 6, 2024

2024-12-10, Version 23.4.0 (Current), @targos #56119

Merged

ruyadorno mentioned this pull request Dec 20, 2024

2025-01-07, Version 22.13.0 'Jod' (LTS) #56329

Merged

Uh oh!

Conversation

RafaelGSS commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Nov 27, 2024

Uh oh!

anonrig Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

RafaelGSS Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 28, 2024

Uh oh!

nodejs-github-bot commented Nov 28, 2024

Uh oh!

Uh oh!

LiviaMedeiros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Nov 29, 2024

Uh oh!

RafaelGSS commented Nov 29, 2024

Uh oh!

juanarbol left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 3, 2024

Uh oh!

BridgeAR Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

RafaelGSS Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 4, 2024

Uh oh!

nodejs-github-bot commented Dec 4, 2024

Uh oh!

nodejs-github-bot commented Dec 4, 2024

Uh oh!

nodejs-github-bot commented Dec 5, 2024

Uh oh!

RafaelGSS commented Dec 5, 2024 • edited by aduh95 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Dec 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

RafaelGSS commented Nov 27, 2024 •

edited

Loading

codecov Bot commented Nov 27, 2024 •

edited

Loading

RafaelGSS Nov 28, 2024 •

edited

Loading

RafaelGSS commented Dec 5, 2024 •

edited by aduh95

Loading