fs: improve error perf of sync lstat+fstat

CanadaHonk · anonrig · commit 44ee0a1eca13 · 2023-10-15T13:38:55.000-04:00
diff --git a/benchmark/fs/bench-statSync-failure.js b/benchmark/fs/bench-statSync-failure.js
@@ -5,21 +5,29 @@ const fs = require('fs');
 const path = require('path');
 
 const bench = common.createBenchmark(main, {
-  n: [1e6],
-  statSyncType: ['throw', 'noThrow'],
+  n: [1e4],
+  statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
+  throwType: [ 'throw', 'noThrow' ],
+}, {
+  // fstatSync does not support throwIfNoEntry
+  combinationFilter: ({ statSyncType, throwType }) => !(statSyncType === 'fstatSync' && throwType === 'noThrow'),
 });
 
 
-function main({ n, statSyncType }) {
-  const arg = path.join(__dirname, 'non.existent');
+function main({ n, statSyncType, throwType }) {
+  const arg = (statSyncType === 'fstatSync' ?
+    (1 << 30) :
+    path.join(__dirname, 'non.existent'));
+
+  const fn = fs[statSyncType];
 
   bench.start();
   for (let i = 0; i < n; i++) {
-    if (statSyncType === 'noThrow') {
-      fs.statSync(arg, { throwIfNoEntry: false });
+    if (throwType === 'noThrow') {
+      fn(arg, { throwIfNoEntry: false });
     } else {
       try {
-        fs.statSync(arg);
+        fn(arg);
       } catch {
         // Continue regardless of error.
       }
diff --git a/benchmark/fs/bench-statSync.js b/benchmark/fs/bench-statSync.js
@@ -4,7 +4,7 @@ const common = require('../common');
 const fs = require('fs');
 
 const bench = common.createBenchmark(main, {
-  n: [1e6],
+  n: [1e4],
   statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
 });
 
diff --git a/lib/fs.js b/lib/fs.js
@@ -72,8 +72,6 @@ const {
     ERR_INVALID_ARG_VALUE,
   },
   AbortError,
-  uvErrmapGet,
-  uvException,
 } = require('internal/errors');
 
 const {
@@ -398,11 +396,9 @@ function readFile(path, options, callback) {
 }
 
 function tryStatSync(fd, isUserFd) {
-  const ctx = {};
-  const stats = binding.fstat(fd, false, undefined, ctx);
-  if (ctx.errno !== undefined && !isUserFd) {
+  const stats = binding.fstat(fd, false, undefined, true /* shouldNotThrow */);
+  if (stats === undefined && !isUserFd) {
     fs.closeSync(fd);
-    throw uvException(ctx);
   }
   return stats;
 }
@@ -1616,33 +1612,21 @@ function statfs(path, options = { bigint: false }, callback) {
   binding.statfs(pathModule.toNamespacedPath(path), options.bigint, req);
 }
 
-function hasNoEntryError(ctx) {
-  if (ctx.errno) {
-    const uvErr = uvErrmapGet(ctx.errno);
-    return uvErr?.[0] === 'ENOENT';
-  }
-
-  if (ctx.error) {
-    return ctx.error.code === 'ENOENT';
-  }
-
-  return false;
-}
-
 /**
  * Synchronously retrieves the `fs.Stats` for
  * the file descriptor.
  * @param {number} fd
  * @param {{
  *   bigint?: boolean;
  *   }} [options]
- * @returns {Stats}
+ * @returns {Stats | undefined}
  */
 function fstatSync(fd, options = { bigint: false }) {
   fd = getValidatedFd(fd);
-  const ctx = { fd };
-  const stats = binding.fstat(fd, options.bigint, undefined, ctx);
-  handleErrorFromBinding(ctx);
+  const stats = binding.fstat(fd, options.bigint, undefined, false);
+  if (stats === undefined) {
+    return;
+  }
   return getStatsFromBinding(stats);
 }
 
@@ -1654,17 +1638,20 @@ function fstatSync(fd, options = { bigint: false }) {
  *   bigint?: boolean;
  *   throwIfNoEntry?: boolean;
  *   }} [options]
- * @returns {Stats}
+ * @returns {Stats | undefined}
  */
 function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
   path = getValidatedPath(path);
-  const ctx = { path };
-  const stats = binding.lstat(pathModule.toNamespacedPath(path),
-                              options.bigint, undefined, ctx);
-  if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
-    return undefined;
+  const stats = binding.lstat(
+    pathModule.toNamespacedPath(path),
+    options.bigint,
+    undefined,
+    options.throwIfNoEntry,
+  );
+
+  if (stats === undefined) {
+    return;
   }
-  handleErrorFromBinding(ctx);
   return getStatsFromBinding(stats);
 }
 
@@ -2649,9 +2636,10 @@ function realpathSync(p, options) {
 
   // On windows, check that the root exists. On unix there is no need.
   if (isWindows) {
-    const ctx = { path: base };
-    binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
-    handleErrorFromBinding(ctx);
+    const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
+    if (out === undefined) {
+      return;
+    }
     knownHard.add(base);
   }
 
@@ -2691,9 +2679,10 @@ function realpathSync(p, options) {
       // for our internal use.
 
       const baseLong = pathModule.toNamespacedPath(base);
-      const ctx = { path: base };
-      const stats = binding.lstat(baseLong, true, undefined, ctx);
-      handleErrorFromBinding(ctx);
+      const stats = binding.lstat(baseLong, true, undefined, true /* throwIfNoEntry */);
+      if (stats === undefined) {
+        return;
+      }
 
       if (!isFileType(stats, S_IFLNK)) {
         knownHard.add(base);
@@ -2734,9 +2723,10 @@ function realpathSync(p, options) {
 
     // On windows, check that the root exists. On unix there is no need.
     if (isWindows && !knownHard.has(base)) {
-      const ctx = { path: base };
-      binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
-      handleErrorFromBinding(ctx);
+      const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
+      if (out === undefined) {
+        return;
+      }
       knownHard.add(base);
     }
   }
diff --git a/src/node_file.cc b/src/node_file.cc
@@ -1192,21 +1192,26 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(*path);
 
   bool use_bigint = args[1]->IsTrue();
-  FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
-  if (req_wrap_async != nullptr) {  // lstat(path, use_bigint, req)
+  if (!args[2]->IsUndefined()) {  // lstat(path, use_bigint, req)
+    FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
     FS_ASYNC_TRACE_BEGIN1(
         UV_FS_LSTAT, req_wrap_async, "path", TRACE_STR_COPY(*path))
     AsyncCall(env, req_wrap_async, args, "lstat", UTF8, AfterStat,
               uv_fs_lstat, *path);
-  } else {  // lstat(path, use_bigint, undefined, ctx)
-    CHECK_EQ(argc, 4);
-    FSReqWrapSync req_wrap_sync;
+  } else {  // lstat(path, use_bigint, undefined, throw_if_no_entry)
+    bool do_not_throw_if_no_entry = args[3]->IsFalse();
+    FSReqWrapSync req_wrap_sync("lstat", *path);
     FS_SYNC_TRACE_BEGIN(lstat);
-    int err = SyncCall(env, args[3], &req_wrap_sync, "lstat", uv_fs_lstat,
-                       *path);
+    int result;
+    if (do_not_throw_if_no_entry) {
+      result = SyncCallAndThrowIf(
+          is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_lstat, *path);
+    } else {
+      result = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_lstat, *path);
+    }
     FS_SYNC_TRACE_END(lstat);
-    if (err != 0) {
-      return;  // error info is in ctx
+    if (is_uv_error(result)) {
+      return;
     }
 
     Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
@@ -1227,19 +1232,22 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
   int fd = args[0].As<Int32>()->Value();
 
   bool use_bigint = args[1]->IsTrue();
-  FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
-  if (req_wrap_async != nullptr) {  // fstat(fd, use_bigint, req)
+  if (!args[2]->IsUndefined()) {  // fstat(fd, use_bigint, req)
+    FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
     FS_ASYNC_TRACE_BEGIN0(UV_FS_FSTAT, req_wrap_async)
     AsyncCall(env, req_wrap_async, args, "fstat", UTF8, AfterStat,
               uv_fs_fstat, fd);
-  } else {  // fstat(fd, use_bigint, undefined, ctx)
-    CHECK_EQ(argc, 4);
-    FSReqWrapSync req_wrap_sync;
+  } else {  // fstat(fd, use_bigint, undefined, do_not_throw_error)
+    bool do_not_throw_error = args[2]->IsTrue();
+    const auto should_throw = [do_not_throw_error](int result) {
+      return is_uv_error(result) && !do_not_throw_error;
+    };
+    FSReqWrapSync req_wrap_sync("fstat");
     FS_SYNC_TRACE_BEGIN(fstat);
-    int err = SyncCall(env, args[3], &req_wrap_sync, "fstat", uv_fs_fstat, fd);
+    int err = SyncCallAndThrowIf(should_throw, env, &req_wrap_sync, uv_fs_fstat, fd);
     FS_SYNC_TRACE_END(fstat);
-    if (err != 0) {
-      return;  // error info is in ctx
+    if (is_uv_error(err)) {
+      return;
     }
 
     Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
diff --git a/test/parallel/test-fs-sync-fd-leak.js b/test/parallel/test-fs-sync-fd-leak.js
@@ -42,15 +42,11 @@ fs.writeSync = function() {
   throw new Error('BAM');
 };
 
-internalBinding('fs').fstat = function(fd, bigint, _, ctx) {
-  ctx.errno = UV_EBADF;
-  ctx.syscall = 'fstat';
+internalBinding('fs').fstat = function() {
+  throw new Error('EBADF: bad file descriptor, fstat');
 };
 
 let close_called = 0;
-ensureThrows(function() {
-  fs.readFileSync('dummy');
-}, 'EBADF: bad file descriptor, fstat');
 ensureThrows(function() {
   fs.writeFileSync('dummy', 'xxx');
 }, 'BAM');
diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts
@@ -91,9 +91,9 @@ declare namespace InternalFSBinding {
   function fstat(fd: number, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
   function fstat(fd: number, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
   function fstat(fd: number, useBigint: false, req: FSReqCallback<Float64Array>): void;
-  function fstat(fd: number, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
-  function fstat(fd: number, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
-  function fstat(fd: number, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
+  function fstat(fd: number, useBigint: boolean, req: undefined, shouldNotThrow: boolean): Float64Array | BigUint64Array;
+  function fstat(fd: number, useBigint: true, req: undefined, shouldNotThrow: boolean): BigUint64Array;
+  function fstat(fd: number, useBigint: false, req: undefined, shouldNotThrow: boolean): Float64Array;
   function fstat(fd: number, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
   function fstat(fd: number, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
   function fstat(fd: number, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;
@@ -124,9 +124,9 @@ declare namespace InternalFSBinding {
   function lstat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
   function lstat(path: StringOrBuffer, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
   function lstat(path: StringOrBuffer, useBigint: false, req: FSReqCallback<Float64Array>): void;
-  function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
-  function lstat(path: StringOrBuffer, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
-  function lstat(path: StringOrBuffer, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
+  function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, throwIfNoEntry: boolean): Float64Array | BigUint64Array;
+  function lstat(path: StringOrBuffer, useBigint: true, req: undefined, throwIfNoEntry: boolean): BigUint64Array;
+  function lstat(path: StringOrBuffer, useBigint: false, req: undefined, throwIfNoEntry: boolean): Float64Array;
   function lstat(path: StringOrBuffer, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
   function lstat(path: StringOrBuffer, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
   function lstat(path: StringOrBuffer, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;
