From 15c339d4b36ecfe019c81c1428d7dfa41c36b69b Mon Sep 17 00:00:00 2001 From: pgonzal Date: Fri, 2 Nov 2018 15:32:06 -0700 Subject: [PATCH 1/4] Fix removeSync() to measure its retry interval in milliseconds instead of CPU cycles --- lib/remove/rimraf.js | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/lib/remove/rimraf.js b/lib/remove/rimraf.js index c89c1c01..20cbcd0d 100644 --- a/lib/remove/rimraf.js +++ b/lib/remove/rimraf.js @@ -285,29 +285,37 @@ function rmdirSync (p, options, originalEr) { } } +function getMilliseconds(lastTime) { + let [seconds, nanoseconds] = process.hrtime(lastTime); + // limit the non-fractional portion to be less than 1^32 + return (seconds & 0xffffffff) * 1000 + nanoseconds / 1000000; +} + function rmkidsSync (p, options) { assert(p) assert(options) options.readdirSync(p).forEach(f => rimrafSync(path.join(p, f), options)) - // We only end up here once we got ENOTEMPTY at least once, and - // at this point, we are guaranteed to have removed all the kids. - // So, we know that it won't be ENOENT or ENOTDIR or anything else. - // try really hard to delete stuff on windows, because it has a - // PROFOUNDLY annoying habit of not closing handles promptly when - // files are deleted, resulting in spurious ENOTEMPTY errors. - const retries = isWindows ? 100 : 1 - let i = 0 - do { - let threw = true - try { - const ret = options.rmdirSync(p, options) - threw = false - return ret - } finally { - if (++i < retries && threw) continue // eslint-disable-line - } - } while (true) + if (isWindows) { + // We only end up here once we got ENOTEMPTY at least once, and + // at this point, we are guaranteed to have removed all the kids. + // So, we know that it won't be ENOENT or ENOTDIR or anything else. + // try really hard to delete stuff on windows, because it has a + // PROFOUNDLY annoying habit of not closing handles promptly when + // files are deleted, resulting in spurious ENOTEMPTY errors. + const startTime = getMilliseconds(); + do { + try { + const ret = options.rmdirSync(p, options) + return ret + } catch { } + + // This comparison uses modular arithmetic since getMilliseconds() can roll over + } while (((getMilliseconds() - startTime) & 0xffffffff) < 500) // give up after 500ms + } else { + const ret = options.rmdirSync(p, options) + return ret + } } module.exports = rimraf From 04c6e820fe26e7563b7bd0bef4f585cc9ff7455d Mon Sep 17 00:00:00 2001 From: pgonzal Date: Fri, 2 Nov 2018 16:09:38 -0700 Subject: [PATCH 2/4] PR feedback --- lib/remove/rimraf.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/remove/rimraf.js b/lib/remove/rimraf.js index 20cbcd0d..f5ca01d7 100644 --- a/lib/remove/rimraf.js +++ b/lib/remove/rimraf.js @@ -285,12 +285,6 @@ function rmdirSync (p, options, originalEr) { } } -function getMilliseconds(lastTime) { - let [seconds, nanoseconds] = process.hrtime(lastTime); - // limit the non-fractional portion to be less than 1^32 - return (seconds & 0xffffffff) * 1000 + nanoseconds / 1000000; -} - function rmkidsSync (p, options) { assert(p) assert(options) @@ -303,15 +297,16 @@ function rmkidsSync (p, options) { // try really hard to delete stuff on windows, because it has a // PROFOUNDLY annoying habit of not closing handles promptly when // files are deleted, resulting in spurious ENOTEMPTY errors. - const startTime = getMilliseconds(); + const startTime = Date.now(); do { + console.log(Date.now() - startTime); + try { const ret = options.rmdirSync(p, options) return ret } catch { } - // This comparison uses modular arithmetic since getMilliseconds() can roll over - } while (((getMilliseconds() - startTime) & 0xffffffff) < 500) // give up after 500ms + } while (Date.now() - startTime < 500) // give up after 500ms } else { const ret = options.rmdirSync(p, options) return ret From 2275b7f9b1598580a7a71349ccad5f2bf751d07e Mon Sep 17 00:00:00 2001 From: pgonzal Date: Fri, 2 Nov 2018 16:11:10 -0700 Subject: [PATCH 3/4] Fix AppVeyor build error --- lib/remove/rimraf.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/remove/rimraf.js b/lib/remove/rimraf.js index f5ca01d7..d75ad0a1 100644 --- a/lib/remove/rimraf.js +++ b/lib/remove/rimraf.js @@ -304,7 +304,7 @@ function rmkidsSync (p, options) { try { const ret = options.rmdirSync(p, options) return ret - } catch { } + } catch (er) { } } while (Date.now() - startTime < 500) // give up after 500ms } else { From ccd7d3493a79a13c7ece20d42adbc4292ba61ee0 Mon Sep 17 00:00:00 2001 From: pgonzal Date: Fri, 2 Nov 2018 16:18:07 -0700 Subject: [PATCH 4/4] ``` standard: Use JavaScript Standard Style (https://standardjs.com) standard: Run `standard --fix` to automatically fix some problems. /home/travis/build/jprichardson/node-fs-extra/lib/remove/rimraf.js:300:33: Extra semicolon. /home/travis/build/jprichardson/node-fs-extra/lib/remove/rimraf.js:302:42: Extra semicolon. /home/travis/build/jprichardson/node-fs-extra/lib/remove/rimraf.js:309:5: Block must not be padded by blank lines ``` --- lib/remove/rimraf.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/remove/rimraf.js b/lib/remove/rimraf.js index d75ad0a1..f287e4e1 100644 --- a/lib/remove/rimraf.js +++ b/lib/remove/rimraf.js @@ -297,15 +297,12 @@ function rmkidsSync (p, options) { // try really hard to delete stuff on windows, because it has a // PROFOUNDLY annoying habit of not closing handles promptly when // files are deleted, resulting in spurious ENOTEMPTY errors. - const startTime = Date.now(); + const startTime = Date.now() do { - console.log(Date.now() - startTime); - try { const ret = options.rmdirSync(p, options) return ret } catch (er) { } - } while (Date.now() - startTime < 500) // give up after 500ms } else { const ret = options.rmdirSync(p, options)