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: #30580 PR-URL: #30644 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Nov 30, 2019 · 612a3a2 · 612a3a2
1 parent fa1f87b
commit 612a3a2
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
@@ -563,7 +563,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 = {