fs: improve error performance of fs.dir

anonrig · aduh95 · commit 128e514d81f0 · 2024-07-16T12:01:15.000+02:00
PR-URL: #53667 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js
@@ -26,7 +26,6 @@ const {
   getDirent,
   getOptions,
   getValidatedPath,
-  handleErrorFromBinding,
 } = require('internal/fs/utils');
 const {
   validateFunction,
@@ -155,27 +154,26 @@ class Dir {
 
   readSyncRecursive(dirent) {
     const path = pathModule.join(dirent.parentPath, dirent.name);
-    const ctx = { path };
     const handle = dirBinding.opendir(
       pathModule.toNamespacedPath(path),
       this.#options.encoding,
-      undefined,
-      ctx,
     );
-    handleErrorFromBinding(ctx);
+
+    // Terminate early, since it's already thrown.
+    if (handle === undefined) {
+      return;
+    }
+
     const result = handle.read(
       this.#options.encoding,
       this.#options.bufferSize,
-      undefined,
-      ctx,
     );
 
     if (result) {
       this.processReadResult(path, result);
     }
 
-    handle.close(undefined, ctx);
-    handleErrorFromBinding(ctx);
+    handle.close();
   }
 
   readSync() {
@@ -195,14 +193,10 @@ class Dir {
       return dirent;
     }
 
-    const ctx = { path: this.#path };
     const result = this.#handle.read(
       this.#options.encoding,
       this.#options.bufferSize,
-      undefined,
-      ctx,
     );
-    handleErrorFromBinding(ctx);
 
     if (result === null) {
       return result;
@@ -257,10 +251,7 @@ class Dir {
     }
 
     this.#closed = true;
-    const ctx = { path: this.#path };
-    const result = this.#handle.close(undefined, ctx);
-    handleErrorFromBinding(ctx);
-    return result;
+    this.#handle.close();
   }
 
   async* entries() {
diff --git a/src/node_dir.cc b/src/node_dir.cc
@@ -3,6 +3,7 @@
 #include "node_external_reference.h"
 #include "node_file-inl.h"
 #include "node_process-inl.h"
+#include "path.h"
 #include "permission/permission.h"
 #include "util.h"
 
@@ -183,26 +184,24 @@ void AfterClose(uv_fs_t* req) {
 void DirHandle::Close(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  const int argc = args.Length();
-  CHECK_GE(argc, 1);
+  CHECK_GE(args.Length(), 0);  // [req]
 
   DirHandle* dir;
   ASSIGN_OR_RETURN_UNWRAP(&dir, args.This());
 
   dir->closing_ = false;
   dir->closed_ = true;
 
-  FSReqBase* req_wrap_async = GetReqWrap(args, 0);
-  if (req_wrap_async != nullptr) {  // close(req)
+  if (!args[0]->IsUndefined()) {  // close(req)
+    FSReqBase* req_wrap_async = GetReqWrap(args, 0);
+    CHECK_NOT_NULL(req_wrap_async);
     FS_DIR_ASYNC_TRACE_BEGIN0(UV_FS_CLOSEDIR, req_wrap_async)
     AsyncCall(env, req_wrap_async, args, "closedir", UTF8, AfterClose,
               uv_fs_closedir, dir->dir());
-  } else {  // close(undefined, ctx)
-    CHECK_EQ(argc, 2);
-    FSReqWrapSync req_wrap_sync;
+  } else {  // close()
+    FSReqWrapSync req_wrap_sync("closedir");
     FS_DIR_SYNC_TRACE_BEGIN(closedir);
-    SyncCall(env, args[1], &req_wrap_sync, "closedir", uv_fs_closedir,
-             dir->dir());
+    SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_closedir, dir->dir());
     FS_DIR_SYNC_TRACE_END(closedir);
   }
 }
@@ -282,8 +281,7 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
 
-  const int argc = args.Length();
-  CHECK_GE(argc, 3);
+  CHECK_GE(args.Length(), 2);  // encoding, bufferSize, [callback]
 
   const enum encoding encoding = ParseEncoding(isolate, args[0], UTF8);
 
@@ -299,27 +297,25 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
     dir->dir_->dirents = dir->dirents_.data();
   }
 
-  FSReqBase* req_wrap_async = GetReqWrap(args, 2);
-  if (req_wrap_async != nullptr) {  // dir.read(encoding, bufferSize, req)
+  if (!args[2]->IsUndefined()) {  // dir.read(encoding, bufferSize, req)
+    FSReqBase* req_wrap_async = GetReqWrap(args, 2);
+    CHECK_NOT_NULL(req_wrap_async);
     FS_DIR_ASYNC_TRACE_BEGIN0(UV_FS_READDIR, req_wrap_async)
     AsyncCall(env, req_wrap_async, args, "readdir", encoding,
               AfterDirRead, uv_fs_readdir, dir->dir());
-  } else {  // dir.read(encoding, bufferSize, undefined, ctx)
-    CHECK_EQ(argc, 4);
-    FSReqWrapSync req_wrap_sync;
+  } else {  // dir.read(encoding, bufferSize)
+    FSReqWrapSync req_wrap_sync("readdir");
     FS_DIR_SYNC_TRACE_BEGIN(readdir);
-    int err = SyncCall(env, args[3], &req_wrap_sync, "readdir", uv_fs_readdir,
-                       dir->dir());
+    int err =
+        SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_readdir, dir->dir());
     FS_DIR_SYNC_TRACE_END(readdir);
     if (err < 0) {
-      return;  // syscall failed, no need to continue, error info is in ctx
+      return;  // syscall failed, no need to continue, error is already thrown
     }
 
     if (req_wrap_sync.req.result == 0) {
       // Done
-      Local<Value> done = Null(isolate);
-      args.GetReturnValue().Set(done);
-      return;
+      return args.GetReturnValue().SetNull();
     }
 
     CHECK_GE(req_wrap_sync.req.result, 0);
@@ -332,8 +328,9 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
                            encoding,
                            &error)
              .ToLocal(&js_array)) {
-      Local<Object> ctx = args[2].As<Object>();
-      USE(ctx->Set(env->context(), env->error_string(), error));
+      // TODO(anonrig): Initializing BufferValue here is wasteful.
+      BufferValue error_payload(isolate, error);
+      env->ThrowError(error_payload.out());
       return;
     }
 
@@ -362,16 +359,16 @@ static void OpenDir(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
 
-  const int argc = args.Length();
-  CHECK_GE(argc, 3);
+  CHECK_GE(args.Length(), 2);  // path, encoding, [callback]
 
   BufferValue path(isolate, args[0]);
   CHECK_NOT_NULL(*path);
 
   const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8);
 
-  FSReqBase* req_wrap_async = GetReqWrap(args, 2);
-  if (req_wrap_async != nullptr) {  // openDir(path, encoding, req)
+  if (!args[2]->IsUndefined()) {  // openDir(path, encoding, req)
+    FSReqBase* req_wrap_async = GetReqWrap(args, 2);
+    CHECK_NOT_NULL(req_wrap_async);
     ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS(
         env,
         req_wrap_async,
@@ -381,17 +378,16 @@ static void OpenDir(const FunctionCallbackInfo<Value>& args) {
         UV_FS_OPENDIR, req_wrap_async, "path", TRACE_STR_COPY(*path))
     AsyncCall(env, req_wrap_async, args, "opendir", encoding, AfterOpenDir,
               uv_fs_opendir, *path);
-  } else {  // openDir(path, encoding, undefined, ctx)
-    CHECK_EQ(argc, 4);
+  } else {  // openDir(path, encoding)
     THROW_IF_INSUFFICIENT_PERMISSIONS(
         env, permission::PermissionScope::kFileSystemRead, path.ToStringView());
-    FSReqWrapSync req_wrap_sync;
+    FSReqWrapSync req_wrap_sync("opendir", *path);
     FS_DIR_SYNC_TRACE_BEGIN(opendir);
-    int result = SyncCall(env, args[3], &req_wrap_sync, "opendir",
-                          uv_fs_opendir, *path);
+    int result =
+        SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_opendir, *path);
     FS_DIR_SYNC_TRACE_END(opendir);
     if (result < 0) {
-      return;  // syscall failed, no need to continue, error info is in ctx
+      return;  // syscall failed, no need to continue, error is already thrown
     }
 
     uv_fs_t* req = &req_wrap_sync.req;
diff --git a/typings/internalBinding/fs_dir.d.ts b/typings/internalBinding/fs_dir.d.ts
@@ -6,12 +6,14 @@ declare namespace InternalFSDirBinding {
   type StringOrBuffer = string | Buffer;
 
   class DirHandle {
-    read(encoding: string, bufferSize: number, _: unknown, ctx: ReadFileContext): string[] | undefined;
-    close(_: unknown, ctx: ReadFileContext): void;
+    read(encoding: string, bufferSize: number, callback: FSReqCallback): string[] | undefined;
+    read(encoding: string, bufferSize: number): string[] | undefined;
+    close(callback: FSReqCallback): void;
+    close(): void;
   }
 
   function opendir(path: StringOrBuffer, encoding: string, req: FSReqCallback): DirHandle;
-  function opendir(path: StringOrBuffer, encoding: string, _: undefined, ctx: ReadFileContext): DirHandle;
+  function opendir(path: StringOrBuffer, encoding: string): DirHandle;
   function opendirSync(path: StringOrBuffer): DirHandle;
 }
 
