Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: add rm method #35494

Closed
wants to merge 9 commits into from
Closed

fs: add rm method #35494

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9845d05
fs: add rm method
iansu Oct 4, 2020
6c26d17
code review changes
iansu Oct 4, 2020
f0f37b1
Update lib/fs.js
bcoe Oct 7, 2020
b1fa541
Update lib/fs.js
bcoe Oct 7, 2020
641bc3b
Remove deprecation warning for recursive rmdir
iansu Oct 7, 2020
adbc955
Update doc/api/fs.md
iansu Oct 7, 2020
7aca540
Use promisify when validating rm options
iansu Oct 7, 2020
384a7e9
Add docs for promise version of rm
iansu Oct 7, 2020
1876684
Fix lint errors
iansu Oct 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2650,6 +2650,16 @@ Type: Documentation-only
The [`crypto.Certificate()` constructor][] is deprecated. Use
[static methods of `crypto.Certificate()`][] instead.

### DEP0147: `Permissive rmdir recursive is deprecated, use rm recursive`
iansu marked this conversation as resolved.
Show resolved Hide resolved
<!-- YAML
changes:
- version: v15.0.0
iansu marked this conversation as resolved.
Show resolved Hide resolved
pr-url: https://github.com/nodejs/node/pull/?
iansu marked this conversation as resolved.
Show resolved Hide resolved
description: Runtime deprecation.
-->

Type: Runtime
iansu marked this conversation as resolved.
Show resolved Hide resolved

[Legacy URL API]: url.md#url_legacy_url_api
[NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
[RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3
Expand Down
5 changes: 5 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,11 @@ added: v14.0.0
Used when a feature that is not available
to the current platform which is running Node.js is used.

<a id="ERR_FS_EISDIR"></a>
### `ERR_FS_EISDIR`

Path is a directory.

<a id="ERR_FS_FILE_TOO_LARGE"></a>
### `ERR_FS_FILE_TOO_LARGE`

Expand Down
45 changes: 45 additions & 0 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3598,6 +3598,51 @@ that represent files will be deleted. The permissive behavior of the
`recursive` option is deprecated, `ENOTDIR` and `ENOENT` will be thrown in
the future.

## `fs.rm(path[, options], callback)`
<!-- YAML
added: v15.0.0
iansu marked this conversation as resolved.
Show resolved Hide resolved
-->

* `path` {string|Buffer|URL}
* `options` {Object}
* `force` Ignore errors
iansu marked this conversation as resolved.
Show resolved Hide resolved
iansu marked this conversation as resolved.
Show resolved Hide resolved
* `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `ENFILE`, `ENOTEMPTY`, or
`EPERM` error is encountered, Node.js will retry the operation with a linear
backoff wait of `retryDelay` ms longer on each try. This option represents
iansu marked this conversation as resolved.
Show resolved Hide resolved
the number of retries. This option is ignored if the `recursive` option is
not `true`. **Default:** `0`.
* `recursive` {boolean} If `true`, perform a recursive removal. In
recursive mode operations are retried on failure. **Default:** `false`.
* `retryDelay` {integer} The amount of time in milliseconds to wait between
retries. This option is ignored if the `recursive` option is not `true`.
**Default:** `100`.
* `callback` {Function}
* `err` {Error}

Asynchronous rm(2). No arguments other than a possible exception are given
iansu marked this conversation as resolved.
Show resolved Hide resolved
to the completion callback.
iansu marked this conversation as resolved.
Show resolved Hide resolved

## `fs.rmSync(path[, options])`
<!-- YAML
added: v15.0.0
iansu marked this conversation as resolved.
Show resolved Hide resolved
-->

* `path` {string|Buffer|URL}
* `options` {Object}
* `force` Ignore errors
* `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `ENFILE`, `ENOTEMPTY`, or
`EPERM` error is encountered, Node.js will retry the operation with a linear
backoff wait of `retryDelay` ms 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 operations are retried on failure. **Default:** `false`.
* `retryDelay` {integer} The amount of time in milliseconds to wait between
retries. This option is ignored if the `recursive` option is not `true`.
**Default:** `100`.

Synchronous rm(2). Returns `undefined`.
iansu marked this conversation as resolved.
Show resolved Hide resolved

## `fs.stat(path[, options], callback)`
<!-- YAML
added: v0.0.2
Expand Down
60 changes: 49 additions & 11 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ const {
validateOffsetLengthRead,
validateOffsetLengthWrite,
validatePath,
validateRmOptions,
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
warnOnNonPortableTemplate
Expand Down Expand Up @@ -847,30 +849,64 @@ function rmdir(path, options, callback) {

callback = makeCallback(callback);
path = pathModule.toNamespacedPath(getValidatedPath(path));
options = validateRmdirOptions(options);

if (options.recursive) {
lazyLoadRimraf();
return rimraf(path, options, callback);
}
if (options && options.recursive) {
options = validateRmOptions(
path,
{ ...options, force: true },
true,
(err, options) => {
if (err) {
return callback(err);
}

const req = new FSReqCallback();
req.oncomplete = callback;
binding.rmdir(path, req);
lazyLoadRimraf();
return rimraf(path, options, callback);
});

} else {
options = validateRmdirOptions(options);
const req = new FSReqCallback();
req.oncomplete = callback;
return binding.rmdir(path, req);
}
}

function rmdirSync(path, options) {
path = getValidatedPath(path);
options = validateRmdirOptions(options);

if (options.recursive) {
if (options && options.recursive) {
options = validateRmOptionsSync(path, { ...options, force: true }, true);
lazyLoadRimraf();
return rimrafSync(pathModule.toNamespacedPath(path), options);
}

options = validateRmdirOptions(options);
const ctx = { path };
binding.rmdir(pathModule.toNamespacedPath(path), undefined, ctx);
handleErrorFromBinding(ctx);
return handleErrorFromBinding(ctx);
}

function rm(path, options, callback) {
if (typeof options === 'function') {
callback = options;
options = undefined;
}

validateRmOptions(path, options, false, (err, options) => {
if (err) {
return callback(err);
}
lazyLoadRimraf();
return rimraf(path, options, callback);
bcoe marked this conversation as resolved.
Show resolved Hide resolved
});
}

function rmSync(path, options) {
options = validateRmOptionsSync(path, options, false);

lazyLoadRimraf();
return rimrafSync(path, options);
bcoe marked this conversation as resolved.
Show resolved Hide resolved
}

function fdatasync(fd, callback) {
Expand Down Expand Up @@ -2022,6 +2058,8 @@ module.exports = fs = {
realpathSync,
rename,
renameSync,
rm,
rmSync,
rmdir,
rmdirSync,
stat,
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,7 @@ E('ERR_FEATURE_UNAVAILABLE_ON_PLATFORM',
'The feature %s is unavailable on the current platform' +
', which is being used to run Node.js',
TypeError);
E('ERR_FS_EISDIR', 'is a directory', SystemError);
iansu marked this conversation as resolved.
Show resolved Hide resolved
E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than 2 GB', RangeError);
E('ERR_FS_INVALID_SYMLINK_TYPE',
'Symlink type must be one of "dir", "file", or "junction". Received "%s"',
Expand Down
9 changes: 9 additions & 0 deletions lib/internal/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const {
validateBufferArray,
validateOffsetLengthRead,
validateOffsetLengthWrite,
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
warnOnNonPortableTemplate
Expand Down Expand Up @@ -417,6 +418,13 @@ async function ftruncate(handle, len = 0) {
return binding.ftruncate(handle.fd, len, kUsePromises);
}

async function rm(path, options) {
path = pathModule.toNamespacedPath(getValidatedPath(path));
options = validateRmOptionsSync(path, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use the async validation here, otherwise we're going to create a bottleneck when performing many rm operations, you can do something like this:

options = await new Promise((resolve, reject) => {
  validateRmOptionsSync(path, options, false, (err, options) => {
     if (err) return reject(err);
     else return resolve(options);
  });
})
return rimrafPromises(path, options);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this still creating the same bottleneck tho, it's just deferring the result? a new Promise executor runs synchronously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb my bad, I meant:

options = await new Promise((resolve, reject) => {
  validateRmOptions(path, options, false, (err, options) => {
     if (err) return reject(err);
     else return resolve(options);
  });
})
return rimrafPromises(path, options);

i.e., not using the sync version of the validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented this a few days ago and I did catch that mistake in your suggested change @bcoe. I'm not sure if that changes the point @ljharb raised.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't been updated yet, but yes, @bcoe's updated suggestion would address my point. I'd also say that it's worth adding a custom promisify implementation to validateRmOptions, and then use await promisify(validateRmOptions)(path, options, false) instead?


return rimrafPromises(path, options);
}

async function rmdir(path, options) {
path = pathModule.toNamespacedPath(getValidatedPath(path));
options = validateRmdirOptions(options);
Expand Down Expand Up @@ -635,6 +643,7 @@ module.exports = {
opendir: promisify(opendir),
rename,
truncate,
rm,
rmdir,
mkdir,
readdir,
Expand Down
124 changes: 113 additions & 11 deletions lib/internal/fs/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const {
const { Buffer } = require('buffer');
const {
codes: {
ERR_FS_EISDIR,
ERR_FS_INVALID_SYMLINK_TYPE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
Expand Down Expand Up @@ -650,28 +651,127 @@ function warnOnNonPortableTemplate(template) {
}
}

const defaultRmOptions = {
recursive: false,
force: false,
retryDelay: 100,
maxRetries: 0
};

const defaultRmdirOptions = {
retryDelay: 100,
maxRetries: 0,
recursive: false,
};

const validateRmdirOptions = hideStackFrames((options) => {
if (options === undefined)
return defaultRmdirOptions;
if (options === null || typeof options !== 'object')
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
let permissiveRmdirWarned = false;

options = { ...defaultRmdirOptions, ...options };
function emitPermissiveRmdirWarning() {
if (!permissiveRmdirWarned) {
process.emitWarning(
'Permissive rmdir recursive is deprecated, use rm recursive instead',
'DEP0147'
);
iansu marked this conversation as resolved.
Show resolved Hide resolved
permissiveRmdirWarned = true;
}
}

if (typeof options.recursive !== 'boolean')
throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', options.recursive);
const validateRmOptions = (path, options, warn, callback) => {
try {
options = validateRmdirOptions(options, defaultRmOptions);
} catch (err) {
return callback(err);
}

validateInt32(options.retryDelay, 'retryDelay', 0);
validateUint32(options.maxRetries, 'maxRetries');
if (typeof options.force !== 'boolean')
return callback(
new ERR_INVALID_ARG_TYPE('force', 'boolean', options.force)
);

lazyLoadFs().stat(path, (err, stats) => {
if (err && err.code === 'ENOENT') {
if (options.force) {
if (warn) {
emitPermissiveRmdirWarning();
}
return callback(null, options);
}
return callback(err, options);
}

if (err) {
return callback(err);
}

if (warn && !stats.isDirectory()) {
emitPermissiveRmdirWarning();
}

if (stats.isDirectory() && !options.recursive) {
return callback(new ERR_FS_EISDIR({
code: 'EISDIR',
message: 'is a directory',
path,
syscall: 'rm',
errno: -21
}));
}
return callback(null, options);
});
};

const validateRmOptionsSync = (path, options, warn) => {
options = validateRmdirOptions(options, defaultRmOptions);

if (typeof options.force !== 'boolean')
throw new ERR_INVALID_ARG_TYPE('force', 'boolean', options.force);

try {
const stats = lazyLoadFs().statSync(path);

if (warn && !stats.isDirectory()) {
emitPermissiveRmdirWarning();
}

if (stats.isDirectory() && !options.recursive) {
throw new ERR_FS_EISDIR({
code: 'EISDIR',
message: 'is a directory',
path,
syscall: 'rm',
errno: -21
});
}
} catch (err) {
if (err.code !== 'ENOENT') {
throw err;
} else if (err.code === 'ENOENT' && !options.force) {
throw err;
} else if (warn && err.code === 'ENOENT') {
emitPermissiveRmdirWarning();
}
}

return options;
});
};

const validateRmdirOptions = hideStackFrames(
iansu marked this conversation as resolved.
Show resolved Hide resolved
(options, defaults = defaultRmdirOptions) => {
if (options === undefined)
return defaults;
if (options === null || typeof options !== 'object')
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);

options = { ...defaults, ...options };

if (typeof options.recursive !== 'boolean')
throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', options.recursive);

validateInt32(options.retryDelay, 'retryDelay', 0);
validateUint32(options.maxRetries, 'maxRetries');

return options;
});

const getValidMode = hideStackFrames((mode, type) => {
let min = kMinimumAccessMode;
Expand Down Expand Up @@ -741,6 +841,8 @@ module.exports = {
validateOffsetLengthRead,
validateOffsetLengthWrite,
validatePath,
validateRmOptions,
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
warnOnNonPortableTemplate
Expand Down
8 changes: 4 additions & 4 deletions test/common/tmpdir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ const fs = require('fs');
const path = require('path');
const { isMainThread } = require('worker_threads');

function rimrafSync(pathname) {
fs.rmdirSync(pathname, { maxRetries: 3, recursive: true });
function rmSync(pathname) {
fs.rmSync(pathname, { maxRetries: 3, recursive: true, force: true });
}

const testRoot = process.env.NODE_TEST_DIR ?
Expand All @@ -20,7 +20,7 @@ const tmpPath = path.join(testRoot, tmpdirName);

let firstRefresh = true;
function refresh() {
rimrafSync(this.path);
rmSync(this.path);
fs.mkdirSync(this.path);

if (firstRefresh) {
Expand All @@ -37,7 +37,7 @@ function onexit() {
process.chdir(testRoot);

try {
rimrafSync(tmpPath);
rmSync(tmpPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love it 😄

} catch (e) {
console.error('Can\'t clean tmpdir:', tmpPath);

Expand Down
Loading