fs: improve error performance of readdirSync

PR-URL: nodejs#50131 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
alexfernandez · Nov 1, 2023 · b7b3ef0 · b7b3ef0
1 parent 5db165c
commit b7b3ef0
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 35 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -1410,17 +1410,16 @@ function readdirSyncRecursive(basePath, options) {
   const readdirResults = [];
   const pathsQueue = [basePath];
 
-  const ctx = { path: basePath };
   function read(path) {
-    ctx.path = path;
     const readdirResult = binding.readdir(
       pathModule.toNamespacedPath(path),
       encoding,
       withFileTypes,
-      undefined,
-      ctx,
     );
-    handleErrorFromBinding(ctx);
+
+    if (readdirResult === undefined) {
+      return;
+    }
 
     if (withFileTypes) {
       // Calling `readdir` with `withFileTypes=true`, the result is an array of arrays.
@@ -1519,12 +1518,13 @@ function readdirSync(path, options) {
     return readdirSyncRecursive(path, options);
   }
 
-  const ctx = { path };
-  const result = binding.readdir(pathModule.toNamespacedPath(path),
-                                 options.encoding, !!options.withFileTypes,
-                                 undefined, ctx);
-  handleErrorFromBinding(ctx);
-  return options.withFileTypes ? getDirents(path, result) : result;
+  const result = binding.readdir(
+    pathModule.toNamespacedPath(path),
+    options.encoding,
+    !!options.withFileTypes,
+  );
+
+  return result !== undefined && options.withFileTypes ? getDirents(path, result) : result;
 }
 
 /**

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -1891,8 +1891,8 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
 
   bool with_types = args[2]->IsTrue();
 
-  FSReqBase* req_wrap_async = GetReqWrap(args, 3);
-  if (req_wrap_async != nullptr) {  // readdir(path, encoding, withTypes, req)
+  if (argc > 3) {  // readdir(path, encoding, withTypes, req)
+    FSReqBase* req_wrap_async = GetReqWrap(args, 3);
     req_wrap_async->set_with_file_types(with_types);
     FS_ASYNC_TRACE_BEGIN1(
         UV_FS_SCANDIR, req_wrap_async, "path", TRACE_STR_COPY(*path))
@@ -1905,18 +1905,16 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
               uv_fs_scandir,
               *path,
               0 /*flags*/);
-  } else {  // readdir(path, encoding, withTypes, undefined, ctx)
-    CHECK_EQ(argc, 5);
-    FSReqWrapSync req_wrap_sync;
+  } else {  // readdir(path, encoding, withTypes)
+    FSReqWrapSync req_wrap_sync("scandir", *path);
     FS_SYNC_TRACE_BEGIN(readdir);
-    int err = SyncCall(env, args[4], &req_wrap_sync, "scandir",
-                       uv_fs_scandir, *path, 0 /*flags*/);
+    int err = SyncCallAndThrowOnError(
+        env, &req_wrap_sync, uv_fs_scandir, *path, 0 /*flags*/);
     FS_SYNC_TRACE_END(readdir);
-    if (err < 0) {
-      return;  // syscall failed, no need to continue, error info is in ctx
+    if (is_uv_error(err)) {
+      return;
     }
 
-    CHECK_GE(req_wrap_sync.req.result, 0);
     int r;
     std::vector<Local<Value>> name_v;
     std::vector<Local<Value>> type_v;
@@ -1927,12 +1925,8 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
       r = uv_fs_scandir_next(&(req_wrap_sync.req), &ent);
       if (r == UV_EOF)
         break;
-      if (r != 0) {
-        Local<Object> ctx = args[4].As<Object>();
-        ctx->Set(env->context(), env->errno_string(),
-                 Integer::New(isolate, r)).Check();
-        ctx->Set(env->context(), env->syscall_string(),
-                 OneByteString(isolate, "readdir")).Check();
+      if (is_uv_error(r)) {
+        env->ThrowUVException(r, "scandir", nullptr, *path);
         return;
       }
 
@@ -1943,8 +1937,7 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
                                                        &error);
 
       if (filename.IsEmpty()) {
-        Local<Object> ctx = args[4].As<Object>();
-        ctx->Set(env->context(), env->error_string(), error).Check();
+        isolate->ThrowException(error);
         return;
       }
 

diff --git a/test/parallel/test-fs-readdir-types.js b/test/parallel/test-fs-readdir-types.js
@@ -29,7 +29,7 @@ tmpdir.refresh();
 
 // Create the necessary files
 files.forEach(function(currentFile) {
-  fs.closeSync(fs.openSync(`${readdirDir}/${currentFile}`, 'w'));
+  fs.writeFileSync(`${readdirDir}/${currentFile}`, '', 'utf8');
 });
 
 
@@ -95,7 +95,7 @@ binding.readdir = common.mustCall((path, encoding, types, req, ctx) => {
     };
     oldReaddir(path, encoding, types, req);
   } else {
-    const results = oldReaddir(path, encoding, types, req, ctx);
+    const results = oldReaddir(path, encoding, types);
     results[1] = results[1].map(() => UNKNOWN);
     return results;
   }

diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js
@@ -180,9 +180,9 @@ function testError() {
       /^Uncaught Error: ENOENT: no such file or directory, scandir '.*nonexistent\?'/,
       /Object\.readdirSync/,
       /^ {2}errno: -(2|4058),$/,
-      "  syscall: 'scandir',",
       "  code: 'ENOENT',",
-      "  path: '/nonexistent?'",
+      "  syscall: 'scandir',",
+      /^ {2}path: '*'/,
       '}',
       "'ENOENT'",
       "'scandir'",

diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts
@@ -166,8 +166,8 @@ declare namespace InternalFSBinding {
   function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: true, req: FSReqCallback<[string[], number[]]>): void;
   function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: false, req: FSReqCallback<string[]>): void;
   function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: boolean, req: undefined, ctx: FSSyncContext): string[] | [string[], number[]];
-  function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: true, req: undefined, ctx: FSSyncContext): [string[], number[]];
-  function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: false, req: undefined, ctx: FSSyncContext): string[];
+  function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: true): [string[], number[]];
+  function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: false): string[];
   function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: boolean, usePromises: typeof kUsePromises): Promise<string[] | [string[], number[]]>;
   function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: true, usePromises: typeof kUsePromises): Promise<[string[], number[]]>;
   function readdir(path: StringOrBuffer, encoding: unknown, withFileTypes: false, usePromises: typeof kUsePromises): Promise<string[]>;