Improved workarounds for win32, continued

[jgoz]

Extends #119 to address feedback from @isaacs.

See the original PR description in #119 for a full explanation of changes.

This PR adds the following:

• Reduce backoff to 5 seconds (@mekwall — was the previous value a deliberate choice?)
• Make backoff configurable via process.env.GRACEFUL_FS_WIN32_MAX_BACKOFF
• Handle ENOTEMPTY when renaming a directory to a target that is not empty
• Guard cb calls
• Fix tests + add ENOTEMPTY tests

Closes #119.

[mekwall]

@jgoz Thanks for picking this up! To answer your question. 60 seconds was based on this comment: https://github.com/isaacs/node-graceful-fs/blob/master/polyfills.js#L86

[jgoz]

@mekwall Thank you. Based on that, I'd be inclined to restore the default backoff to 60s, but I'll await guidance from @isaacs.

[fluffynuts]

Sorry to be a general nag, but this issue is related to jestjs/jest#4444, which has resulted in a lot of hackarounds to prevent the problems faced by people there -- could someone blow off the cobwebs and revive this for a merge?

[mekwall]

@fluffynuts Decided it was time to do something about this. I've created a new package named normalized-fs. I will release it once I've ported the rest of the code to TypeScript and gotten all the tests in place but would be great to get some early adopters/testers.

[SimenB]

@iarna is this something you folks at npm could look into? And perhaps nodejs/node#25060? I'm not sure if npm manages this module at all, but this PR has lingered for half a year without feedback, and it seems to impact a lot of people.

[fluffynuts]

👍 willing to adopt and try: how tho'? Is there some secret sauce to forcing this? Or can I just make this a top-level dev-dep?

[isaacs]

If I'm reading this correctly, it's synchronously unlinking the target prior to even attempting to rename.

That's... not how it works on Unix systems.

$ mkdir dir

$ touch file

$ node -e 'require("fs").renameSync("dir", "file")'
internal/fs/utils.js:259
    throw err;
    ^

Error: ENOTDIR: not a directory, rename 'dir' -> 'file'
    at Object.renameSync (fs.js:756:3)
    at [eval]:1:15
    at Script.runInThisContext (vm.js:131:20)
    at Object.runInThisContext (vm.js:297:38)
    at Object.<anonymous> ([eval]-wrapper:10:26)
    at Module._compile (internal/modules/cjs/loader.js:1176:30)
    at evalScript (internal/process/execution.js:94:25)
    at internal/main/eval_string.js:23:3 {
  errno: -20,
  syscall: 'rename',
  code: 'ENOTDIR',
  path: 'dir',
  dest: 'file'
}

$ node -e 'require("fs").renameSync("file", "dir")'
internal/fs/utils.js:259
    throw err;
    ^

Error: EISDIR: illegal operation on a directory, rename 'file' -> 'dir'
    at Object.renameSync (fs.js:756:3)
    at [eval]:1:15
    at Script.runInThisContext (vm.js:131:20)
    at Object.runInThisContext (vm.js:297:38)
    at Object.<anonymous> ([eval]-wrapper:10:26)
    at Module._compile (internal/modules/cjs/loader.js:1176:30)
    at evalScript (internal/process/execution.js:94:25)
    at internal/main/eval_string.js:23:3 {
  errno: -21,
  syscall: 'rename',
  code: 'EISDIR',
  path: 'file',
  dest: 'dir'
}

[isaacs]

Unreachable code.

[isaacs]

This is extremely hazardous, as it has the potential to lock up the event loop for up to 5 seconds.

It'd probably be safer to just let synchronous renames fail.

[jgoz]

@isaacs Thank you for taking a look at this. Since I'm not the original author of these changes and it's been nearly 2 years since I looked at this, I don't think I will be able to adequately address your concerns. I'm not sure they are even necessary anymore since many packages have since stopped using graceful-fs.

If someone else would like to take over this PR, let me know and I'll add you as a collaborator on my fork.