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: nodejs#30580 PR-URL: nodejs#30644 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
cjihrig · Nov 27, 2019 · 7e85f06 · 7e85f06
1 parent 4059055
commit 7e85f06
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 13 deletions.
diff --git 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
@@ -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
@@ -567,7 +567,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
@@ -156,7 +156,7 @@ function removeAsync(dir) {
 {
   const defaults = {
     emfileWait: 1000,
-    maxRetries: 3,
+    maxRetries: 0,
     recursive: false
   };
   const modified = {