Repository navigation
fs: inconsistent options treatment between rm() and rmdir() #35689
Description
Activity
I agree that they should both treat
maxRetriesthe same but I'm not sure that they should respect that option ifrecursivehas not been set.maxRetriesis arimrafoption. If you're callingrmdirwithoutrecursivethen it won't userimrafat all so the option doesn't really make sense and shouldn't have any effect.In your example it also seems strange that
fs.rmdir, when pointed at a file without therecursiveoption doesn't produce an error. I can take a closer look into what's going on here.In your example it also seems strange that
fs.rmdir, when pointed at a file without therecursiveoption doesn't produce an error. I can take a closer look into what's going on here.It produces an error, but my callback swallows it.
I agree that they should both treat
maxRetriesthe same but I'm not sure that they should respect that option ifrecursivehas not been set.maxRetriesis arimrafoption. If you're callingrmdirwithoutrecursivethen it won't userimrafat all so the option doesn't really make sense and shouldn't have any effect.For non-recursive calls, I wonder if retries still make sense when dealing with NFS, for example.
For non-recursive calls, I wonder if retries still make sense when dealing with NFS, for example.
I think it makes sense, not having a file inside is not the only reason for an unsuccessful deletion, it would be nice to have maxRetries in this case too 🤔
I think the right thing to do here is for both methods to honor maxRetries without recursive set
maxRetrieswas added at some point to allow folks to pass this option to therimraf.js. The reason for the option is that Windows has race conditions when deleting all files in a folder, so it's common to need to attempt the removal a few times.I think
maxRetriesshould be a noop if not used in conjunction with recursive, probably with a warning.
I don't hold this opinion particularly strongly, I just can't see retries being as useful outside the context of
rimraf.js.Edit: it's called maxBusyTries in rimraf, but I think it does the same thing.
Reacted by Ryan ZimmermanThis issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
What steps will reproduce the bug?
Use
/dev/nullas the path like below, or use some other path that will not be removed and force a retry.This tries, doesn't remove
/dev/null, and exits with "done" very quickly.$ node -e 'fs.rmdir("/dev/null", { maxRetries: 42 }, () => { console.log("done"); })'So does this:
$ node -e 'fs.rmdir("/dev/null", { maxRetries: 420 }, () => { console.log("done"); })'This does the same:
$ node -e 'fs.rm("/dev/null", { maxRetries: 1 }, () => { console.log("done"); })'But this takes about 90 seconds to finish, presumably because it is respecting the
maxRetriesoption (with a backoff, I imagine) whereasfs.rmdir()ignores it ifrecursiveis not set:$ node -e 'fs.rm("/dev/null", { maxRetries: 42 }, () => { console.log("done"); })'How often does it reproduce? Is there a required condition?
Reproduces every time.
What is the expected behavior?
I would expect
fs.rm()andfs.rmdir()to treatmaxRetriesthe same.What do you see instead?
fs.rm()honors it with or withoutrecursivebeing set, whereasfs.rmdir()ignores it unlessrecursiveis set.Additional information