fs: inconsistent options treatment between rm() and rmdir()

[Trott]

• Version: 14.14.0 and 15.0.0-pre (master branch)
• Platform: macOS
• Subsystem: fs

What steps will reproduce the bug?

Use /dev/null as 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 maxRetries option (with a backoff, I imagine) whereas fs.rmdir() ignores it if recursive is 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() and fs.rmdir() to treat maxRetries the same.

What do you see instead?

fs.rm() honors it with or without recursive being set, whereas fs.rmdir() ignores it unless recursive is set.

Additional information

[Trott]

@iansu @bcoe @cjihrig @nodejs/fs I think the right thing to do here is for both methods to honor maxRetries without recursive set. As a user, that's what I would expect. But I want to make sure there's agreement on that. WDYT?

[iansu]

I agree that they should both treat maxRetries the same but I'm not sure that they should respect that option if recursive has not been set. maxRetries is a rimraf option. If you're calling rmdir without recursive then it won't use rimraf at 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 the recursive option doesn't produce an error. I can take a closer look into what's going on here.

[Trott]

In your example it also seems strange that fs.rmdir, when pointed at a file without the recursive option 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.

[Trott]

I agree that they should both treat maxRetries the same but I'm not sure that they should respect that option if recursive has not been set. maxRetries is a rimraf option. If you're calling rmdir without recursive then it won't use rimraf at 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.

[IgorHalfeld]

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 🤔

[bcoe]

I think the right thing to do here is for both methods to honor maxRetries without recursive set

maxRetries was added at some point to allow folks to pass this option to the rimraf.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 maxRetries should 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.