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

Support bufferSize option with recursive mode in fs.opendir #55764

Open
Ethan-Arrowood opened this issue Nov 7, 2024 · 13 comments
Open

Support bufferSize option with recursive mode in fs.opendir #55764

Ethan-Arrowood opened this issue Nov 7, 2024 · 13 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.

Comments

@Ethan-Arrowood
Copy link
Contributor

Related to: #48820 and #55744

After the recursive option was added to readdir and opendir, it was noted that when specifying bufferSize alongside recursive: true, the result of opendir was incorrect. This is fixed in #55744 . However, the fix is a naive solution, and doesn't properly respect the bufferSize option. Furthermore, it could result in a blocked event loop. This should be fixed.

I recommend reading the discussion in #48820 for more information. This should only involve changes to the Dir class in lib/internal/fs/dir.js.

@Ethan-Arrowood Ethan-Arrowood added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors. labels Nov 7, 2024
@Ethan-Arrowood
Copy link
Contributor Author

If someone would like to submit a fix here, I'm happy to help you land the contribution. Otherwise, I'll work on it eventually.

@cu8code
Copy link

cu8code commented Nov 7, 2024

Hey I would love to work on this!

@Ethan-Arrowood
Copy link
Contributor Author

Awesome! Feel free to request my review on your PR. I'll keep track of notifications here as well. Reach out to me in the OpenJS Slack if you have any questions

@mertcanaltin
Copy link
Member

I'm here if you need help

@KunalKumar-1
Copy link
Contributor

@Ethan-Arrowood Can you clarify what further changes is needed in lib/internal/fs/dir.js ?
seems you made some changes there already

@cu8code
Copy link

cu8code commented Nov 8, 2024

@KunalKumar-1 from what i saw its was a temporary patch to get around the problem. probably have to rewrite this.

@Ethan-Arrowood
Copy link
Contributor Author

Yes, the changes I added does not actually respect the bufferSize option, but it at least will return the full contents of a directory. The change I'm requesting here is that we figure out how to properly support bufferSize option.

My loose idea for a solution would be to add a new queue to the Dir class, something like #handlers, and as things are read, if its in recursive mode, and the current item is a directory, create a handle for that directory, do the read operation (while respecting bufferSize), and if there is more to be read, push that handle to #handlers, and on subsequent reads, make sure to fully read that handler.

@KunalKumar-1
Copy link
Contributor

Yes, the changes I added does not actually respect the bufferSize option, but it at least will return the full contents of a directory. The change I'm requesting here is that we figure out how to properly support bufferSize option.

My loose idea for a solution would be to add a new queue to the Dir class, something like #handlers, and as things are read, if its in recursive mode, and the current item is a directory, create a handle for that directory, do the read operation (while respecting bufferSize), and if there is more to be read, push that handle to #handlers, and on subsequent reads, make sure to fully read that handler.

Now I understood the issue clearly
thanks @Ethan-Arrowood for the explanation.
i have started working on this issue

@cu8code
Copy link

cu8code commented Nov 8, 2024

@KunalKumar-1 cool but i was working on this!

@cu8code
Copy link

cu8code commented Nov 13, 2024

I will be doping this issue, as I was unable to coming with any good solution for this, you can see the draft PR, if you want to know my approach! Ya! that will be it!

@amansharma612
Copy link

I would like to take up this issue

@cu8code
Copy link

cu8code commented Nov 29, 2024

@Ethan-Arrowood, it’s been two weeks since I opened this PR #55896 When you have some time, could you please take a look?

@babyitachi
Copy link

Hey, I would like to pick this up if not done. Let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

6 participants