Skip to content

fs: improve error performance of fs.dir #53667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 9, 2024
Merged

fs: improve error performance of fs.dir #53667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 8 additions & 17 deletions lib/internal/fs/dir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ const {
getDirent,
getOptions,
getValidatedPath,
handleErrorFromBinding,
} = require('internal/fs/utils');
const {
validateFunction,
Expand Down Expand Up @@ -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() {
Expand All @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
64 changes: 30 additions & 34 deletions src/node_dir.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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);

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

Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand Down
8 changes: 5 additions & 3 deletions typings/internalBinding/fs_dir.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down