From 717e233cd95602f79256c5b70c49703fa699174b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 25 Sep 2023 19:13:28 +0200 Subject: [PATCH 1/2] test: mark test-runner-output as flaky MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This has been flaky on many platforms for months. Mark it as flaky for now to avoid blocking the CI. PR-URL: https://github.com/nodejs/node/pull/49854 Refs: https://github.com/nodejs/node/issues/49853 Refs: https://github.com/nodejs/reliability/issues/673 Reviewed-By: Tobias Nießen Reviewed-By: Yagiz Nizipli Reviewed-By: LiviaMedeiros Reviewed-By: Luigi Pinca --- test/parallel/parallel.status | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index 6f4b9de73778b2..fe8ddee7cbf05e 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -5,6 +5,8 @@ prefix parallel # sample-test : PASS,FLAKY [true] # This section applies to all platforms +# https://github.com/nodejs/node/issues/49853 +test-runner-output: PASS,FLAKY [$system==win32] # https://github.com/nodejs/node/issues/41206 From f16f41c5b3cf2345abc7bd4d5e951a43479ce4a7 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Mon, 25 Sep 2023 17:56:03 -0400 Subject: [PATCH 2/2] 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 a19c4fa5bb7893..b76eb385295836 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2155,7 +2155,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()); @@ -2546,30 +2546,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)); @@ -2580,7 +2591,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;