From bf4569d9e18cbcb647ac26f9b7fbdccc1593d28f Mon Sep 17 00:00:00 2001 From: Marcus Ekwall Date: Tue, 10 Oct 2017 18:01:20 +0200 Subject: [PATCH 01/23] Stat from file to make sure it was renamed --- polyfills.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/polyfills.js b/polyfills.js index ab69201..71e46f3 100644 --- a/polyfills.js +++ b/polyfills.js @@ -98,11 +98,17 @@ function patch (fs) { && (er.code === "EACCES" || er.code === "EPERM") && Date.now() - start < 60000) { setTimeout(function() { - fs.stat(to, function (stater, st) { - if (stater && stater.code === "ENOENT") - fs$rename(from, to, CB); + fs.stat(to, function (toStater, toSt) { + if (toStater && toStater.code === "ENOENT") + fs$rename(from, to, CB) else - cb(er) + fs.stat(from, function (fromStater, fromSt) { + if (!fromSt && toSt) { + if (cb) cb(er) + } else { + fs$rename(from, to, CB) + } + }) }) }, backoff) if (backoff < 100) From eb6c68dee5a2f49cb77f1a613e55ad4db313b0bc Mon Sep 17 00:00:00 2001 From: mekwall Date: Tue, 10 Oct 2017 18:52:46 +0200 Subject: [PATCH 02/23] Resolve if source and destination is the same size --- polyfills.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polyfills.js b/polyfills.js index 71e46f3..2acb378 100644 --- a/polyfills.js +++ b/polyfills.js @@ -103,8 +103,8 @@ function patch (fs) { fs$rename(from, to, CB) else fs.stat(from, function (fromStater, fromSt) { - if (!fromSt && toSt) { - if (cb) cb(er) + if (!fromSt && toSt || fromSt && toSt && fromSt.size === toSt.size) { + if (cb) cb(toStater ? er : null) } else { fs$rename(from, to, CB) } From f37b06af65a5aa1b4a80467329649dfe91d225db Mon Sep 17 00:00:00 2001 From: mekwall Date: Tue, 10 Oct 2017 18:53:14 +0200 Subject: [PATCH 03/23] Except rename to not return an error --- test/windows-rename-polyfill.js | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/test/windows-rename-polyfill.js b/test/windows-rename-polyfill.js index f48ba74..ba10d06 100644 --- a/test/windows-rename-polyfill.js +++ b/test/windows-rename-polyfill.js @@ -1,14 +1,6 @@ process.env.GRACEFUL_FS_PLATFORM = 'win32' var fs = require('fs') -fs.rename = function (a, b, cb) { - setTimeout(function () { - var er = new Error('EPERM blerg') - er.code = 'EPERM' - cb(er) - }) -} - var gfs = require('../') var t = require('tap') var a = __dirname + '/a' @@ -22,9 +14,8 @@ t.test('setup', function (t) { t.test('rename', { timeout: 100 }, function (t) { t.plan(1) - gfs.rename(a, b, function (er) { - t.ok(er) + t.ok(!er) }) }) From e85b6c0762f3832bb1dfbbfc272c1f3d384eeab3 Mon Sep 17 00:00:00 2001 From: mekwall Date: Tue, 10 Oct 2017 19:20:49 +0200 Subject: [PATCH 04/23] Unlink source if it's the same as destination --- polyfills.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/polyfills.js b/polyfills.js index 2acb378..a804d0c 100644 --- a/polyfills.js +++ b/polyfills.js @@ -103,8 +103,12 @@ function patch (fs) { fs$rename(from, to, CB) else fs.stat(from, function (fromStater, fromSt) { - if (!fromSt && toSt || fromSt && toSt && fromSt.size === toSt.size) { + if (!fromSt && toSt) { if (cb) cb(toStater ? er : null) + } else if (fromSt && toSt && fromSt.size === toSt.size) { + fs.unlink(from, function () { + cb(toStater ? er : null) + }) } else { fs$rename(from, to, CB) } From 8c177e4198710f9df22241a1452efabf8eb97f3b Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 09:52:06 +0200 Subject: [PATCH 05/23] Do not unlink file --- polyfills.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/polyfills.js b/polyfills.js index a804d0c..56f0137 100644 --- a/polyfills.js +++ b/polyfills.js @@ -106,9 +106,7 @@ function patch (fs) { if (!fromSt && toSt) { if (cb) cb(toStater ? er : null) } else if (fromSt && toSt && fromSt.size === toSt.size) { - fs.unlink(from, function () { - cb(toStater ? er : null) - }) + if (cb) cb(null) } else { fs$rename(from, to, CB) } From e6c706ec7df6c7960ccc3c2780c3988270a67eb8 Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 09:58:36 +0200 Subject: [PATCH 06/23] Add fs.renameSync for win32 --- polyfills.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/polyfills.js b/polyfills.js index 56f0137..aad5489 100644 --- a/polyfills.js +++ b/polyfills.js @@ -120,6 +120,29 @@ function patch (fs) { if (cb) cb(er) }) }})(fs.rename) + + + fs.renameSync = (function (fs$renameSync) { return function (from, to) { + var start = Date.now() + var backoff = 0; + var backoffUntil = start + 60000; + function tryRename () { + try { + fs$renameSync(from, to) + } catch (e) { + if ((e.code === "EACCS" || e.code === "EPERM") && start < backoffUntil) { + if (backoff < 100) + backoff += 10 + var waitUntil = Date.now() + backoff + while (waitUntil > Date.now()){} + tryRename() + } else { + throw e + } + } + } + tryRename() + }})(fs.renameSync) } // if read() returns EAGAIN, then just try it again. From 34dadcf24bc0e36c5781431963171def70dad373 Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 09:59:10 +0200 Subject: [PATCH 07/23] Remove extraneous line --- polyfills.js | 1 - 1 file changed, 1 deletion(-) diff --git a/polyfills.js b/polyfills.js index aad5489..6a8f453 100644 --- a/polyfills.js +++ b/polyfills.js @@ -121,7 +121,6 @@ function patch (fs) { }) }})(fs.rename) - fs.renameSync = (function (fs$renameSync) { return function (from, to) { var start = Date.now() var backoff = 0; From 0131d836a34e0cfd016db4e2c4ab90a549b098b9 Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 12:57:07 +0200 Subject: [PATCH 08/23] Improve stat compare between src and dest --- polyfills.js | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/polyfills.js b/polyfills.js index 6a8f453..d358ca5 100644 --- a/polyfills.js +++ b/polyfills.js @@ -93,31 +93,29 @@ function patch (fs) { fs.rename = (function (fs$rename) { return function (from, to, cb) { var start = Date.now() var backoff = 0; + var backoffUntil = start + 60000; fs$rename(from, to, function CB (er) { - if (er - && (er.code === "EACCES" || er.code === "EPERM") - && Date.now() - start < 60000) { + if (er && (er.code === "EACCES" || er.code === "EPERM") && Date.now() < backoffUntil) { setTimeout(function() { - fs.stat(to, function (toStater, toSt) { - if (toStater && toStater.code === "ENOENT") - fs$rename(from, to, CB) - else - fs.stat(from, function (fromStater, fromSt) { - if (!fromSt && toSt) { - if (cb) cb(toStater ? er : null) - } else if (fromSt && toSt && fromSt.size === toSt.size) { - if (cb) cb(null) - } else { - fs$rename(from, to, CB) - } - }) + fs.stat(from, function (erFrom, statFrom) { + if (erFrom) return cb(erFrom) + fs.stat(to, function (erTo, statTo) { + if (statFrom.size === statTo.size && + statFrom.ctime === statTo.ctime) { + cb(null) + } else + fs$rename(from, to, CB) + }); }) }, backoff) - if (backoff < 100) + if (backoff < 250) backoff += 10; - return; + } else if (backoff && er && er.code === "ENOENT") { + // The source does no longer exist so we can assume it was moved during one of the tries + if (cb) cb(null) + } else { + if (cb) cb(er) } - if (cb) cb(er) }) }})(fs.rename) From e5e958f2ba783bdf93b2a5d4c8583024036df8d5 Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 12:57:47 +0200 Subject: [PATCH 09/23] Ignore ENOENT for src on retry --- polyfills.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/polyfills.js b/polyfills.js index d358ca5..810b9e2 100644 --- a/polyfills.js +++ b/polyfills.js @@ -133,6 +133,9 @@ function patch (fs) { var waitUntil = Date.now() + backoff while (waitUntil > Date.now()){} tryRename() + } else if (backoff > 0 && e.code === "ENOENT") { + // The source does no longer exist because it was moved during one of the retries + // so we can safely ignore this exception } else { throw e } From 3384945a62b0ba02db72d93bcc7a59d2632f41e3 Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 12:58:14 +0200 Subject: [PATCH 10/23] Wait until dest exist and src does not --- polyfills.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/polyfills.js b/polyfills.js index 810b9e2..8a25fc2 100644 --- a/polyfills.js +++ b/polyfills.js @@ -139,6 +139,8 @@ function patch (fs) { } else { 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) {} } } tryRename() From 73cab418a21f5318807bc3ad0fce7c58ab3480f1 Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 12:58:59 +0200 Subject: [PATCH 11/23] Revamped rename tests --- test/windows-rename-polyfill.js | 68 +++++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 11 deletions(-) diff --git a/test/windows-rename-polyfill.js b/test/windows-rename-polyfill.js index ba10d06..ed09740 100644 --- a/test/windows-rename-polyfill.js +++ b/test/windows-rename-polyfill.js @@ -3,24 +3,70 @@ process.env.GRACEFUL_FS_PLATFORM = 'win32' var fs = require('fs') var gfs = require('../') var t = require('tap') -var a = __dirname + '/a' -var b = __dirname + '/b' +var tmpDir = __dirname + '/tmp' +var testFiles = []; +var id = 0; -t.test('setup', function (t) { - try { fs.mkdirSync(a) } catch (e) {} - try { fs.mkdirSync(b) } catch (e) {} +function anyFileExists (files) { + var exists = false; + for (var i = 0, len = files.length; i < len; i++) { + if (fs.existsSync(files[i])) + return true + } + return false +} + +t.test('setup async', function (t) { + try { fs.mkdirSync(tmpDir) } catch (e) {} + for (var i = 0; i < 500; i++) { + var testFile = tmpDir + '/test-' + id++ + fs.writeFileSync(testFile, id) + testFiles.push(testFile) + } + t.end() +}) + +t.test('rename async', { timeout: 60000 }, function (t) { + t.plan(testFiles.length * 2) + var dest = tmpDir + '/test' + testFiles.forEach((src) => { + gfs.rename(src, dest, function (er) { + t.error(er, 'Failed to rename file', er) + t.notOk(fs.existsSync(src), 'Source file still exists:' + src) + }) + }) +}) + +t.test('setup sync', function (t) { + try { fs.mkdirSync(tmpDir) } catch (e) {} + testFiles = [] + for (var i = 0; i < 100; i++) { + var testFile = tmpDir + '/test-' + id++ + fs.writeFileSync(testFile, id) + testFiles.push(testFile) + } t.end() }) -t.test('rename', { timeout: 100 }, function (t) { - t.plan(1) - gfs.rename(a, b, function (er) { - t.ok(!er) +t.test('rename sync', { timeout: 60000 }, function (t) { + t.plan((testFiles.length * 2) + 1) + var done = 0; + var errors = 0; + var dest = tmpDir + '/test' + testFiles.forEach((src) => { + var srcData = fs.readFileSync(src).toString() + t.doesNotThrow(function () { gfs.renameSync(src, dest) }, 'Exception thrown when renaming') + var destData = fs.readFileSync(dest).toString() + t.equal(srcData, destData, 'Data between source and destination differs: ' + srcData + ' !== ' + destData) }) + t.notOk(anyFileExists(testFiles), 'Some source files still exist') }) t.test('cleanup', function (t) { - try { fs.rmdirSync(a) } catch (e) {} - try { fs.rmdirSync(b) } catch (e) {} + testFiles.forEach(function (file) { + try { fs.removeSync(file) } catch (e) {} + }) + try { fs.removeSync(tmpDir + '/test') } catch (e) {} + try { fs.rmdirSync(tmpDir) } catch (e) {} t.end() }) From c6581c9797e26537fdaad27b8721b27fd9ce052d Mon Sep 17 00:00:00 2001 From: mekwall Date: Wed, 11 Oct 2017 13:02:34 +0200 Subject: [PATCH 12/23] Remove sneaky arrow functions --- test/windows-rename-polyfill.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/windows-rename-polyfill.js b/test/windows-rename-polyfill.js index ed09740..e92189a 100644 --- a/test/windows-rename-polyfill.js +++ b/test/windows-rename-polyfill.js @@ -29,7 +29,7 @@ t.test('setup async', function (t) { t.test('rename async', { timeout: 60000 }, function (t) { t.plan(testFiles.length * 2) var dest = tmpDir + '/test' - testFiles.forEach((src) => { + testFiles.forEach(function (src) { gfs.rename(src, dest, function (er) { t.error(er, 'Failed to rename file', er) t.notOk(fs.existsSync(src), 'Source file still exists:' + src) @@ -53,7 +53,7 @@ t.test('rename sync', { timeout: 60000 }, function (t) { var done = 0; var errors = 0; var dest = tmpDir + '/test' - testFiles.forEach((src) => { + testFiles.forEach(function (src) { var srcData = fs.readFileSync(src).toString() t.doesNotThrow(function () { gfs.renameSync(src, dest) }, 'Exception thrown when renaming') var destData = fs.readFileSync(dest).toString() From 2a1fe1f4936d2ef87dffedfc6bdea3147dc0e17f Mon Sep 17 00:00:00 2001 From: mekwall Date: Thu, 12 Oct 2017 10:37:37 +0200 Subject: [PATCH 13/23] Updated comments and formatting --- polyfills.js | 44 +++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/polyfills.js b/polyfills.js index 8a25fc2..34d18fc 100644 --- a/polyfills.js +++ b/polyfills.js @@ -80,15 +80,19 @@ function patch (fs) { fs.lchownSync = function () {} } - // 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. + // fs.rename and fs.renameSync uses MoveFileEx function on Windows. + // MoveFileEx is not atomic and honors Windows sharing modes, compared to + // os/syscall.rename used by Linux and OSX that are atomic and does not + // care if the file or directory is locked. + // + // This means that whenever a file or parent directory is locked (in use) + // on Windows the rename might fail with EACCS or EPERM errors depending + // the sharing mode set on the file and/or directory. + // + // These win32-only overrides try to normalize fs.rename/renameSync + // behavior so it's more in line with how it works on Linux and OSX. + // It does this by retrying a failed rename for up to 60 seconds until + // actually failing. if (platform === "win32") { fs.rename = (function (fs$rename) { return function (from, to, cb) { var start = Date.now() @@ -98,10 +102,11 @@ function patch (fs) { if (er && (er.code === "EACCES" || er.code === "EPERM") && Date.now() < backoffUntil) { setTimeout(function() { fs.stat(from, function (erFrom, statFrom) { - if (erFrom) return cb(erFrom) fs.stat(to, function (erTo, statTo) { - if (statFrom.size === statTo.size && - statFrom.ctime === statTo.ctime) { + if ( + statFrom.size === statTo.size && + statFrom.ctime === statTo.ctime + ) { cb(null) } else fs$rename(from, to, CB) @@ -111,7 +116,8 @@ function patch (fs) { if (backoff < 250) backoff += 10; } else if (backoff && er && er.code === "ENOENT") { - // The source does no longer exist so we can assume it was moved during one of the tries + // The source does no longer exist so we + // can assume it was moved during one of the tries if (cb) cb(null) } else { if (cb) cb(er) @@ -134,13 +140,17 @@ function patch (fs) { while (waitUntil > Date.now()){} tryRename() } else if (backoff > 0 && e.code === "ENOENT") { - // The source does no longer exist because it was moved during one of the retries - // so we can safely ignore this exception + // The source does no longer exist because so we can + // assume it was moved } else { 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) {} + // 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 + ) {} } } tryRename() From 476117a250de88551775ebd8741df71a8e1ef603 Mon Sep 17 00:00:00 2001 From: mekwall Date: Thu, 12 Oct 2017 10:38:15 +0200 Subject: [PATCH 14/23] Assume rename was successful if source no longer exists --- polyfills.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/polyfills.js b/polyfills.js index 34d18fc..aabe5d4 100644 --- a/polyfills.js +++ b/polyfills.js @@ -103,7 +103,11 @@ function patch (fs) { setTimeout(function() { fs.stat(from, function (erFrom, statFrom) { fs.stat(to, function (erTo, statTo) { - if ( + if (erFrom && !erTo) { + // If the source no longer exists we + // can probably assume it was moved + cb(null) + } else if ( statFrom.size === statTo.size && statFrom.ctime === statTo.ctime ) { From 2ab3f1558ddf94a911f1e60380acbc6973c94b41 Mon Sep 17 00:00:00 2001 From: mekwall Date: Tue, 7 Nov 2017 17:59:35 +0100 Subject: [PATCH 15/23] Make sure both statFrom and statTo are set --- polyfills.js | 1 + 1 file changed, 1 insertion(+) diff --git a/polyfills.js b/polyfills.js index aabe5d4..f4754a7 100644 --- a/polyfills.js +++ b/polyfills.js @@ -108,6 +108,7 @@ function patch (fs) { // can probably assume it was moved cb(null) } else if ( + statFrom && statTo && statFrom.size === statTo.size && statFrom.ctime === statTo.ctime ) { From d3dc056f4e4e17a340c713cf65466552adeabf1b Mon Sep 17 00:00:00 2001 From: mekwall Date: Tue, 7 Nov 2017 18:01:56 +0100 Subject: [PATCH 16/23] Add additional comment about size/ctime comparison --- polyfills.js | 3 +++ test/tmp/test | 1 + 2 files changed, 4 insertions(+) create mode 100644 test/tmp/test diff --git a/polyfills.js b/polyfills.js index f4754a7..9b5476c 100644 --- a/polyfills.js +++ b/polyfills.js @@ -112,6 +112,9 @@ function patch (fs) { statFrom.size === statTo.size && statFrom.ctime === statTo.ctime ) { + // If the source and target have + // the same size and ctime, we + // can assume it was moved cb(null) } else fs$rename(from, to, CB) diff --git a/test/tmp/test b/test/tmp/test new file mode 100644 index 0000000..73623d1 --- /dev/null +++ b/test/tmp/test @@ -0,0 +1 @@ +600 \ No newline at end of file From 5af97e5e4b411cd539ee4c2b172a65dce577521d Mon Sep 17 00:00:00 2001 From: mekwall Date: Fri, 10 Nov 2017 11:34:59 +0100 Subject: [PATCH 17/23] Unlink target it if exists --- polyfills.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/polyfills.js b/polyfills.js index 9b5476c..ce50f12 100644 --- a/polyfills.js +++ b/polyfills.js @@ -95,6 +95,15 @@ function patch (fs) { // actually failing. if (platform === "win32") { fs.rename = (function (fs$rename) { return function (from, to, cb) { + try { + var stat = fs.statSync(to) + if (!stat) return + if (stat.isDirectory()) { + fs.rmdirSync(to) + } else { + fs.unlinkSync(to) + } + } catch (e) { /* Ignore any error */ } var start = Date.now() var backoff = 0; var backoffUntil = start + 60000; @@ -134,6 +143,15 @@ function patch (fs) { }})(fs.rename) fs.renameSync = (function (fs$renameSync) { return function (from, to) { + try { + var stat = fs.statSync(to) + if (!stat) return + if (stat.isDirectory()) { + fs.rmdirSync(to) + } else { + fs.unlinkSync(to) + } + } catch (e) { /* Ignore any error */ } var start = Date.now() var backoff = 0; var backoffUntil = start + 60000; From 17326a05b28f55fa77afcc72b5889c000681e6f0 Mon Sep 17 00:00:00 2001 From: mekwall Date: Fri, 10 Nov 2017 11:54:25 +0100 Subject: [PATCH 18/23] Add test for renaming dirs --- test/windows-rename-polyfill.js | 36 +++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/windows-rename-polyfill.js b/test/windows-rename-polyfill.js index e92189a..5aaef02 100644 --- a/test/windows-rename-polyfill.js +++ b/test/windows-rename-polyfill.js @@ -70,3 +70,39 @@ t.test('cleanup', function (t) { try { fs.rmdirSync(tmpDir) } catch (e) {} t.end() }) + + +var testDir1 = tmpDir + 'test1' +var testDir2 = tmpDir + 'test2' + +function setupDirs() { + try { fs.mkdirSync(testDir1) } catch (e) {} + try { fs.mkdirSync(testDir2) } catch (e) {} +} + +function teardownDirs() { + try { fs.rmdirSync(testDir1) } catch (e) {} + try { fs.rmdirSync(testDir2) } catch (e) {} +} + +t.test('rename async dir to existing', { timeout: 60000 }, function (t) { + t.plan(2) + setupDirs() + gfs.rename(testDir1, testDir2, function (err) { + t.notOk(err) + if (!err) + t.notOk(fs.existsSync(testDir1), 'Source directory still exists') + else + t.ok(!err) + }) + teardownDirs() +}) + +t.test('rename sync dir to existing', { timeout: 60000 }, function (t) { + t.plan(2) + setupDirs(); + t.doesNotThrow(function () { + gfs.renameSync(testDir1, testDir2) + }) + t.notOk(fs.existsSync(testDir1), 'Source directory still exists') +}) From 0e5e89a45a21d07b439fbf4d35bcdff24d6a1aa2 Mon Sep 17 00:00:00 2001 From: John Gozde Date: Wed, 27 Jun 2018 10:39:55 -0600 Subject: [PATCH 19/23] Reduce backoff to 5s, make configurable via env --- polyfills.js | 9 +++++---- test/windows-rename-polyfill.js | 8 ++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/polyfills.js b/polyfills.js index ce50f12..1f08aa6 100644 --- a/polyfills.js +++ b/polyfills.js @@ -4,6 +4,7 @@ var origCwd = process.cwd var cwd = null var platform = process.env.GRACEFUL_FS_PLATFORM || process.platform +var win32MaxBackoff = process.env.GRACEFUL_FS_WIN32_MAX_BACKOFF || 5000; process.cwd = function() { if (!cwd) @@ -91,8 +92,8 @@ function patch (fs) { // // These win32-only overrides try to normalize fs.rename/renameSync // behavior so it's more in line with how it works on Linux and OSX. - // It does this by retrying a failed rename for up to 60 seconds until - // actually failing. + // It does this by retrying a failed rename for up to 5 seconds (or + // value of GRACEFUL_FS_WIN32_MAX_BACKOFF) until actually failing. if (platform === "win32") { fs.rename = (function (fs$rename) { return function (from, to, cb) { try { @@ -106,7 +107,7 @@ function patch (fs) { } catch (e) { /* Ignore any error */ } var start = Date.now() var backoff = 0; - var backoffUntil = start + 60000; + var backoffUntil = start + win32MaxBackoff; fs$rename(from, to, function CB (er) { if (er && (er.code === "EACCES" || er.code === "EPERM") && Date.now() < backoffUntil) { setTimeout(function() { @@ -154,7 +155,7 @@ function patch (fs) { } catch (e) { /* Ignore any error */ } var start = Date.now() var backoff = 0; - var backoffUntil = start + 60000; + var backoffUntil = start + win32MaxBackoff; function tryRename () { try { fs$renameSync(from, to) diff --git a/test/windows-rename-polyfill.js b/test/windows-rename-polyfill.js index 5aaef02..56db682 100644 --- a/test/windows-rename-polyfill.js +++ b/test/windows-rename-polyfill.js @@ -26,7 +26,7 @@ t.test('setup async', function (t) { t.end() }) -t.test('rename async', { timeout: 60000 }, function (t) { +t.test('rename async', { timeout: 5000 }, function (t) { t.plan(testFiles.length * 2) var dest = tmpDir + '/test' testFiles.forEach(function (src) { @@ -48,7 +48,7 @@ t.test('setup sync', function (t) { t.end() }) -t.test('rename sync', { timeout: 60000 }, function (t) { +t.test('rename sync', { timeout: 5000 }, function (t) { t.plan((testFiles.length * 2) + 1) var done = 0; var errors = 0; @@ -85,7 +85,7 @@ function teardownDirs() { try { fs.rmdirSync(testDir2) } catch (e) {} } -t.test('rename async dir to existing', { timeout: 60000 }, function (t) { +t.test('rename async dir to existing', { timeout: 5000 }, function (t) { t.plan(2) setupDirs() gfs.rename(testDir1, testDir2, function (err) { @@ -98,7 +98,7 @@ t.test('rename async dir to existing', { timeout: 60000 }, function (t) { teardownDirs() }) -t.test('rename sync dir to existing', { timeout: 60000 }, function (t) { +t.test('rename sync dir to existing', { timeout: 5000 }, function (t) { t.plan(2) setupDirs(); t.doesNotThrow(function () { From 43a6cb5f2031a9934e624f7486b970f216121d19 Mon Sep 17 00:00:00 2001 From: John Gozde Date: Wed, 27 Jun 2018 10:40:43 -0600 Subject: [PATCH 20/23] Guard cb calls --- polyfills.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polyfills.js b/polyfills.js index 1f08aa6..d920b60 100644 --- a/polyfills.js +++ b/polyfills.js @@ -116,7 +116,7 @@ function patch (fs) { if (erFrom && !erTo) { // If the source no longer exists we // can probably assume it was moved - cb(null) + if (cb) cb(null) } else if ( statFrom && statTo && statFrom.size === statTo.size && @@ -125,7 +125,7 @@ function patch (fs) { // If the source and target have // the same size and ctime, we // can assume it was moved - cb(null) + if (cb) cb(null) } else fs$rename(from, to, CB) }); From f5d0bfd64bb5f503a1430506b3889b1b429d5949 Mon Sep 17 00:00:00 2001 From: John Gozde Date: Wed, 27 Jun 2018 10:41:06 -0600 Subject: [PATCH 21/23] Ensure teardownDirs is called appropriately --- test/windows-rename-polyfill.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/windows-rename-polyfill.js b/test/windows-rename-polyfill.js index 56db682..809c447 100644 --- a/test/windows-rename-polyfill.js +++ b/test/windows-rename-polyfill.js @@ -94,8 +94,8 @@ t.test('rename async dir to existing', { timeout: 5000 }, function (t) { t.notOk(fs.existsSync(testDir1), 'Source directory still exists') else t.ok(!err) + teardownDirs() }) - teardownDirs() }) t.test('rename sync dir to existing', { timeout: 5000 }, function (t) { @@ -105,4 +105,5 @@ t.test('rename sync dir to existing', { timeout: 5000 }, function (t) { gfs.renameSync(testDir1, testDir2) }) t.notOk(fs.existsSync(testDir1), 'Source directory still exists') + teardownDirs() }) From d7aad73e3e18d7a2d85d6b0c27fdad84940db813 Mon Sep 17 00:00:00 2001 From: John Gozde Date: Wed, 27 Jun 2018 10:42:04 -0600 Subject: [PATCH 22/23] Respect ENOTEMPTY errors from rmdir --- polyfills.js | 17 +++++++++++++++-- test/windows-rename-polyfill.js | 21 +++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/polyfills.js b/polyfills.js index d920b60..10789df 100644 --- a/polyfills.js +++ b/polyfills.js @@ -104,7 +104,14 @@ function patch (fs) { } else { fs.unlinkSync(to) } - } catch (e) { /* Ignore any error */ } + } catch (e) { + if (e.code === "ENOTEMPTY") { + // target directory not empty, can't rename + if (cb) cb(e) + return + } + // ignore other errors + } var start = Date.now() var backoff = 0; var backoffUntil = start + win32MaxBackoff; @@ -152,7 +159,13 @@ function patch (fs) { } else { fs.unlinkSync(to) } - } catch (e) { /* Ignore any error */ } + } catch (e) { + if (e.code === "ENOTEMPTY") { + // target directory not empty, can't rename + throw e + } + // ignore other errors + } var start = Date.now() var backoff = 0; var backoffUntil = start + win32MaxBackoff; diff --git a/test/windows-rename-polyfill.js b/test/windows-rename-polyfill.js index 809c447..e5b37cf 100644 --- a/test/windows-rename-polyfill.js +++ b/test/windows-rename-polyfill.js @@ -107,3 +107,24 @@ t.test('rename sync dir to existing', { timeout: 5000 }, function (t) { t.notOk(fs.existsSync(testDir1), 'Source directory still exists') teardownDirs() }) + +t.test('rename async dir to existing, not empty', { timeout: 5000 }, function (t) { + t.plan(3) + setupDirs() + gfs.rename(testDir1, tmpDir, function (err) { + t.ok(err) + t.equal(err.code, "ENOTEMPTY") + t.ok(fs.existsSync(testDir1), 'Source directory was renamed') + teardownDirs() + }) +}) + +t.test('rename sync dir to existing, not empty', { timeout: 5000 }, function (t) { + t.plan(2) + setupDirs(); + t.throws(function () { + gfs.renameSync(testDir1, tmpDir) + }, { code: "ENOTEMPTY" }); + t.ok(fs.existsSync(testDir1), 'Source directory was renamed') + teardownDirs() +}) From 314e721ea379c92175b570f64e464e18d38a5132 Mon Sep 17 00:00:00 2001 From: John Gozde Date: Wed, 27 Jun 2018 10:46:05 -0600 Subject: [PATCH 23/23] Formatting --- polyfills.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/polyfills.js b/polyfills.js index 10789df..9e4f7bd 100644 --- a/polyfills.js +++ b/polyfills.js @@ -4,7 +4,7 @@ var origCwd = process.cwd var cwd = null var platform = process.env.GRACEFUL_FS_PLATFORM || process.platform -var win32MaxBackoff = process.env.GRACEFUL_FS_WIN32_MAX_BACKOFF || 5000; +var win32MaxBackoff = process.env.GRACEFUL_FS_WIN32_MAX_BACKOFF || 5000 process.cwd = function() { if (!cwd) @@ -113,8 +113,8 @@ function patch (fs) { // ignore other errors } var start = Date.now() - var backoff = 0; - var backoffUntil = start + win32MaxBackoff; + var backoff = 0 + var backoffUntil = start + win32MaxBackoff fs$rename(from, to, function CB (er) { if (er && (er.code === "EACCES" || er.code === "EPERM") && Date.now() < backoffUntil) { setTimeout(function() { @@ -135,11 +135,11 @@ function patch (fs) { if (cb) cb(null) } else fs$rename(from, to, CB) - }); + }) }) }, backoff) if (backoff < 250) - backoff += 10; + backoff += 10 } else if (backoff && er && er.code === "ENOENT") { // The source does no longer exist so we // can assume it was moved during one of the tries @@ -167,8 +167,8 @@ function patch (fs) { // ignore other errors } var start = Date.now() - var backoff = 0; - var backoffUntil = start + win32MaxBackoff; + var backoff = 0 + var backoffUntil = start + win32MaxBackoff function tryRename () { try { fs$renameSync(from, to)