From 7168295e7a94e6cfef69e43d26d7d5b57d3e1cf9 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Thu, 18 Jul 2024 16:02:49 -0400 Subject: [PATCH] fs: move `rmSync` implementation to c++ PR-URL: https://github.com/nodejs/node/pull/53617 Reviewed-By: Daniel Lemire Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum --- lib/fs.js | 13 +- lib/internal/fs/rimraf.js | 145 +---------------------- src/node_errors.h | 1 + src/node_file.cc | 104 ++++++++++++++++ test/parallel/test-fs-rmdir-recursive.js | 25 ---- typings/internalBinding/fs.d.ts | 3 + 6 files changed, 114 insertions(+), 177 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 65889ee97ea..2119554c0e7 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -161,7 +161,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 @@ -1124,7 +1123,7 @@ function lazyLoadCp() { function lazyLoadRimraf() { if (rimraf === undefined) - ({ rimraf, rimrafSync } = require('internal/fs/rimraf')); + ({ rimraf } = require('internal/fs/rimraf')); } /** @@ -1192,8 +1191,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); @@ -1244,11 +1242,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); } /** diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index 967ca6600a3..7269c0fc3fd 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -16,26 +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, isWindows } = require('internal/util'); +const { isWindows } = require('internal/util'); const notEmptyErrorCodes = new SafeSet(['ENOTEMPTY', 'EEXIST', 'EPERM']); const retryErrorCodes = new SafeSet( ['EBUSY', 'EMFILE', 'ENFILE', 'ENOTEMPTY', 'EPERM']); const epermHandler = isWindows ? fixWinEPERM : _rmdir; -const epermHandlerSync = isWindows ? fixWinEPERMSync : _rmdirSync; const readdirEncoding = 'buffer'; const separator = Buffer.from(sep); @@ -172,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 }; diff --git a/src/node_errors.h b/src/node_errors.h index 0a74373cf5d..b6e6b6ee8f9 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -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) \ diff --git a/src/node_file.cc b/src/node_file.cc index 81f7e8d2dd5..76463c2a823 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -48,6 +48,12 @@ # include #endif +#ifdef _WIN32 +#include +#else +#include +#endif + namespace node { namespace fs { @@ -1624,6 +1630,102 @@ static void RMDir(const FunctionCallbackInfo& args) { } } +static void RmSync(const FunctionCallbackInfo& 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()->Value(); + int recursive = args[2]->IsTrue(); + int retryDelay = args[3].As()->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) { +#ifdef _WIN32 + Sleep(retryDelay / 1000); +#else + sleep(retryDelay / 1000); +#endif + } + maxRetries--; + } + + // This is required since std::filesystem::path::c_str() returns different + // values in Windows and Unix. +#ifdef _WIN32 + auto file_ = file_path.string().c_str(); + int permission_denied_error = EPERM; +#else + auto file_ = file_path.c_str(); + int permission_denied_error = EACCES; +#endif // !_WIN32 + + 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_); + } 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_); + } 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_); + } else if (error == std::errc::permission_denied) { + std::string message = "Permission denied: " + file_path.string(); + return env->ThrowErrnoException( + permission_denied_error, "rm", message.c_str(), file_); + } + + std::string message = "Unknown error: " + error.message(); + return env->ThrowErrnoException(UV_UNKNOWN, "rm", message.c_str(), file_); +} + int MKDirpSync(uv_loop_t* loop, uv_fs_t* req, const std::string& path, @@ -3342,6 +3444,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, @@ -3468,6 +3571,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); diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index f98376488ed..77c205794b1 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -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); -} diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index db8eac020ea..5c385157693 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -196,6 +196,8 @@ declare namespace InternalFSBinding { function rmdir(path: string): void; function rmdir(path: string, usePromises: typeof kUsePromises): Promise; + function rmSync(path: StringOrBuffer, maxRetries: number, recursive: boolean, retryDelay: number): void; + function stat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback): void; function stat(path: StringOrBuffer, useBigint: true, req: FSReqCallback): void; function stat(path: StringOrBuffer, useBigint: false, req: FSReqCallback): void; @@ -279,6 +281,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;