From 06aa4b9fe9b6ced043e9dac74dceb1e3891cdcf2 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sat, 14 Oct 2023 15:35:01 -0400 Subject: [PATCH] fs: improve error performance of `readdirSync` PR-URL: https://github.com/nodejs/node/pull/50131 Reviewed-By: Matteo Collina Reviewed-By: Geoffrey Booth --- lib/fs.js | 22 +++++++++---------- src/node_file.cc | 29 ++++++++++---------------- test/parallel/test-fs-readdir-types.js | 4 ++-- test/parallel/test-repl-underscore.js | 4 ++-- typings/internalBinding/fs.d.ts | 4 ++-- 5 files changed, 28 insertions(+), 35 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 72e81de4afa42c..31d8e60c7b674e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1408,17 +1408,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. @@ -1517,12 +1516,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 index 1cee74e35a87d8..786c113712c56b 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1926,8 +1926,8 @@ static void ReadDir(const FunctionCallbackInfo& 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)) @@ -1940,18 +1940,16 @@ static void ReadDir(const FunctionCallbackInfo& 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> name_v; std::vector> type_v; @@ -1962,12 +1960,8 @@ static void ReadDir(const FunctionCallbackInfo& args) { r = uv_fs_scandir_next(&(req_wrap_sync.req), &ent); if (r == UV_EOF) break; - if (r != 0) { - Local ctx = args[4].As(); - 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; } @@ -1978,8 +1972,7 @@ static void ReadDir(const FunctionCallbackInfo& args) { &error); if (filename.IsEmpty()) { - Local ctx = args[4].As(); - 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 index 9116a04f44ed70..3cc6b1cceff7fc 100644 --- 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 index 3abd01ba9d0cbc..0f8103ce4573cd 100644 --- 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 index 3ef49f4ab0ced7..c4dd973293ab22 100644 --- 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): 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; 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;