GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fs: fix readdir recursive sync & callback by Ethan-Arrowood · Pull Request #48698 · nodejs/node

Conversation

Ethan-Arrowood commented Jul 7, 2023 •

edited by aduh95

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This PR fixes the broken behavior for readdir recursive sync and callback. It updates the test assertions to catch the issue in the old code, and then adds the necessary fix in the implementation.

Refs: #48640
Fixes: #48858

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss and kachick reacted with thumbs up emoji

All reactions

👍 2 reactions

nodejs-github-bot added fs

Issues and PRs related to file-system APIs and the fs module.

needs-ci PRs that need a full CI run. labels

Jul 7, 2023

Ethan-Arrowood commented Jul 7, 2023

Copy link

Copy Markdown

Contributor Author

The issue noted that documentation is also missing. I want to keep that contribution available for a new contributor (some folks reached out in the thread). If we don't get a response back from them in a couple of days I'll add the docs fix in here too.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig reviewed

Jul 7, 2023

View reviewed changes

Comment thread

test/sequential/test-fs-readdir-recursive.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/fs.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

chris-sophos commented Jul 8, 2023

Copy link

Copy Markdown

Does this also address the case where dirents are missing entirely as well?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ethan-Arrowood mentioned this pull request

Jul 18, 2023

fs#opendir does not return all files in subdirectories when bufferSize is less than the directory size #48820

Closed


          fs: fix readdir recursive sync & callback

d636303

Refs: nodejs#48640

Ethan-Arrowood force-pushed the fix/readdir-recursive-sync-and-callback branch from 7bcf27f to d636303 Compare

July 19, 2023 16:45

Ethan-Arrowood commented Jul 19, 2023

Copy link

Copy Markdown

Contributor Author

@That-Guy977 yes I believe it does because it asserts the length of the look up matches the expected total length.

chris-sophos reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ethan-Arrowood requested a review from anonrig

July 19, 2023 16:46

MattIPv4 mentioned this pull request

Jul 20, 2023

fs#readdir(Sync) breaks when recursive + withFileTypes combined #48858

Closed

BridgeAR approved these changes

Jul 26, 2023

View reviewed changes

BridgeAR left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Jul 26, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Jul 26, 2023

nodejs-github-bot commented Jul 26, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr approved these changes

Jul 26, 2023

View reviewed changes

nodejs-github-bot commented Jul 26, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow approved these changes

Jul 26, 2023

View reviewed changes

nodejs-github-bot commented Jul 26, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 26, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jul 27, 2023

CI Reliability 2023-07-27 nodejs/reliability#625

Open

CI Reliability 2023-07-28 nodejs/reliability#626

Open

CI Reliability 2023-07-29 nodejs/reliability#627

Open

CI Reliability 2023-07-30 nodejs/reliability#628

Open

debadree25 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Jul 30, 2023

nodejs-github-bot commented Jul 30, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jul 31, 2023

CI Reliability 2023-07-31 nodejs/reliability#629

Open

CI Reliability 2023-08-01 nodejs/reliability#630

Open

CI Reliability 2023-08-02 nodejs/reliability#631

Open

CI Reliability 2023-08-03 nodejs/reliability#632

Open

nodejs-github-bot commented Aug 12, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow added the commit-queue PRs queued for automated landing through the Commit Queue. label

Aug 12, 2023

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Aug 12, 2023

nodejs-github-bot merged commit 27cadf5 into nodejs:main

Aug 12, 2023

nodejs-github-bot commented Aug 12, 2023

Copy link

Copy Markdown

Collaborator

Landed in 27cadf5

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 13, 2023

CI Reliability 2023-08-13 nodejs/reliability#635

Open

CI Reliability 2023-08-14 nodejs/reliability#636

Open

Ceres6 pushed a commit to Ceres6/node that referenced this pull request

Aug 14, 2023


          fs: fix readdir recursive sync & callback

292b9c2

Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Aug 15, 2023


          fs: fix readdir recursive sync & callback

acb6da3

Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

UlisesGascon mentioned this pull request

Aug 15, 2023

v20.6.0 proposal #49185

Merged

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request

Aug 15, 2023


          fs: fix readdir recursive sync & callback

036e9fa

Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Aug 16, 2023


          fs: fix readdir recursive sync & callback

d7269a3

Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Aug 17, 2023


          fs: fix readdir recursive sync & callback

3d942d9

Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

kachick mentioned this pull request

Aug 28, 2023

2023-08-28 - Node.js の fs.readdir と fs.readdirSync にバグがあって、recursive と withFileTypes オプションを両方有効にすると戻りのdirent一覧が大幅に欠落する kachick/times_kachick#244

Closed

lux01 mentioned this pull request

Sep 11, 2023

fs: fix readdir and opendir recursive with unknown file types #49603

Merged

MattIPv4 commented Oct 12, 2023

Copy link

Copy Markdown

Member

👋 Asked in slack, but asking here as well for visibility, is there intent to backport this fix to the 18.x line? #48858 was a report for 18.x, and this remains broken there currently.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau added the lts-watch-v18.x label

Oct 12, 2023

targos added backported-to-v18.x and removed lts-watch-v18.x labels

Oct 28, 2023

targos pushed a commit that referenced this pull request

Oct 28, 2023


          fs: fix readdir recursive sync & callback

7c758f6

Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

targos mentioned this pull request

Nov 28, 2023

v18.19.0 release proposal #50953

Merged

richardlau mentioned this pull request

Feb 15, 2024

readdirSync cannot read files properly when using { recursive: true, withFileTypes: true } #51773

Closed

sercher added a commit to sercher/graaljs that referenced this pull request

Apr 25, 2024


          fs: fix readdir recursive sync & callback

c6dca02

Refs: nodejs/node#48640
PR-URL: nodejs/node#48698
Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

sercher added a commit to sercher/graaljs that referenced this pull request

Apr 25, 2024


          fs: fix readdir recursive sync & callback

a45f06b

Refs: nodejs/node#48640
PR-URL: nodejs/node#48698
Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

juanarbol mentioned this pull request

Nov 4, 2024

withFileTypes has different behavior in fs.readdir when reading symbolic directories #52663

Closed

coderabbitai Bot mentioned this pull request

Jun 19, 2026

Align harness reviewers/skills to FastAPI+React, enforce skill ownership, expand evals mravinale/llm-harness-fastapi-react#3

Merged

coderabbitai Bot mentioned this pull request

Aug 1, 2026

feat(access,cli): the integrity checks, ow check, and source state protonspy/open-wiki#8

Merged

coderabbitai Bot mentioned this pull request

Aug 22, 2026

decisions: a package ships a gate behind a bin, or declares it is a primitive clossys/foundry#465

Merged

coderabbitai Bot mentioned this pull request

Sep 19, 2026

Add smoke tests for the built CLI primocms/primo-cli#25

Merged

Reviewers

benjamingr

benjamingr approved these changes

anonrig

anonrig approved these changes

MoLow

MoLow approved these changes

BridgeAR

BridgeAR approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

Issues and PRs related to file-system APIs and the fs module.

needs-ci

PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fs#readdir(Sync) breaks when recursive + withFileTypes combined

Uh oh!

There was an error while loading. Please reload this page.

11 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Ethan-Arrowood commented Jul 7, 2023 • edited by aduh95 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ethan-Arrowood commented Jul 7, 2023

Uh oh!

Uh oh!

Uh oh!

chris-sophos commented Jul 8, 2023

Uh oh!

Ethan-Arrowood commented Jul 19, 2023

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 30, 2023

Uh oh!

nodejs-github-bot commented Aug 12, 2023

Uh oh!

nodejs-github-bot commented Aug 12, 2023

Uh oh!

MattIPv4 commented Oct 12, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Ethan-Arrowood commented Jul 7, 2023 •

edited by aduh95

Loading

Uh oh!

There was an error while loading. Please reload this page.