fs: move rmSync implementation to c++

Author: anonrig

lib/fs.js

@@ -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
@@ -1125,7 +1124,7 @@ function lazyLoadCp() {
 
 function lazyLoadRimraf() {
   if (rimraf === undefined)
-    ({ rimraf, rimrafSync } = require('internal/fs/rimraf'));
+    ({ rimraf } = require('internal/fs/rimraf'));
 }
 
 /**
@@ -1193,8 +1192,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);
@@ -1245,11 +1243,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);
 }
 
 /**

lib/internal/fs/rimraf.js

@@ -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);
 
@@ -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 };

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)                                          \

src/node_file.cc

@@ -48,6 +48,12 @@
 # include <io.h>
 #endif
 
+#ifdef _WIN32
+#include <windows.h>
+#else
+#include <unistd.h>
+#endif
+
 namespace node {
 
 namespace fs {
@@ -1599,6 +1605,102 @@ 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) {
+#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,
@@ -3317,6 +3419,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,
@@ -3441,6 +3544,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);

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);
-}

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<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;
@@ -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;