From ccc228b4384359b320f5721e20d66eb5e9b34bd3 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 25 Nov 2019 15:31:44 -0500 Subject: [PATCH] fs: make rimraf default to 0 retries This commit makes retries an opt-in feature by defaulting to no automatic retries. This will be particularly important once synchronous operations can sleep between attempts. Refs: https://github.com/nodejs/node/issues/30580 PR-URL: https://github.com/nodejs/node/pull/30644 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- doc/api/fs.md | 17 ++++++++++++----- lib/internal/fs/rimraf.js | 11 +++++------ lib/internal/fs/utils.js | 2 +- test/parallel/test-fs-rmdir-recursive.js | 2 +- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 24864256118523..c2862900a30386 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -3222,7 +3222,8 @@ added: v0.0.2 changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/30644 - description: The `maxBusyTries` option is renamed to `maxRetries`. + description: The `maxBusyTries` option is renamed to `maxRetries`, and its + default is 0. - version: v12.10.0 pr-url: https://github.com/nodejs/node/pull/29168 description: The `recursive`, `maxBusyTries`, and `emfileWait` options are @@ -3252,7 +3253,7 @@ changes: * `maxRetries` {integer} If an `EBUSY`, `ENOTEMPTY`, or `EPERM` error is encountered, Node.js will retry the operation with a linear backoff wait of 100ms longer on each try. This option represents the number of retries. This - option is ignored if the `recursive` option is not `true`. **Default:** `3`. + option is ignored if the `recursive` option is not `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In recursive mode, errors are not reported if `path` does not exist, and operations are retried on failure. **Default:** `false`. @@ -3271,7 +3272,8 @@ added: v0.1.21 changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/30644 - description: The `maxBusyTries` option is renamed to `maxRetries`. + description: The `maxBusyTries` option is renamed to `maxRetries`, and its + default is 0. - version: v12.10.0 pr-url: https://github.com/nodejs/node/pull/29168 description: The `recursive`, `maxBusyTries`, and `emfileWait` options are @@ -3286,6 +3288,10 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} + * `maxRetries` {integer} If an `EBUSY`, `ENOTEMPTY`, or `EPERM` error is + encountered, Node.js will retry the operation. This option represents the + number of retries. This option is ignored if the `recursive` option is not + `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In recursive mode, errors are not reported if `path` does not exist, and operations are retried on failure. **Default:** `false`. @@ -4998,7 +5004,8 @@ added: v10.0.0 changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/30644 - description: The `maxBusyTries` option is renamed to `maxRetries`. + description: The `maxBusyTries` option is renamed to `maxRetries`, and its + default is 0. - version: v12.10.0 pr-url: https://github.com/nodejs/node/pull/29168 description: The `recursive`, `maxBusyTries`, and `emfileWait` options are @@ -5016,7 +5023,7 @@ changes: * `maxRetries` {integer} If an `EBUSY`, `ENOTEMPTY`, or `EPERM` error is encountered, Node.js will retry the operation with a linear backoff wait of 100ms longer on each try. This option represents the number of retries. This - option is ignored if the `recursive` option is not `true`. **Default:** `3`. + option is ignored if the `recursive` option is not `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In recursive mode, errors are not reported if `path` does not exist, and operations are retried on failure. **Default:** `false`. diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index c9f3b3e01d963f..17e4fe5c9d6d17 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -25,19 +25,18 @@ const notEmptyErrorCodes = new Set(['ENOTEMPTY', 'EEXIST', 'EPERM']); const isWindows = process.platform === 'win32'; const epermHandler = isWindows ? fixWinEPERM : _rmdir; const epermHandlerSync = isWindows ? fixWinEPERMSync : _rmdirSync; -const numRetries = isWindows ? 100 : 1; function rimraf(path, options, callback) { let timeout = 0; // For EMFILE handling. - let busyTries = 0; + let retries = 0; _rimraf(path, options, function CB(err) { if (err) { if ((err.code === 'EBUSY' || err.code === 'ENOTEMPTY' || - err.code === 'EPERM') && busyTries < options.maxRetries) { - busyTries++; - return setTimeout(_rimraf, busyTries * 100, path, options, CB); + err.code === 'EPERM') && retries < options.maxRetries) { + retries++; + return setTimeout(_rimraf, retries * 100, path, options, CB); } if (err.code === 'EMFILE' && timeout < options.emfileWait) @@ -211,7 +210,7 @@ function _rmdirSync(path, options, originalErr) { rimrafSync(join(path, child), options); }); - for (let i = 0; i < numRetries; i++) { + for (let i = 0; i < options.maxRetries + 1; i++) { try { return rmdirSync(path, options); } catch {} // Ignore errors. diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 63bcfcc2ad3a6e..527a14e654cda1 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -564,7 +564,7 @@ function warnOnNonPortableTemplate(template) { const defaultRmdirOptions = { emfileWait: 1000, - maxRetries: 3, + maxRetries: 0, recursive: false, }; diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 6bc7fc6e5671ab..471eb69cb2ab8d 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -156,7 +156,7 @@ function removeAsync(dir) { { const defaults = { emfileWait: 1000, - maxRetries: 3, + maxRetries: 0, recursive: false }; const modified = {