search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
108.03k stars 29.82k forks source link

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

Open Ethan-Arrowood opened 3 weeks ago

Ethan-Arrowood commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Hey I would love to work on this!

Ethan-Arrowood commented 3 weeks ago

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 commented 3 weeks ago

I'm here if you need help

KunalKumar-1 commented 3 weeks ago

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

cu8code commented 3 weeks ago

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

Ethan-Arrowood commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

cu8code commented 2 weeks ago

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 commented 2 weeks ago

I would like to take up this issue

cu8code commented 13 hours ago

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