GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: fix readdir and opendir recursive with unknown file types by lux01 · Pull Request #49603 · nodejs/node

Conversation

@lux01

@lux01 lux01 commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

If the libuv operations invoked by readdir/opendir return uv_dirent_t values where the type is UV_DIRENT_UNKNOWN then a further lstat is issued to fully construct the Dirent values. In the recursive versions of these functions, the path parameter was incorrectly assumed to be the path to the entry when it should be the path to the directory containing the entry.

Fixes #49499.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to file-system APIs and the fs module. label Sep 11, 2023
@lux01 lux01 mentioned this pull request Sep 11, 2023
Regression in fs.readdirSync on AIX introduced in 18.17.0 #49499
Closed
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 11, 2023
@lux01
fs: fix readdir and opendir recursive with unknown file types
632705c 
If the libuv operations invoked by `readdir`/`opendir` return
`uv_dirent_t` values where the `type` is `UV_DIRENT_UNKNOWN` then a
further `lstat` is issued to fully construct the `Dirent` values. In the
recursive versions of these functions, the `path` parameter was
incorrectly assumed to be the path to the entry when it should be the
path to the directory containing the entry.

Fixes #49499.
@richardlau richardlau 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 Sep 11, 2023
@github-actions 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 Sep 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 11, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53891/
All reactions

Sorry, something went wrong.

Uh oh!

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

anonrig commented Sep 11, 2023

Copy link
Copy Markdown
Member

cc @Ethan-Arrowood
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ethan-Arrowood
Ethan-Arrowood approved these changes Sep 11, 2023
View reviewed changes

@Ethan-Arrowood Ethan-Arrowood left a comment

Copy link
Copy Markdown
Contributor

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
@Ethan-Arrowood

Ethan-Arrowood commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

Great investigation and fix @lux01 . Thank you very much!
lux01 reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Sep 11, 2023
View reviewed changes

@anonrig anonrig 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
MoLow
MoLow approved these changes Sep 11, 2023
View reviewed changes
@MoLow

MoLow commented Sep 11, 2023

Copy link
Copy Markdown
Member

@ruyadorno this seems like a serious bug and is ready for landing, perhaps it is a good idea to land this into v18?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added fast-track PRs proposed for a shorter-than-standard waiting period before landing. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Sep 11, 2023
@github-actions

github-actions Bot commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @MoLow. Please 👍 to approve.
anonrig 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.
@lux01

lux01 commented Sep 11, 2023

Copy link
Copy Markdown
Contributor Author

To fix this on the v18.x branch I needed #48698 so that would need back porting too (if it's not already marked as such).
MoLow reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 11, 2023
@nodejs-github-bot
nodejs-github-bot merged commit d8b378c into nodejs:main Sep 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 11, 2023

Copy link
Copy Markdown
Collaborator

Landed in d8b378c
All reactions

Sorry, something went wrong.

Uh oh!

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

ruyadorno commented Sep 12, 2023

Copy link
Copy Markdown
Member

@MoLow it hasn't landed on time yet for the v20 release and I prefer not to rush it to v18 before it's out in a current release line. 😊 thanks for the heads up though!
MoLow 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.
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@lux01 @ruyadorno
fs: fix readdir and opendir recursive with unknown file types
db3fc6d 
If the libuv operations invoked by `readdir`/`opendir` return
`uv_dirent_t` values where the `type` is `UV_DIRENT_UNKNOWN` then a
further `lstat` is issued to fully construct the `Dirent` values. In the
recursive versions of these functions, the `path` parameter was
incorrectly assumed to be the path to the entry when it should be the
path to the directory containing the entry.

Fixes #49499.

PR-URL: #49603
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
This was referenced Sep 28, 2023
v20.7.1 proposal #49917
Closed
v20.8.0 proposal #49932
Merged
@Ethan-Arrowood Ethan-Arrowood mentioned this pull request Oct 11, 2023
fs#opendir does not return all files in subdirectories when bufferSize is less than the directory size #48820
Closed
@aduh95 aduh95 added the dont-land-on-v18.x label Dec 3, 2023
@aduh95

aduh95 commented Dec 3, 2023

Copy link
Copy Markdown
Contributor

This introduced a breaking change for dirent.path (and should have been marked as semver-major PRs that contain breaking changes and should be released in the next major version. IMO), marking it as dont-land-on-v18.x.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sonsurim sonsurim mentioned this pull request Jul 21, 2024
[v18.x backport] fs: fix the file name is not included when the withFileTypes option of readdir #53969
Closed
sonsurim pushed a commit to sonsurim/node that referenced this pull request Jul 21, 2024
sonny @sonsurim
fs: resolve error in test-fs-opendir-recursive.js
79da800 
An error occurred in the test/sequential/test-fs-opendir-recursive.js file
after changing the code. This was resolved by referring to the code resolved
in PR nodejs#49603.

Refs: nodejs#49603
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@MoLow MoLow MoLow approved these changes
@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood 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. fast-track PRs proposed for a shorter-than-standard waiting period before landing. fs 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.

Regression in fs.readdirSync on AIX introduced in 18.17.0

Uh oh!

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

8 participants

@lux01 @nodejs-github-bot @anonrig @Ethan-Arrowood @MoLow @ruyadorno @aduh95 @richardlau
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.