Trying to rename() a directory over another fails slowly on Windows

[binki]

$ mkdir -p a b && time node -e "require('.').rename('a', 'b', err => console.log(err))"
{ Error: EPERM: operation not permitted, rename 'C:\Users\ohnob\repos\node-graceful-fs\a' -> 'C:\Users\ohnob\repos\node-graceful-fs\b'
    at Error (native)
  errno: -4048,
  code: 'EPERM',
  syscall: 'rename',
  path: 'C:\\Users\\ohnob\\repos\\node-graceful-fs\\a',
  dest: 'C:\\Users\\ohnob\\repos\\node-graceful-fs\\b' }

real    1m0.326s
user    0m0.000s
sys     0m0.031s

It should fail instantly, like with fs.rename:

$ mkdir -p a b && time node -e "require('fs').rename('a', 'b', err => console.log(err))"
{ Error: EPERM: operation not permitted, rename 'C:\Users\ohnob\repos\node-graceful-fs\a' -> 'C:\Users\ohnob\repos\node-graceful-fs\b'
    at Error (native)
  errno: -4048,
  code: 'EPERM',
  syscall: 'rename',
  path: 'C:\\Users\\ohnob\\repos\\node-graceful-fs\\a',
  dest: 'C:\\Users\\ohnob\\repos\\node-graceful-fs\\b' }

real    0m0.284s
user    0m0.000s
sys     0m0.031s

$ git describe --long
v4.1.10-0-gdb8df44

[binki]

This is a regression. Renaming a directory over another used to fail fairly instantly in graceful-fs-4.1.9.

[isaacs]

This is because of 59145ce. @sam-github, any thoughts?

[binki]

I know it would be racy, but retrying is already introducing potential race conditions. I don’t think that the return error code of fs.rename() gives you enough information to determine that waiting should happen. Perhaps upon getting an error, check if the destination folder already exists and, if so, return the failure instantly as a special case?

[sam-github]

So, it used to retry, it just retries longer now. Though enough longer, I guess you are noticing it?

The existing code checked for `er.code === "EACCES" || er.code === "EPERM", it only retries if it is one of those. I agree, it looks like the underlying node error code doesn't contain enough information to know if its a temporary failure, to be retried, or permanent.

The slow retry loop that gives time for virus checkers to complete could be moved out of graceful-fs, and into npm. I'm not clear if it is a loop that should ever have been in graceful-fs... I understand graceful-fs to retry on fd exhuastion, but should it also retry on fs errors?

@isaacs, what is this history? Is this retry loop something that should be hoisted into npm proper, out of graceful-fs?

[binki]

I personally like that this loop is in graceful-fs because this is a Windows weirdness that people should be able to magically supporting just by using it. But I think that with the right type of operations, even the short 1s loop could cause programs which run fine on Unix to be unusably slow on Windows if they rely on rename() throwing errors (though maybe such patterns are actually uncommon) such as the references I made to this issue from a test suite which broke after this change. So how about we just add detection of known situations where this error is permanent (in this case, destination existing) and fail instantly?

[binki]

Here’s the output from graceful-fs-4.1.9 for reference, especially now that I know why this is 1.275s (compared to fs.rename() which is 0.284s—probably mostly just node startup/compilation time):

$ mkdir -p a b && time node -e "require('graceful-fs').rename('a', 'b', err => console.log(err))"
{ Error: EPERM: operation not permitted, rename 'C:\Users\ohnob\repos\node-fs-extra\a' -> 'C:\Users\ohnob\repos\node-fs-extra\b'
    at Error (native)
  errno: -4048,
  code: 'EPERM',
  syscall: 'rename',
  path: 'C:\\Users\\ohnob\\repos\\node-fs-extra\\a',
  dest: 'C:\\Users\\ohnob\\repos\\node-fs-extra\\b' }

real    0m1.275s
user    0m0.000s
sys     0m0.015s

[isaacs]

I'd be fine with statting the target to see if it exists. Anything but a failure with ENOENT would trigger the retry.

[isaacs]

@binki Can you npm i isaacs/node-graceful-fs#windows-rename-polyfill-slow in your project and see if it fixes the issue for you?

If it does, then I'll land and publish this as a patch release.

[binki]

@isaacs That seems to fix it. The operation looks like it runs instantly so that the project’s tests run fine without having to extend the timeout. Thanks!