Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move hangs when given a target file path that is located in a root directory #1011

Closed
rotemdan opened this issue Aug 5, 2023 · 5 comments
Closed

Comments

@rotemdan
Copy link

rotemdan commented Aug 5, 2023

  • Operating System: Windows 10 x64
  • Node.js version: v18.17.0
  • fs-extra version: 11.1.1

Reproduction:

import { move } from "fs-extra"

console.log("Moving..")
await move("D:\\test\\test.txt", "D:\\test.txt")
console.log("Done")

Stuck at Moving...

Debugging shows it gets trapped in some sort of a loop performing stat multiple times and possibly failing.

I'm surprised this hasn't been reported yet? (as far as I know)

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 5, 2023

AFAIK, it hasn't been reported. What's strange is that we had an issue about an EPERM error when moving to the root of a drive a few years ago: #819, which was fixed in #897. From what I can see, we haven't changed the code in question since then, and I'm surprised we didn't catch it then.

I don't have access to a Windows machine (these problems are Windows-exclusive, since you basically never write to root in a *nix OS), so I'm rather incapable of debugging this myself. You mentioned you did some debugging, were you able to isolate which stat call was looping?

@rotemdan
Copy link
Author

rotemdan commented Aug 5, 2023

The drive seem to have, in general, permissions issues with writing to the root directory from Node.js (but not to any other directory, and not through Windows explorer), which I was investigating due to a user's report.

After more investigation, it turns out Error: EPERM: operation not permitted errors occur with copyFile and createWriteStream.

What was strange is that move doesn't output the error, it seems to try to deal with it and gets stuck in a loop.

Based on step debugging, it may be that this code is relevant:

  // on Windows, A/V software can lock the directory, causing this
  // to fail with an EACCES or EPERM if the directory contains newly
  // created files.  Try again on failure, for up to 60 seconds.

  // Set the timeout this long because some Windows Anti-Virus, such as Parity
  // bit9, may lock files for up to a minute, causing npm package install
  // failures. Also, take care to yield the scheduler. Windows scheduling gives
  // CPU to a busy looping process, which can cause the program causing the lock
  // contention to be starved of CPU by node, so the contention doesn't resolve.
  if (platform === "win32") {
    fs.rename = typeof fs.rename !== 'function' ? fs.rename
    : (function (fs$rename) {
      function rename (from, to, cb) {
        var start = Date.now()
        var backoff = 0;
        fs$rename(from, to, function CB (er) {
          if (er
              && (er.code === "EACCES" || er.code === "EPERM" || er.code === "EBUSY")
              && Date.now() - start < 60000) {
            setTimeout(function() {
              fs.stat(to, function (stater, st) {
                if (stater && stater.code === "ENOENT")
                  fs$rename(from, to, CB);
                else
                  cb(er)
              })
            }, backoff)
            if (backoff < 100)
              backoff += 10;
            return;
          }
          if (cb) cb(er)
        })
      }
      if (Object.setPrototypeOf) Object.setPrototypeOf(rename, fs$rename)
      return rename
    })(fs.rename)
  }

It seems to keep setting timeouts in a loop when it gets a permission error writing to the target file.

The code says it gives up after 60 seconds. After measuring, it does actually give up. I guess I assumed it continues forever.

I mean from a user's perspective, 60 seconds is forever, and since there is no indication, they assume it'll never end. Maybe there's a better way to detect cases when the permission errors are caused by actual problems writing to the disk?

Anyway, I'm not sure it's related to the root directory anymore (though that's the way it was reported to me), it's just that I guess it's common to have permission problems with writing to it, and the user was trying to write an JSON file to his D:\ root directory (not system drive), via my program.

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 7, 2023

Yeah, it's a known thing that Windows gives an EPERM for writing to root in some cases. It's also the case that Windows randomly gives EPERM errors due to antivirus software, hence the retry logic in graceful-fs. Not sure that there's really any good solution here.

@rotemdan
Copy link
Author

rotemdan commented Aug 7, 2023

The thing is, the goal of using move was to have atomic, or at least near-atomic write operation, ensuring there are no partially written files at the target directory. 60 second wait is frustrating for users, and is effectively perceived as "eternity".

My current (non-ideal) workaround, unfortunately, is to first try to create a randomly named file at the target directory, check if I get a permission error (and error if it happens), then delete the file, and then call move (this can create a bit of random junk files at some rare situations).

Why? Because the most reliable way I've found, so far, to provide the end-user a reasonable user experience when there are permission errors (especially on Windows).

Another approach would be to rewrite move myself, but that sort of defeats the purpose.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 24, 2023

fs-extra exists to provide additional convenience for the 90% use-case. graceful-fs' EPERM retry logic for antivirus is a part of that. Unfortunately, that degrades the experience when attempting to write to a location you don't have permissions for. It's a tradeoff, but in this case, I think it's a necessary one. If you need a different set of tradeoffs for your use-case, custom code is probably the best route.

@RyanZim RyanZim closed this as completed Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants