Skip to content

Commit

Permalink
fs: improve error performance of readdirSync
Browse files Browse the repository at this point in the history
PR-URL: #50131
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
  • Loading branch information
anonrig authored Oct 14, 2023
1 parent 9df864d commit 18a8187
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 35 deletions.
22 changes: 11 additions & 11 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}

/**
Expand Down
29 changes: 11 additions & 18 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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;
Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-readdir-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});


Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-repl-underscore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Expand Down
4 changes: 2 additions & 2 deletions typings/internalBinding/fs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[]>;
Expand Down

0 comments on commit 18a8187

Please sign in to comment.