From 7a321989ac96610e18724b051015116c5a380061 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 25 Nov 2019 16:13:27 -0500 Subject: [PATCH] fs: remove rimraf's emfileWait option This commit removes the emfileWait option. EMFILE errors are now handled the same as any other retriable error. 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 | 35 +++++++++++------------- lib/internal/fs/rimraf.js | 8 ++---- lib/internal/fs/utils.js | 3 -- test/parallel/test-fs-rmdir-recursive.js | 11 -------- 4 files changed, 18 insertions(+), 39 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index c2862900a30386..887bb9e54a6b96 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -3223,7 +3223,8 @@ changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/30644 description: The `maxBusyTries` option is renamed to `maxRetries`, and its - default is 0. + default is 0. The `emfileWait` option has been removed, and + `EMFILE` errors use the same retry logic as other errors. - version: v12.10.0 pr-url: https://github.com/nodejs/node/pull/29168 description: The `recursive`, `maxBusyTries`, and `emfileWait` options are @@ -3246,14 +3247,11 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} - * `emfileWait` {integer} If an `EMFILE` error is encountered, Node.js will - retry the operation with a linear backoff of 1ms longer on each try until the - timeout duration passes this limit. This option is ignored if the `recursive` - option is not `true`. **Default:** `1000`. - * `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:** `0`. + * `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `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:** `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`. @@ -3273,7 +3271,8 @@ changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/30644 description: The `maxBusyTries` option is renamed to `maxRetries`, and its - default is 0. + default is 0. The `emfileWait` option has been removed, and + `EMFILE` errors use the same retry logic as other errors. - version: v12.10.0 pr-url: https://github.com/nodejs/node/pull/29168 description: The `recursive`, `maxBusyTries`, and `emfileWait` options are @@ -5005,7 +5004,8 @@ changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/30644 description: The `maxBusyTries` option is renamed to `maxRetries`, and its - default is 0. + default is 0. The `emfileWait` option has been removed, and + `EMFILE` errors use the same retry logic as other errors. - version: v12.10.0 pr-url: https://github.com/nodejs/node/pull/29168 description: The `recursive`, `maxBusyTries`, and `emfileWait` options are @@ -5016,14 +5016,11 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} - * `emfileWait` {integer} If an `EMFILE` error is encountered, Node.js will - retry the operation with a linear backoff of 1ms longer on each try until the - timeout duration passes this limit. This option is ignored if the `recursive` - option is not `true`. **Default:** `1000`. - * `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:** `0`. + * `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `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:** `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 17e4fe5c9d6d17..9d5fcad49c9fca 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -22,26 +22,22 @@ const { const { join } = require('path'); const { setTimeout } = require('timers'); const notEmptyErrorCodes = new Set(['ENOTEMPTY', 'EEXIST', 'EPERM']); +const retryErrorCodes = new Set(['EBUSY', 'EMFILE', 'ENOTEMPTY', 'EPERM']); const isWindows = process.platform === 'win32'; const epermHandler = isWindows ? fixWinEPERM : _rmdir; const epermHandlerSync = isWindows ? fixWinEPERMSync : _rmdirSync; function rimraf(path, options, callback) { - let timeout = 0; // For EMFILE handling. let retries = 0; _rimraf(path, options, function CB(err) { if (err) { - if ((err.code === 'EBUSY' || err.code === 'ENOTEMPTY' || - err.code === 'EPERM') && retries < options.maxRetries) { + if (retryErrorCodes.has(err.code) && retries < options.maxRetries) { retries++; return setTimeout(_rimraf, retries * 100, path, options, CB); } - if (err.code === 'EMFILE' && timeout < options.emfileWait) - return setTimeout(_rimraf, timeout++, path, options, CB); - // The file is already gone. if (err.code === 'ENOENT') err = null; diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 527a14e654cda1..0e77ddc267c564 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -24,7 +24,6 @@ const { const { once } = require('internal/util'); const { toPathIfFileURL } = require('internal/url'); const { - validateInt32, validateUint32 } = require('internal/validators'); const pathModule = require('path'); @@ -563,7 +562,6 @@ function warnOnNonPortableTemplate(template) { } const defaultRmdirOptions = { - emfileWait: 1000, maxRetries: 0, recursive: false, }; @@ -579,7 +577,6 @@ const validateRmdirOptions = hideStackFrames((options) => { if (typeof options.recursive !== 'boolean') throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', options.recursive); - validateInt32(options.emfileWait, 'emfileWait', 0); validateUint32(options.maxRetries, 'maxRetries'); return options; diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 471eb69cb2ab8d..53f863872d80ea 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -155,12 +155,10 @@ function removeAsync(dir) { // Test input validation. { const defaults = { - emfileWait: 1000, maxRetries: 0, recursive: false }; const modified = { - emfileWait: 953, maxRetries: 5, recursive: true }; @@ -171,7 +169,6 @@ function removeAsync(dir) { assert.deepStrictEqual(validateRmdirOptions({ maxRetries: 99 }), { - emfileWait: 1000, maxRetries: 99, recursive: false }); @@ -196,14 +193,6 @@ function removeAsync(dir) { }); }); - common.expectsError(() => { - validateRmdirOptions({ emfileWait: -1 }); - }, { - code: 'ERR_OUT_OF_RANGE', - type: RangeError, - message: /^The value of "emfileWait" is out of range\./ - }); - common.expectsError(() => { validateRmdirOptions({ maxRetries: -1 }); }, {