From 31702c94031e4c4b90a5314df7927aa8901b3d9a Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Mon, 25 Sep 2023 17:56:03 -0400 Subject: [PATCH] fs: improve `readFileSync` with file descriptors PR-URL: https://github.com/nodejs/node/pull/49691 Reviewed-By: Stephen Belanger --- benchmark/fs/readFileSync.js | 18 +++++++++++--- lib/fs.js | 6 ++--- lib/internal/fs/sync.js | 6 +++-- src/node_file.cc | 47 ++++++++++++++++++++++-------------- 4 files changed, 50 insertions(+), 27 deletions(-) diff --git a/benchmark/fs/readFileSync.js b/benchmark/fs/readFileSync.js index b81bdce8f27f69..800ab31450f43a 100644 --- a/benchmark/fs/readFileSync.js +++ b/benchmark/fs/readFileSync.js @@ -6,12 +6,21 @@ const fs = require('fs'); const bench = common.createBenchmark(main, { encoding: ['undefined', 'utf8'], path: ['existing', 'non-existing'], - n: [60e1], + hasFileDescriptor: ['true', 'false'], + n: [1e4], }); -function main({ n, encoding, path }) { +function main({ n, encoding, path, hasFileDescriptor }) { const enc = encoding === 'undefined' ? undefined : encoding; - const file = path === 'existing' ? __filename : '/tmp/not-found'; + let file; + let shouldClose = false; + + if (hasFileDescriptor === 'true') { + shouldClose = path === 'existing'; + file = path === 'existing' ? fs.openSync(__filename) : -1; + } else { + file = path === 'existing' ? __filename : '/tmp/not-found'; + } bench.start(); for (let i = 0; i < n; ++i) { try { @@ -21,4 +30,7 @@ function main({ n, encoding, path }) { } } bench.end(n); + if (shouldClose) { + fs.closeSync(file); + } } diff --git a/lib/fs.js b/lib/fs.js index 015e424efbdf4c..9b6b61dc8efd42 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -437,13 +437,11 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { function readFileSync(path, options) { options = getOptions(options, { flag: 'r' }); - const isUserFd = isFd(path); // File descriptor ownership - - // TODO(@anonrig): Do not handle file descriptor ownership for now. - if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) { + if (options.encoding === 'utf8' || options.encoding === 'utf-8') { return syncFs.readFileUtf8(path, options.flag); } + const isUserFd = isFd(path); // File descriptor ownership const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666); const stats = tryStatSync(fd, isUserFd); diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js index 7fd356833b11a2..0d4ba90150e186 100644 --- a/lib/internal/fs/sync.js +++ b/lib/internal/fs/sync.js @@ -9,7 +9,7 @@ const { getStatFsFromBinding, getValidatedFd, } = require('internal/fs/utils'); -const { parseFileMode } = require('internal/validators'); +const { parseFileMode, isInt32 } = require('internal/validators'); const binding = internalBinding('fs'); @@ -19,7 +19,9 @@ const binding = internalBinding('fs'); * @return {string} */ function readFileUtf8(path, flag) { - path = pathModule.toNamespacedPath(getValidatedPath(path)); + if (!isInt32(path)) { + path = pathModule.toNamespacedPath(getValidatedPath(path)); + } return binding.readFileUtf8(path, stringToFlags(flag)); } diff --git a/src/node_file.cc b/src/node_file.cc index 39d8ac078b435b..7ac61d55797061 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2190,7 +2190,7 @@ static void OpenSync(const FunctionCallbackInfo& args) { uv_fs_t req; auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); FS_SYNC_TRACE_BEGIN(open); - int err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr); + auto err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr); FS_SYNC_TRACE_END(open); if (err < 0) { return env->ThrowUVException(err, "open", nullptr, path.out()); @@ -2581,30 +2581,41 @@ static void ReadFileUtf8(const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 2); - BufferValue path(env->isolate(), args[0]); - CHECK_NOT_NULL(*path); - CHECK(args[1]->IsInt32()); const int flags = args[1].As()->Value(); - if (CheckOpenPermissions(env, path, flags).IsNothing()) return; - + uv_file file; uv_fs_t req; - auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - FS_SYNC_TRACE_BEGIN(open); - uv_file file = uv_fs_open(nullptr, &req, *path, flags, 438, nullptr); - FS_SYNC_TRACE_END(open); - if (req.result < 0) { - // req will be cleaned up by scope leave. - return env->ThrowUVException(req.result, "open", nullptr, path.out()); + bool is_fd = args[0]->IsInt32(); + + // Check for file descriptor + if (is_fd) { + file = args[0].As()->Value(); + } else { + BufferValue path(env->isolate(), args[0]); + CHECK_NOT_NULL(*path); + if (CheckOpenPermissions(env, path, flags).IsNothing()) return; + + FS_SYNC_TRACE_BEGIN(open); + file = uv_fs_open(nullptr, &req, *path, flags, O_RDONLY, nullptr); + FS_SYNC_TRACE_END(open); + if (req.result < 0) { + uv_fs_req_cleanup(&req); + // req will be cleaned up by scope leave. + return env->ThrowUVException(req.result, "open", nullptr, path.out()); + } } - auto defer_close = OnScopeLeave([file]() { - uv_fs_t close_req; - CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr)); - uv_fs_req_cleanup(&close_req); + auto defer_close = OnScopeLeave([file, is_fd, &req]() { + if (!is_fd) { + FS_SYNC_TRACE_BEGIN(close); + CHECK_EQ(0, uv_fs_close(nullptr, &req, file, nullptr)); + FS_SYNC_TRACE_END(close); + } + uv_fs_req_cleanup(&req); }); + std::string result{}; char buffer[8192]; uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer)); @@ -2615,7 +2626,7 @@ static void ReadFileUtf8(const FunctionCallbackInfo& args) { if (req.result < 0) { FS_SYNC_TRACE_END(read); // req will be cleaned up by scope leave. - return env->ThrowUVException(req.result, "read", nullptr, path.out()); + return env->ThrowUVException(req.result, "read", nullptr); } if (r <= 0) { break;