Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance openDir to Respect Buffer Size #55817

Closed
wants to merge 1 commit into from
Closed

Conversation

cu8code
Copy link

@cu8code cu8code commented Nov 11, 2024

Issue #55764

The current openDir implementation does not fully respect the buffer size. This PR introduces an initial solution based on suggestions from @Ethan-Arrowood. Feedback on the concept would be appreciated.

PR Status

  • Ready for merge? (Pending feedback)
  • Is passing all the tests

Proposed Solution

I am currently working on a #handlerQueue that stores a readNext function. readNext calls handler.read, and if the return value is not null, it re-adds itself to the queue for continued processing.

Note: This solution might be modified or scrapped, but the aim here is to get the conversation started. And just get the work done.

Current Challenges

  1. handler.read does not always populate this.#bufferedEntries—for instance, when a directory is empty. This creates issues with triggering this.processNextHandler after every yield, as it may yield nothing.
  2. Determining the appropriate point to return all remaining items.
  3. Handling readSync

@nodejs-github-bot 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 11, 2024
@cu8code cu8code changed the title fs: fix bufferSize option for opendir recursive Enhance openDir to Respect Buffer Size and Improve Handler Queue Management Nov 11, 2024
@cu8code cu8code changed the title Enhance openDir to Respect Buffer Size and Improve Handler Queue Management Enhance openDir to Respect Buffer Size Nov 11, 2024
@Ethan-Arrowood
Copy link
Contributor

Is this working? I'd recommend making the PR a draft until you have the unit tests working.

@cu8code cu8code marked this pull request as draft November 11, 2024 16:47
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 87.34177% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.39%. Comparing base (fe1dd26) to head (d30518b).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/dir.js 87.34% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55817      +/-   ##
==========================================
+ Coverage   87.92%   88.39%   +0.46%     
==========================================
  Files         654      654              
  Lines      187815   187871      +56     
  Branches    35830    36140     +310     
==========================================
+ Hits       165139   166063     +924     
+ Misses      15878    15046     -832     
+ Partials     6798     6762      -36     
Files with missing lines Coverage Δ
lib/internal/fs/dir.js 90.52% <87.34%> (-6.70%) ⬇️

... and 97 files with indirect coverage changes

@cu8code cu8code closed this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants