szwacz / fs-jetpack

Better file system API for Node.js
MIT License
777 stars 41 forks source link

Find no longer crashes on ENOENT #79

Closed matortheeternal closed 5 years ago

matortheeternal commented 5 years ago

treeWalker.sync creates an item object by calling inspect.sync, which can return undefined if the file is not found. This can happen in a few situations when walking a directory tree, I specifically experienced it when the MAX_PATH limit of 255 was exceeded in a VFS context, though there likely are other situations where a directory may report a child file but inspect would throw an ENOENT file when trying to inspect that file.

This simple fix just verifies that the item passed to the treeWalker.sync callback in findSync is truthy before attempting to access its properties, so in a situation where an ENOENT error occurs while walking a file tree we simply ignore the error and continue walking the tree. I think this is the desired behavior for find, but some kind of reporting of the error may be prudent :question:

matortheeternal commented 5 years ago

I can try to add a regression test if this change is deemed acceptable.

codecov-io commented 5 years ago

Codecov Report

Merging #79 into master will increase coverage by 0.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   96.35%   96.36%   +0.01%     
==========================================
  Files          24       24              
  Lines        1263     1267       +4     
  Branches      242      246       +4     
==========================================
+ Hits         1217     1221       +4     
  Misses         46       46
Impacted Files Coverage Δ
lib/find.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 49def92...a4eb39c. Read the comment docs.

szwacz commented 5 years ago

I agree this is a desired behaviour. File system is not still, so you can even expect that some other party deletes a file after you listed a directory content.

But, I'm also big for the same behaviour for sync and async api, so are you willing to amend your PR with async fix as well?

Also how would you like to test for that case? I'm just curious :)

matortheeternal commented 5 years ago

Also how would you like to test for that case? I'm just curious :)

I was thinking about trying to see if exceeding MAX_PATH without a VFS involved would cause the issue. Unfortunately it appears to not cause an ENOENT error, so I'll have to find another method to make a regression test. :thinking:

EDIT: Can't think of another good reliable method to test it. Any ideas?

EDIT 2: The only other thing I can think of is some kind of test which would rely on timing. It would basically create a very large number of files, then start a process up to delete all of those files after a short delay, and then start up the find. The issue with this kind of test is it could easily give false negatives, so I don't think it's a good approach. The only other solution I can think of is to insert some kind of VFS into the testing suite, which seems like overkill.

szwacz commented 5 years ago

Yep, came to the same conclusion, this is too hard to test. I'm merging it without tests.