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

Improved workarounds for win32 #119

Closed
wants to merge 18 commits into from
Closed

Conversation

mekwall
Copy link

@mekwall mekwall commented Oct 10, 2017

Summary

This PR improves on the existing override for fs.rename and also adds a similar but blocking override for fs.renameSync (obsoleting #23). The overrides tries to normalize the behavior of fs.rename and fs.renameSync between platforms. It does this by ensuring that the file was actually moved before resolving within a 60 second time frame.

Background

fs.rename and fs.renameSync uses MoveFileEx function on Windows. It is not an atomic operation and honors the Windows sharing modes, meaning that whenever a file or parent directory is locked (in use) the rename might fail with EACCS or EPERM errors depending on the sharing mode set on the file and/or directory.

This differs from how the api call used by Linux and OSX works where the rename operation is atomic and will go through no matter if the file is locked or not.

Disabling anti-virus is NOT a viable solution

Most active anti-virus file scanners will lock the file and/or directory during scan, so if you are trying to rename it during this time you'll get an EPERM/EACCS error thrown in your face. Just to be clear: Disabling AV is NOT a viable solution and should never be accepted as such.

Issues possibly affected by this

References

@mekwall mekwall changed the title Stat from file to make sure it was renamed Stat destination file to make sure it was moved Oct 10, 2017
@mekwall mekwall changed the title Stat destination file to make sure it was moved Stat source file to make sure it was moved Oct 10, 2017
@mekwall mekwall changed the title Stat source file to make sure it was moved Improved workarounds for win32 Oct 11, 2017
polyfills.js Outdated
fs.stat(from, function (fromStater, fromSt) {
if (!fromSt && toSt) {
if (cb) cb(toStater ? er : null)
} else if (fromSt && toSt && fromSt.size === toSt.size) {
Copy link
Author

@mekwall mekwall Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really fond about this, so could use some feedback/input on how it could be solved differently.

polyfills.js Outdated
throw e
}
// Wait until destination exists and source no longer exists or that we've reached the backoff limit
while ((fs.existsSync(from) || !fs.existsSync(to)) && Date.now() < backoffUntil) {}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly the worst hack ever, but it seems to be the only real way to solve inconsistent behavior on Windows where the destination and source can exist at the same time.

@conceptualspace
Copy link

conceptualspace commented Nov 7, 2017

testing this out on a windows machine lead to crash:

"error": "TypeError: Cannot read property 'size' of undefined", "errorStackTrace": "TypeError: Cannot read property 'size' of undefined\n at node_modules\graceful-fs\polyfills.js:112:45\n at node_modules\graceful-fs\polyfills.js:331:29\n at FSReqWrap.oncomplete (fs.js:114:15)"

taking a quick look -- perhaps there needs to be error handling on the first call to fs.stat?

Node v7.9.0
Windows Server 2008

@mekwall
Copy link
Author

mekwall commented Nov 7, 2017

@conceptualspace I've added some additional sanity checks that should solve your crash. Thanks for testing and reporting the issue!

@conceptualspace
Copy link

that would be fine if node actually reported an EEXIST error, but it does not on windows -- we just get EACCS/EPERM if the destination already exists

@conceptualspace
Copy link

conceptualspace commented Nov 9, 2017

> fs.mkdirSync('test1');
> fs.mkdirSync('test2');
> fs.rename('test2', 'test1', function(err) {console.log(err);});
Error: EPERM: operation not permitted, rename 'D:\Dev\test2' -> 'D:\Dev\test1'

win 10x64
node v8.4.0

@mekwall
Copy link
Author

mekwall commented Nov 10, 2017

This only happens for directories and if you do the same on Linux/OSX you'll see it works just fine since the native rename in Linux will overwrite an existing target no matter what. So if we want to normalize this behavior, we should probably check for an existing target and unlink it if it exists.

fs.mkdir will throw Error: EEXIST: file already exists on all platforms if you try and create a dir that already existsw.

@mekwall
Copy link
Author

mekwall commented Nov 10, 2017

The current polyfill will get stuck on Windows at renaming a directory when the target already exists. I think that we should make sure it works the same on both platform (i.e. remove the target directory if it exists).

I tested using the following code: https://gist.github.com/mekwall/b6c44c6855aa9adbd97af1bba9386e50

@mekwall
Copy link
Author

mekwall commented Nov 10, 2017

The last commit normalizes the behavior. This is what I get with the test code from my gist linked above.

Windows:

FS
Trying to rename dir
Failed!
GFS
Trying to rename dir
Success!

Linux:

FS
Trying to rename dir
Success!
GFS
Trying to rename dir
Success!

@vladimiry
Copy link

vladimiry commented Nov 24, 2017

I was playing around the EPERM issue during the day and it's now clear that the issue's cause is indeed in the resource locking as was written above by @mekwall. This is the way I've got it proven for myself:

  • I open the file for reading and close it (release the locking) only after the 20 seconds:
        const fd = await promisify(fs.open)(this.file, "r+");

        setTimeout(async () => {
            await promisify(fs.close)(fd);    
        }, 20 * 1000);
  • Then in another workflow I run fs.rename with the target set to the same opened file applying backoff retry for 60 seconds max and I get file renaming successfully completed after the 20 seconds - exactly after releasing the file locking doing await promisify(fs.close)(fd);.

So backoff retry scenario should be really helpful, following the @mekwall 's idea. Antiviruses, Dropbox syncing and such kind of software apparently open files for a long time and very frequently, that's why there are so many EPERM issues posted on Github (which usually go to the close state without the actual resolution). So if there is no retry scenario, but just a single renaming attempt (or other actions that might cause EPERM, not only the renaming), then we can likely get EPERM error on Windows happening, that's it.

@vladimiry
Copy link

vladimiry commented Nov 25, 2017

This is the test case I mentioned above https://github.com/vladimiry/fs-no-eperm-anymore/tree/master/src/test/locking - locked fs.rename call.

@JoshuaKGoldberg
Copy link

@isaacs do you think you could please take a look?

@christiango
Copy link

What's the latest on this PR? It would be great to get this in to fix the issues we are seeing with the latest versions of Jest on Windows

@dogboydog
Copy link

I encounter the EPERM issues very frequently still as do my colleagues. A colleague recently got a new workstation with windows 10, and could not do npm install with any version of npm he tried. He got the closest with npm 4.2.0. We tried blowing out the graceful-fs dependency used by npm in his installation with the files from this pull request, which fixed his issues. I hope this fix becomes a part of npm soon. Thank you for fixing it

@RyanCavanaugh
Copy link

Thanks @mekwall for writing this patch! I work on TypeScript, Visual Studio, and Visual Studio Code. The underlying problem described here is coming up a lot for VS developers because the TypeScript language service puts a file watcher on the project folder (including when the user is just writing JavaScript). We typically see errors like this:

npm ERR!   stack: 'Error: EPERM: operation not permitted, rename \'C:\\foo\\node_modules\\pug-error\\package.json.731450482\' -> \'C:\\foo\\node_modules\\pug-error\\package.json\''

As the PR describes, this is definitely related to file watchers - in my testing, if we disable the file watching from the TSLS side, the error stops happening.

I've done some testing and this PR definitely fixes the issue. The particular scenario we're trying to make work has gone from failing 1 in 4 times to succeeding the last 35 times in a row.

We really need this to be merged - it's impacting a lot of developers and there's nothing that can be done on our end. Developers running npm install while VS or VS Code are open with TS or JS files in the editor end up seeing this error all the time and it's going to result in a lot of support noise on the NPM and VS side.

@rjgotten
Copy link

@mekwall

Thank you, thank you, thank you, thank you, thank yo-----

It may take longer to run a big npm install pass, but unlike any other solution including all the stuff the NPM team has been throwing up against the wall in a bid to see what sticks, this actually WORKS.

@nullstd
Copy link

nullstd commented Apr 10, 2018

NPM team really needs to merge this PR ASAP, could any guys in their team give a response?

@isaacs
Copy link
Owner

isaacs commented May 15, 2018

Hi. It seems like there's a semantic change here. If the target directory is not empty, then it should fail with ENOTEMPTY. But, the fs.rmdir call (which should error in that case) is ignoring the error.

Also, I'm not sure it's a good idea to spin on CPU for potentially 60 seconds in this case. Is it possible to make this time shorter, or allow it to be configured?

@mekwall
Copy link
Author

mekwall commented May 29, 2018

Sadly I don't have time to fix this at the moment. Any takers?

@jgoz
Copy link

jgoz commented Jun 27, 2018

@mekwall @isaacs Addressed feedback in #131.

@mekwall
Copy link
Author

mekwall commented Jul 18, 2018

Closing this in favor of #131.

Copy link

@manjaneqx manjaneqx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

Copy link

@manjaneqx manjaneqx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y

Copy link

@manjaneqx manjaneqx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y

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

Successfully merging this pull request may close these issues.