Skip to content

fs: fix readdir recursive sync & callback - #48698

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Ethan-Arrowood:fix/readdir-recursive-sync-and-callback
Aug 12, 2023
Merged

fs: fix readdir recursive sync & callback#48698
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Ethan-Arrowood:fix/readdir-recursive-sync-and-callback

Conversation

@Ethan-Arrowood

@Ethan-ArrowoodEthan-Arrowood commented Jul 7, 2023•
edited by aduh95
Loading

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

@nodejs-github-botnodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 7, 2023
@Ethan-Arrowood

Ethan-Arrowood commented Jul 7, 2023

Copy link
Copy Markdown
ContributorAuthor

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.

anonrig
anonrig reviewed Jul 7, 2023
View reviewed changes
Comment threadtest/sequential/test-fs-readdir-recursive.js Outdated
Comment threadlib/fs.js Outdated
@chris-sophos

chris-sophos commented Jul 8, 2023

Copy link
Copy Markdown

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

@Ethan-ArrowoodEthan-Arrowood mentioned this pull request Jul 18, 2023
Closed
@Ethan-Arrowood
Ethan-Arrowoodforce-pushed the fix/readdir-recursive-sync-and-callback branch from 7bcf27f to d636303CompareJuly 19, 2023 16:45
@Ethan-Arrowood

Ethan-Arrowood commented Jul 19, 2023

Copy link
Copy Markdown
ContributorAuthor

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

@MattIPv4MattIPv4 mentioned this pull request Jul 20, 2023
Closed
BridgeAR
BridgeAR approved these changes Jul 26, 2023
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@BridgeARBridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced Jul 27, 2023
Open
Open
Open
Open
@debadree25debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 30, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced Jul 31, 2023
Open
Open
Open
Open
@nodejs-github-bot

nodejs-github-bot commented Aug 11, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

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

@MoLowMoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 27cadf5 into nodejs:mainAug 12, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

Landed in 27cadf5

This was referenced Aug 13, 2023
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Ethan Arrowood@Ceres6
fs: fix readdir recursive sync & callback
292b9c2
Refs: nodejs#48640
PR-URL: nodejs#48698Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
Ethan Arrowood@RafaelGSS
fs: fix readdir recursive sync & callback
acb6da3
Refs: #48640
PR-URL: #48698Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@UlisesGasconUlisesGascon mentioned this pull request Aug 15, 2023
Merged
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
Ethan Arrowood@RafaelGSS
fs: fix readdir recursive sync & callback
036e9fa
Refs: nodejs#48640
PR-URL: nodejs#48698Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023
Ethan Arrowood@rluvaton
fs: fix readdir recursive sync & callback
62d36d9
Refs: nodejs#48640
PR-URL: nodejs#48698Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
Ethan Arrowood@RafaelGSS
fs: fix readdir recursive sync & callback
d7269a3
Refs: #48640
PR-URL: #48698Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
Ethan Arrowood@RafaelGSS
fs: fix readdir recursive sync & callback
3d942d9
Refs: #48640
PR-URL: #48698Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@kachickkachick mentioned this pull request Aug 28, 2023
Closed
@lux01lux01 mentioned this pull request Sep 11, 2023
Merged
@MattIPv4

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.

targos pushed a commit that referenced this pull request Oct 28, 2023
Ethan Arrowood@targos
fs: fix readdir recursive sync & callback
7c758f6
Refs: #48640
PR-URL: #48698Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targostargos mentioned this pull request Nov 28, 2023
Merged
@richardlaurichardlau mentioned this pull request Feb 15, 2024
Closed
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
fs: fix readdir recursive sync & callback
c6dca02
Refs: nodejs/node#48640
PR-URL: nodejs/node#48698Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
fs: fix readdir recursive sync & callback
a45f06b
Refs: nodejs/node#48640
PR-URL: nodejs/node#48698Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@juanarboljuanarbol mentioned this pull request Nov 4, 2024
Closed
@coderabbitaicoderabbitaiBot mentioned this pull request Jun 19, 2026
Merged
@coderabbitaicoderabbitaiBot mentioned this pull request Aug 1, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes
@anonriganonriganonrig approved these changes
@MoLowMoLowMoLow approved these changes
@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs 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

11 participants

@Ethan-Arrowood@chris-sophos@nodejs-github-bot@MattIPv4@benjamingr@anonrig@MoLow@BridgeAR@targos@richardlau@debadree25