Improved workarounds for win32, continued #131
Conversation
|
@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 |
|
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? |
|
@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. |
|
@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. |
jgoz
force-pushed
the
win32-rename-override
branch
from
313dc2e to
314e721
Compare
|
👍 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? |
| var stat = fs.statSync(to) | ||
| if (!stat) return | ||
| if (stat.isDirectory()) { | ||
| fs.rmdirSync(to) |
There was a problem hiding this comment.
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'
}
| if (platform === "win32") { | ||
| fs.rename = (function (fs$rename) { return function (from, to, cb) { | ||
| try { | ||
| var stat = fs.statSync(to) | ||
| if (!stat) return |
| if (backoff < 100) | ||
| backoff += 10 | ||
| var waitUntil = Date.now() + backoff | ||
| while (waitUntil > Date.now()){} |
There was a problem hiding this comment.
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.
|
@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 If someone else would like to take over this PR, let me know and I'll add you as a collaborator on my fork. |
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:
process.env.GRACEFUL_FS_WIN32_MAX_BACKOFFcbcallsCloses #119.