From 476e8d495441a91e5f8d4bc72ebbe223ecd27386 Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Thu, 21 Sep 2023 13:14:06 -0400
Subject: [PATCH] fs: improve error performance for `fs.mkdtempSync`

---
 benchmark/fs/bench-mkdtempSync.js | 38 +++++++++++++++++++++++++++++++
 lib/fs.js                         | 18 +--------------
 lib/internal/fs/sync.js           | 23 +++++++++++++++++++
 src/node_file.cc                  | 34 +++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 17 deletions(-)
 create mode 100644 benchmark/fs/bench-mkdtempSync.js

diff --git a/benchmark/fs/bench-mkdtempSync.js b/benchmark/fs/bench-mkdtempSync.js
new file mode 100644
index 00000000000000..c7c808b9a9608a
--- /dev/null
+++ b/benchmark/fs/bench-mkdtempSync.js
@@ -0,0 +1,38 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const tmpdir = require('../../test/common/tmpdir');
+tmpdir.refresh();
+
+const nonexistentDir = tmpdir.resolve('non-existent', 'foo', 'bar');
+const bench = common.createBenchmark(main, {
+  type: ['valid', 'invalid'],
+  n: [1e3],
+});
+
+function main({ n, type }) {
+  let prefix;
+
+  switch (type) {
+    case 'valid':
+      prefix = `${Date.now()}`;
+      break;
+    case 'invalid':
+      prefix = nonexistentDir;
+      break;
+    default:
+      new Error('Invalid type');
+  }
+
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    try {
+      const folderPath = fs.mkdtempSync(prefix, { encoding: 'utf8' });
+      fs.rmdirSync(folderPath);
+    } catch {
+      // do nothing
+    }
+  }
+  bench.end(n);
+}
diff --git a/lib/fs.js b/lib/fs.js
index 60594afe9809ad..13f721ac2c0084 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -2895,23 +2895,7 @@ function mkdtemp(prefix, options, callback) {
  * @returns {string}
  */
 function mkdtempSync(prefix, options) {
-  options = getOptions(options);
-
-  prefix = getValidatedPath(prefix, 'prefix');
-  warnOnNonPortableTemplate(prefix);
-
-  let path;
-  if (typeof prefix === 'string') {
-    path = `${prefix}XXXXXX`;
-  } else {
-    path = Buffer.concat([prefix, Buffer.from('XXXXXX')]);
-  }
-
-  const ctx = { path };
-  const result = binding.mkdtemp(path, options.encoding,
-                                 undefined, ctx);
-  handleErrorFromBinding(ctx);
-  return result;
+  return syncFs.mkdtemp(prefix, options);
 }
 
 /**
diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js
index 7fd356833b11a2..d54b147420d6b9 100644
--- a/lib/internal/fs/sync.js
+++ b/lib/internal/fs/sync.js
@@ -8,8 +8,11 @@ const {
   getStatsFromBinding,
   getStatFsFromBinding,
   getValidatedFd,
+  getOptions,
+  warnOnNonPortableTemplate,
 } = require('internal/fs/utils');
 const { parseFileMode } = require('internal/validators');
+const { Buffer } = require('buffer');
 
 const binding = internalBinding('fs');
 
@@ -51,6 +54,25 @@ function copyFile(src, dest, mode) {
   );
 }
 
+function mkdtemp(prefix, options) {
+  options = getOptions(options);
+
+  prefix = getValidatedPath(prefix, 'prefix');
+  warnOnNonPortableTemplate(prefix);
+
+  let path;
+  if (typeof prefix === 'string') {
+    path = `${prefix}XXXXXX`;
+  } else {
+    path = Buffer.concat([prefix, Buffer.from('XXXXXX')]);
+  }
+
+  return binding.mkdtempSync(
+    path,
+    options.encoding,
+  );
+}
+
 function stat(path, options = { bigint: false, throwIfNoEntry: true }) {
   path = getValidatedPath(path);
   const stats = binding.statSync(
@@ -91,6 +113,7 @@ module.exports = {
   exists,
   access,
   copyFile,
+  mkdtemp,
   stat,
   statfs,
   open,
diff --git a/src/node_file.cc b/src/node_file.cc
index 1e2d1c85322dab..e47ccb9c1e41fc 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -2947,6 +2947,38 @@ static void Mkdtemp(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
+static void MkdtempSync(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  Isolate* isolate = env->isolate();
+
+  CHECK_GE(args.Length(), 2);
+
+  BufferValue tmpl(isolate, args[0]);
+  CHECK_NOT_NULL(*tmpl);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, permission::PermissionScope::kFileSystemWrite, tmpl.ToStringView());
+
+  const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8);
+
+  uv_fs_t req;
+  auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
+  FS_SYNC_TRACE_BEGIN(mkdtemp);
+  int err = uv_fs_mkdtemp(nullptr, &req, *tmpl, nullptr);
+  FS_SYNC_TRACE_END(mkdtemp);
+  if (err < 0) {
+    return env->ThrowUVException(err, "mkdtemp", nullptr, *tmpl);
+  }
+
+  Local<Value> error;
+  MaybeLocal<Value> rc =
+      StringBytes::Encode(isolate, req.path, encoding, &error);
+  if (rc.IsEmpty()) {
+    env->isolate()->ThrowException(error);
+    return;
+  }
+  args.GetReturnValue().Set(rc.ToLocalChecked());
+}
+
 static bool FileURLToPath(
     Environment* env,
     const ada::url_aggregator& file_url,
@@ -3399,6 +3431,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
   SetMethod(isolate, target, "lutimes", LUTimes);
 
   SetMethod(isolate, target, "mkdtemp", Mkdtemp);
+  SetMethodNoSideEffect(isolate, target, "mkdtempSync", MkdtempSync);
 
   StatWatcher::CreatePerIsolateProperties(isolate_data, target);
   BindingData::CreatePerIsolateProperties(isolate_data, target);
@@ -3524,6 +3557,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(LUTimes);
 
   registry->Register(Mkdtemp);
+  registry->Register(MkdtempSync);
   registry->Register(NewFSReqCallback);
 
   registry->Register(FileHandle::New);