fs: improve error performance for fs.renameSync

nodejs · Sep 26, 2023 · 1d48a89 · 1d48a89
1 parent 31e727d
commit 1d48a89
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 6 deletions.
diff --git a/benchmark/fs/bench-renameSync.js b/benchmark/fs/bench-renameSync.js
@@ -0,0 +1,53 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const tmpdir = require('../../test/common/tmpdir');
+tmpdir.refresh();
+
+const bench = common.createBenchmark(main, {
+  type: ['invalid', 'valid'],
+  n: [1e3],
+});
+
+function main({ n, type }) {
+  tmpdir.refresh();
+
+  switch (type) {
+    case 'invalid': {
+      bench.start();
+      for (let i = 0; i < n; i++) {
+        try {
+          fs.renameSync(tmpdir.resolve(`.non-existing-file-${i}`), tmpdir.resolve(`.new-file-${i}`));
+        } catch {
+          // do nothing
+        }
+      }
+      bench.end(n);
+
+      break;
+    }
+    case 'valid': {
+      for (let i = 0; i < n; i++) {
+        fs.writeFileSync(tmpdir.resolve(`.existing-file-${i}`), 'bench', 'utf8');
+      }
+
+      bench.start();
+      for (let i = 0; i < n; i++) {
+        try {
+          fs.renameSync(
+            tmpdir.resolve(`.existing-file-${i}`),
+            tmpdir.resolve(`.new-existing-file-${i}`),
+          );
+        } catch {
+          // do nothing
+        }
+      }
+
+      bench.end(n);
+      break;
+    }
+    default:
+      throw new Error('Invalid type');
+  }
+}
diff --git a/lib/fs.js b/lib/fs.js
@@ -1015,12 +1015,7 @@ function rename(oldPath, newPath, callback) {
  * @returns {void}
  */
 function renameSync(oldPath, newPath) {
-  oldPath = getValidatedPath(oldPath, 'oldPath');
-  newPath = getValidatedPath(newPath, 'newPath');
-  const ctx = { path: oldPath, dest: newPath };
-  binding.rename(pathModule.toNamespacedPath(oldPath),
-                 pathModule.toNamespacedPath(newPath), undefined, ctx);
-  handleErrorFromBinding(ctx);
+  return syncFs.rename(oldPath, newPath);
 }
 
 /**

diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js
@@ -88,6 +88,15 @@ function close(fd) {
   return binding.closeSync(fd);
 }
 
+function rename(oldPath, newPath) {
+  oldPath = getValidatedPath(oldPath, 'oldPath');
+  newPath = getValidatedPath(newPath, 'newPath');
+  return binding.renameSync(
+    pathModule.toNamespacedPath(oldPath),
+    pathModule.toNamespacedPath(newPath),
+  );
+}
+
 module.exports = {
   readFileUtf8,
   exists,
@@ -97,4 +106,5 @@ module.exports = {
   statfs,
   open,
   close,
+  rename,
 };
diff --git a/src/node_file.cc b/src/node_file.cc
@@ -1568,6 +1568,38 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
+static void RenameSync(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  Isolate* isolate = env->isolate();
+
+  CHECK_EQ(args.Length(), 2);
+
+  BufferValue old_path(isolate, args[0]);
+  CHECK_NOT_NULL(*old_path);
+  auto view_old_path = old_path.ToStringView();
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, permission::PermissionScope::kFileSystemRead, view_old_path);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, permission::PermissionScope::kFileSystemWrite, view_old_path);
+
+  BufferValue new_path(isolate, args[1]);
+  CHECK_NOT_NULL(*new_path);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env,
+      permission::PermissionScope::kFileSystemWrite,
+      new_path.ToStringView());
+
+  uv_fs_t req;
+  auto cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
+  FS_SYNC_TRACE_BEGIN(rename);
+  int err = uv_fs_rename(nullptr, &req, *old_path, *new_path, nullptr);
+  FS_SYNC_TRACE_END(rename);
+
+  if (err < 0) {
+    return env->ThrowUVException(err, "rename", nullptr, *old_path, *new_path);
+  }
+}
+
 static void FTruncate(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -3374,6 +3406,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
   SetMethod(isolate, target, "fdatasync", Fdatasync);
   SetMethod(isolate, target, "fsync", Fsync);
   SetMethod(isolate, target, "rename", Rename);
+  SetMethod(isolate, target, "renameSync", RenameSync);
   SetMethod(isolate, target, "ftruncate", FTruncate);
   SetMethod(isolate, target, "rmdir", RMDir);
   SetMethod(isolate, target, "mkdir", MKDir);
@@ -3499,6 +3532,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(Fdatasync);
   registry->Register(Fsync);
   registry->Register(Rename);
+  registry->Register(RenameSync);
   registry->Register(FTruncate);
   registry->Register(RMDir);
   registry->Register(MKDir);

diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts
@@ -180,6 +180,7 @@ declare namespace InternalFSBinding {
   function rename(oldPath: string, newPath: string, req: FSReqCallback): void;
   function rename(oldPath: string, newPath: string, req: undefined, ctx: FSSyncContext): void;
   function rename(oldPath: string, newPath: string, usePromises: typeof kUsePromises): Promise<void>;
+  function renameSync(oldPath: string, newPath: string): void;
 
   function rmdir(path: string, req: FSReqCallback): void;
   function rmdir(path: string, req: undefined, ctx: FSSyncContext): void;