From 128e514d81f06ebcd9213fcbc2c77b5c6ff2d580 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 9 Jul 2024 10:38:01 -0400 Subject: [PATCH] fs: improve error performance of `fs.dir` PR-URL: https://github.com/nodejs/node/pull/53667 Reviewed-By: Daniel Lemire Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Mohammed Keyvanzadeh --- lib/internal/fs/dir.js | 25 ++++------- src/node_dir.cc | 64 ++++++++++++++--------------- typings/internalBinding/fs_dir.d.ts | 8 ++-- 3 files changed, 43 insertions(+), 54 deletions(-) diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index 7a7220815dc374..29ea53ac587dab 100644 --- 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 index f8f7af8b52f0bf..26d78aa8bcbf40 100644 --- 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,8 +184,7 @@ void AfterClose(uv_fs_t* req) { void DirHandle::Close(const FunctionCallbackInfo& 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()); @@ -192,17 +192,16 @@ void DirHandle::Close(const FunctionCallbackInfo& args) { 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& 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& 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 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& args) { encoding, &error) .ToLocal(&js_array)) { - Local ctx = args[2].As(); - 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& 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& 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 index 1b823dc820fc18..7e0eb1246c0c35 100644 --- 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; }