Skip to content

Commit

Permalink
fs: move rmSync implementation to c++
Browse files Browse the repository at this point in the history
  • Loading branch information
@anonrig
anonrig committed Jun 28, 2024
1 parent 895fcb0 commit 3d2975d
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 177 deletions.
13 changes: 4 additions & 9 deletions lib/fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ let promises = null;
let ReadStream;
let WriteStream;
let rimraf;
let rimrafSync;
let kResistStopPropagation;

// These have to be separate because of how graceful-fs happens to do it's
Expand Down Expand Up @@ -1126,7 +1125,7 @@ function lazyLoadCp() {

function lazyLoadRimraf() {
if (rimraf === undefined)
({ rimraf, rimrafSync } = require('internal/fs/rimraf'));
({ rimraf } = require('internal/fs/rimraf'));
}

/**
Expand Down Expand Up @@ -1194,8 +1193,7 @@ function rmdirSync(path, options) {
emitRecursiveRmdirWarning();
options = validateRmOptionsSync(path, { ...options, force: false }, true);
if (options !== false) {
lazyLoadRimraf();
return rimrafSync(path, options);
return binding.rmSync(path, options.maxRetries, options.recursive, options.retryDelay);
}
} else {
validateRmdirOptions(options);
Expand Down Expand Up @@ -1246,11 +1244,8 @@ function rm(path, options, callback) {
* @returns {void}
*/
function rmSync(path, options) {
lazyLoadRimraf();
return rimrafSync(
getValidatedPath(path),
validateRmOptionsSync(path, options, false),
);
const opts = validateRmOptionsSync(path, options, false);
return binding.rmSync(getValidatedPath(path), opts.maxRetries, opts.recursive, opts.retryDelay);
}

/**
Expand Down
144 changes: 1 addition & 143 deletions lib/internal/fs/rimraf.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,19 @@ const { Buffer } = require('buffer');
const fs = require('fs');
const {
chmod,
chmodSync,
lstat,
lstatSync,
readdir,
readdirSync,
rmdir,
rmdirSync,
stat,
statSync,
unlink,
unlinkSync,
} = fs;
const { sep } = require('path');
const { setTimeout } = require('timers');
const { sleep } = require('internal/util');
const notEmptyErrorCodes = new SafeSet(['ENOTEMPTY', 'EEXIST', 'EPERM']);
const retryErrorCodes = new SafeSet(
['EBUSY', 'EMFILE', 'ENFILE', 'ENOTEMPTY', 'EPERM']);
const isWindows = process.platform === 'win32';
const epermHandler = isWindows ? fixWinEPERM : _rmdir;
const epermHandlerSync = isWindows ? fixWinEPERMSync : _rmdirSync;
const readdirEncoding = 'buffer';
const separator = Buffer.from(sep);

Expand Down Expand Up @@ -173,138 +165,4 @@ function rimrafPromises(path, options) {
}


function rimrafSync(path, options) {
let stats;

try {
stats = lstatSync(path);
} catch (err) {
if (err.code === 'ENOENT')
return;

// Windows can EPERM on stat.
if (isWindows && err.code === 'EPERM')
fixWinEPERMSync(path, options, err);
}

try {
// SunOS lets the root user unlink directories.
if (stats?.isDirectory())
_rmdirSync(path, options, null);
else
_unlinkSync(path, options);
} catch (err) {
if (err.code === 'ENOENT')
return;
if (err.code === 'EPERM')
return epermHandlerSync(path, options, err);
if (err.code !== 'EISDIR')
throw err;

_rmdirSync(path, options, err);
}
}


function _unlinkSync(path, options) {
const tries = options.maxRetries + 1;

for (let i = 1; i <= tries; i++) {
try {
return unlinkSync(path);
} catch (err) {
// Only sleep if this is not the last try, and the delay is greater
// than zero, and an error was encountered that warrants a retry.
if (retryErrorCodes.has(err.code) &&
i < tries &&
options.retryDelay > 0) {
sleep(i * options.retryDelay);
} else if (err.code === 'ENOENT') {
// The file is already gone.
return;
} else if (i === tries) {
throw err;
}
}
}
}


function _rmdirSync(path, options, originalErr) {
try {
rmdirSync(path);
} catch (err) {
if (err.code === 'ENOENT')
return;
if (err.code === 'ENOTDIR') {
throw originalErr || err;
}

if (notEmptyErrorCodes.has(err.code)) {
// Removing failed. Try removing all children and then retrying the
// original removal. Windows has a habit of not closing handles promptly
// when files are deleted, resulting in spurious ENOTEMPTY failures. Work
// around that issue by retrying on Windows.
const pathBuf = Buffer.from(path);

ArrayPrototypeForEach(readdirSync(pathBuf, readdirEncoding), (child) => {
const childPath = Buffer.concat([pathBuf, separator, child]);

rimrafSync(childPath, options);
});

const tries = options.maxRetries + 1;

for (let i = 1; i <= tries; i++) {
try {
return fs.rmdirSync(path);
} catch (err) {
// Only sleep if this is not the last try, and the delay is greater
// than zero, and an error was encountered that warrants a retry.
if (retryErrorCodes.has(err.code) &&
i < tries &&
options.retryDelay > 0) {
sleep(i * options.retryDelay);
} else if (err.code === 'ENOENT') {
// The file is already gone.
return;
} else if (i === tries) {
throw err;
}
}
}
}

throw originalErr || err;
}
}


function fixWinEPERMSync(path, options, originalErr) {
try {
chmodSync(path, 0o666);
} catch (err) {
if (err.code === 'ENOENT')
return;

throw originalErr;
}

let stats;

try {
stats = statSync(path, { throwIfNoEntry: false });
} catch {
throw originalErr;
}

if (stats === undefined) return;

if (stats.isDirectory())
_rmdirSync(path, options, originalErr);
else
_unlinkSync(path, options);
}


module.exports = { rimraf, rimrafPromises, rimrafSync };
module.exports = { rimraf, rimrafPromises };
1 change: 1 addition & 0 deletions src/node_errors.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
V(ERR_DLOPEN_FAILED, Error) \
V(ERR_ENCODING_INVALID_ENCODED_DATA, TypeError) \
V(ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE, Error) \
V(ERR_FS_EISDIR, Error) \
V(ERR_ILLEGAL_CONSTRUCTOR, Error) \
V(ERR_INVALID_ADDRESS, Error) \
V(ERR_INVALID_ARG_VALUE, TypeError) \
Expand Down
86 changes: 86 additions & 0 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
# include <io.h>
#endif

#include <unistd.h>

namespace node {

namespace fs {
Expand Down Expand Up @@ -1605,6 +1607,88 @@ static void RMDir(const FunctionCallbackInfo<Value>& args) {
}
}

static void RmSync(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

CHECK_EQ(args.Length(), 4); // path, maxRetries, recursive, retryDelay

BufferValue path(isolate, args[0]);
CHECK_NOT_NULL(*path);
ToNamespacedPath(env, &path);
THROW_IF_INSUFFICIENT_PERMISSIONS(
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
auto file_path = std::filesystem::path(path.ToStringView());
std::error_code error;
auto file_status = std::filesystem::status(file_path, error);

if (file_status.type() == std::filesystem::file_type::not_found) {
return;
}

int maxRetries = args[1].As<Int32>()->Value();
int recursive = args[2]->IsTrue();
int retryDelay = args[3].As<Int32>()->Value();

// File is a directory and recursive is false
if (file_status.type() == std::filesystem::file_type::directory &&
!recursive) {
return THROW_ERR_FS_EISDIR(
isolate, "Path is a directory: %s", file_path.c_str());
}

// Allowed errors are:
// - EBUSY: std::errc::device_or_resource_busy
// - EMFILE: std::errc::too_many_files_open
// - ENFILE: std::errc::too_many_files_open_in_system
// - ENOTEMPTY: std::errc::directory_not_empty
// - EPERM: std::errc::operation_not_permitted
auto can_omit_error = [](std::error_code error) -> bool {
return (error == std::errc::device_or_resource_busy ||
error == std::errc::too_many_files_open ||
error == std::errc::too_many_files_open_in_system ||
error == std::errc::directory_not_empty ||
error == std::errc::operation_not_permitted);
};

while (maxRetries >= 0) {
if (recursive) {
std::filesystem::remove_all(file_path, error);
} else {
std::filesystem::remove(file_path, error);
}

if (!error || error == std::errc::no_such_file_or_directory) {
return;
} else if (!can_omit_error(error)) {
break;
}

if (retryDelay != 0) {
sleep(retryDelay / 1000);
}
maxRetries--;
}

if (error == std::errc::operation_not_permitted) {
std::string message = "Operation not permitted: " + file_path.string();
return env->ThrowErrnoException(
EPERM, "rm", message.c_str(), file_path.c_str());
} else if (error == std::errc::directory_not_empty) {
std::string message = "Directory not empty: " + file_path.string();
return env->ThrowErrnoException(
EACCES, "rm", message.c_str(), file_path.c_str());
} else if (error == std::errc::not_a_directory) {
std::string message = "Not a directory: " + file_path.string();
return env->ThrowErrnoException(
ENOTDIR, "rm", message.c_str(), file_path.c_str());
}

std::string message = "Unknown error: " + error.message();
return env->ThrowErrnoException(
UV_UNKNOWN, "rm", message.c_str(), file_path.c_str());
}

int MKDirpSync(uv_loop_t* loop,
uv_fs_t* req,
const std::string& path,
Expand Down Expand Up @@ -3323,6 +3407,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
SetMethod(isolate, target, "rename", Rename);
SetMethod(isolate, target, "ftruncate", FTruncate);
SetMethod(isolate, target, "rmdir", RMDir);
SetMethod(isolate, target, "rmSync", RmSync);
SetMethod(isolate, target, "mkdir", MKDir);
SetMethod(isolate, target, "readdir", ReadDir);
SetFastMethod(isolate,
Expand Down Expand Up @@ -3447,6 +3532,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(Rename);
registry->Register(FTruncate);
registry->Register(RMDir);
registry->Register(RmSync);
registry->Register(MKDir);
registry->Register(ReadDir);
registry->Register(InternalModuleStat);
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-fs-rm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ if (isGitPresent) {
name: 'Error',
});
} catch (err) {
console.log(err);
// Only fail the test if the folder was not deleted.
// as in some cases rmSync successfully deletes read-only folders.
if (fs.existsSync(root)) {
Expand Down
25 changes: 0 additions & 25 deletions test/parallel/test-fs-rmdir-recursive.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,28 +217,3 @@ function removeAsync(dir) {
message: /^The value of "options\.maxRetries" is out of range\./
});
}

// It should not pass recursive option to rmdirSync, when called from
// rimraf (see: #35566)
{
// Make a non-empty directory:
const original = fs.rmdirSync;
const dir = `${nextDirPath()}/foo/bar`;
fs.mkdirSync(dir, { recursive: true });
fs.writeFileSync(`${dir}/foo.txt`, 'hello world', 'utf8');

// When called the second time from rimraf, the recursive option should
// not be set for rmdirSync:
let callCount = 0;
let rmdirSyncOptionsFromRimraf;
fs.rmdirSync = (path, options) => {
if (callCount > 0) {
rmdirSyncOptionsFromRimraf = { ...options };
}
callCount++;
return original(path, options);
};
fs.rmdirSync(dir, { recursive: true });
fs.rmdirSync = original;
assert.strictEqual(rmdirSyncOptionsFromRimraf.recursive, undefined);
}
3 changes: 3 additions & 0 deletions typings/internalBinding/fs.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ declare namespace InternalFSBinding {
function rmdir(path: string): void;
function rmdir(path: string, usePromises: typeof kUsePromises): Promise<void>;

function rmSync(path: StringOrBuffer, maxRetries: number, recursive: boolean, retryDelay: number): void;

function stat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
function stat(path: StringOrBuffer, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
function stat(path: StringOrBuffer, useBigint: false, req: FSReqCallback<Float64Array>): void;
Expand Down Expand Up @@ -276,6 +278,7 @@ export interface FsBinding {
realpath: typeof InternalFSBinding.realpath;
rename: typeof InternalFSBinding.rename;
rmdir: typeof InternalFSBinding.rmdir;
rmSync: typeof InternalFSBinding.rmSync;
stat: typeof InternalFSBinding.stat;
symlink: typeof InternalFSBinding.symlink;
unlink: typeof InternalFSBinding.unlink;
Expand Down

0 comments on commit 3d2975d

Please sign in to comment.