fs: improve error performance of readdirSync

PR-URL: #50131 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
nodejs · Oct 14, 2023 · 18a8187 · 18a8187
1 parent 9df864d
commit 18a8187
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[]>;