From 938471ef556f2d64257059b60889a8c84621eea6 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 17 Sep 2023 16:42:46 -0400 Subject: [PATCH] fs: improve error performance of sync methods PR-URL: https://github.com/nodejs/node/pull/49593 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy --- benchmark/fs/bench-accessSync.js | 42 +++ benchmark/fs/bench-copyFileSync.js | 37 +++ benchmark/fs/bench-existsSync.js | 38 +++ benchmark/fs/bench-openSync.js | 37 +++ lib/fs.js | 79 +----- lib/internal/fs/read/utf8.js | 25 -- lib/internal/fs/sync.js | 98 +++++++ src/node_file.cc | 336 +++++++++++++++++++----- test/parallel/test-assert.js | 33 --- test/parallel/test-bootstrap-modules.js | 2 +- 10 files changed, 529 insertions(+), 198 deletions(-) create mode 100644 benchmark/fs/bench-accessSync.js create mode 100644 benchmark/fs/bench-copyFileSync.js create mode 100644 benchmark/fs/bench-existsSync.js create mode 100644 benchmark/fs/bench-openSync.js delete mode 100644 lib/internal/fs/read/utf8.js create mode 100644 lib/internal/fs/sync.js diff --git a/benchmark/fs/bench-accessSync.js b/benchmark/fs/bench-accessSync.js new file mode 100644 index 00000000000000..a80504620580ce --- /dev/null +++ b/benchmark/fs/bench-accessSync.js @@ -0,0 +1,42 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); + +const tmpfile = tmpdir.resolve(`.existing-file-${process.pid}`); +fs.writeFileSync(tmpfile, 'this-is-for-a-benchmark', 'utf8'); + +const bench = common.createBenchmark(main, { + type: ['existing', 'non-existing', 'non-flat-existing'], + n: [1e5], +}); + +function main({ n, type }) { + let path; + + switch (type) { + case 'existing': + path = __filename; + break; + case 'non-flat-existing': + path = tmpfile; + break; + case 'non-existing': + path = tmpdir.resolve(`.non-existing-file-${process.pid}`); + break; + default: + new Error('Invalid type'); + } + + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.accessSync(path); + } catch { + // do nothing + } + } + bench.end(n); +} diff --git a/benchmark/fs/bench-copyFileSync.js b/benchmark/fs/bench-copyFileSync.js new file mode 100644 index 00000000000000..af77fbaaaaa004 --- /dev/null +++ b/benchmark/fs/bench-copyFileSync.js @@ -0,0 +1,37 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); + +const bench = common.createBenchmark(main, { + type: ['invalid', 'valid'], + n: [1e4], +}); + +function main({ n, type }) { + tmpdir.refresh(); + const dest = tmpdir.resolve(`copy-file-bench-${process.pid}`); + let path; + + switch (type) { + case 'invalid': + path = tmpdir.resolve(`.existing-file-${process.pid}`); + break; + case 'valid': + path = __filename; + break; + default: + throw new Error('Invalid type'); + } + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.copyFileSync(path, dest); + } catch { + // do nothing + } + } + bench.end(n); +} diff --git a/benchmark/fs/bench-existsSync.js b/benchmark/fs/bench-existsSync.js new file mode 100644 index 00000000000000..f9da2765b130f7 --- /dev/null +++ b/benchmark/fs/bench-existsSync.js @@ -0,0 +1,38 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); + +const tmpfile = tmpdir.resolve(`.existing-file-${process.pid}`); +fs.writeFileSync(tmpfile, 'this-is-for-a-benchmark', 'utf8'); + +const bench = common.createBenchmark(main, { + type: ['existing', 'non-existing', 'non-flat-existing'], + n: [1e6], +}); + +function main({ n, type }) { + let path; + + switch (type) { + case 'existing': + path = __filename; + break; + case 'non-flat-existing': + path = tmpfile; + break; + case 'non-existing': + path = tmpdir.resolve(`.non-existing-file-${process.pid}`); + break; + default: + new Error('Invalid type'); + } + + bench.start(); + for (let i = 0; i < n; i++) { + fs.existsSync(path); + } + bench.end(n); +} diff --git a/benchmark/fs/bench-openSync.js b/benchmark/fs/bench-openSync.js new file mode 100644 index 00000000000000..eaa56139dcbf3c --- /dev/null +++ b/benchmark/fs/bench-openSync.js @@ -0,0 +1,37 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); + +const bench = common.createBenchmark(main, { + type: ['existing', 'non-existing'], + n: [1e5], +}); + +function main({ n, type }) { + let path; + + switch (type) { + case 'existing': + path = __filename; + break; + case 'non-existing': + path = tmpdir.resolve(`.non-existing-file-${process.pid}`); + break; + default: + new Error('Invalid type'); + } + + bench.start(); + for (let i = 0; i < n; i++) { + try { + const fd = fs.openSync(path, 'r', 0o666); + fs.closeSync(fd); + } catch { + // do nothing + } + } + bench.end(n); +} diff --git a/lib/fs.js b/lib/fs.js index b17cf4f10cd3c1..60594afe9809ad 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -141,7 +141,7 @@ const { validateObject, validateString, } = require('internal/validators'); -const { readFileSyncUtf8 } = require('internal/fs/read/utf8'); +const syncFs = require('internal/fs/sync'); let truncateWarn = true; let fs; @@ -243,12 +243,7 @@ function access(path, mode, callback) { * @returns {void} */ function accessSync(path, mode) { - path = getValidatedPath(path); - mode = getValidMode(mode, 'access'); - - const ctx = { path }; - binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx); - handleErrorFromBinding(ctx); + syncFs.access(path, mode); } /** @@ -290,23 +285,7 @@ ObjectDefineProperty(exists, kCustomPromisifiedSymbol, { * @returns {boolean} */ function existsSync(path) { - try { - path = getValidatedPath(path); - } catch { - return false; - } - const ctx = { path }; - const nPath = pathModule.toNamespacedPath(path); - binding.access(nPath, F_OK, undefined, ctx); - - // In case of an invalid symlink, `binding.access()` on win32 - // will **not** return an error and is therefore not enough. - // Double check with `binding.stat()`. - if (isWindows && ctx.errno === undefined) { - binding.stat(nPath, false, undefined, ctx); - } - - return ctx.errno === undefined; + return syncFs.exists(path); } function readFileAfterOpen(err, fd) { @@ -462,8 +441,7 @@ function readFileSync(path, options) { // TODO(@anonrig): Do not handle file descriptor ownership for now. if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) { - path = getValidatedPath(path); - return readFileSyncUtf8(pathModule.toNamespacedPath(path), stringToFlags(options.flag)); + return syncFs.readFileUtf8(path, options.flag); } const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666); @@ -540,11 +518,7 @@ function close(fd, callback = defaultCloseCallback) { * @returns {void} */ function closeSync(fd) { - fd = getValidatedFd(fd); - - const ctx = {}; - binding.close(fd, undefined, ctx); - handleErrorFromBinding(ctx); + return syncFs.close(fd); } /** @@ -590,16 +564,7 @@ function open(path, flags, mode, callback) { * @returns {number} */ function openSync(path, flags, mode) { - path = getValidatedPath(path); - const flagsNumber = stringToFlags(flags); - mode = parseFileMode(mode, 'mode', 0o666); - - const ctx = { path }; - const result = binding.open(pathModule.toNamespacedPath(path), - flagsNumber, mode, - undefined, ctx); - handleErrorFromBinding(ctx); - return result; + return syncFs.open(path, flags, mode); } /** @@ -1702,25 +1667,12 @@ function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) { * }} [options] * @returns {Stats} */ -function statSync(path, options = { bigint: false, throwIfNoEntry: true }) { - path = getValidatedPath(path); - const ctx = { path }; - const stats = binding.stat(pathModule.toNamespacedPath(path), - options.bigint, undefined, ctx); - if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) { - return undefined; - } - handleErrorFromBinding(ctx); - return getStatsFromBinding(stats); +function statSync(path, options) { + return syncFs.stat(path, options); } -function statfsSync(path, options = { bigint: false }) { - path = getValidatedPath(path); - const ctx = { path }; - const stats = binding.statfs(pathModule.toNamespacedPath(path), - options.bigint, undefined, ctx); - handleErrorFromBinding(ctx); - return getStatFsFromBinding(stats); +function statfsSync(path, options) { + return syncFs.statfs(path, options); } /** @@ -2999,16 +2951,7 @@ function copyFile(src, dest, mode, callback) { * @returns {void} */ function copyFileSync(src, dest, mode) { - src = getValidatedPath(src, 'src'); - dest = getValidatedPath(dest, 'dest'); - - const ctx = { path: src, dest }; // non-prefixed - - src = pathModule._makeLong(src); - dest = pathModule._makeLong(dest); - mode = getValidMode(mode, 'copyFile'); - binding.copyFile(src, dest, mode, undefined, ctx); - handleErrorFromBinding(ctx); + syncFs.copyFile(src, dest, mode); } /** diff --git a/lib/internal/fs/read/utf8.js b/lib/internal/fs/read/utf8.js deleted file mode 100644 index 5159db5988ee0b..00000000000000 --- a/lib/internal/fs/read/utf8.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict'; - -const { handleErrorFromBinding } = require('internal/fs/utils'); - -const binding = internalBinding('fs'); - -/** - * @param {string} path - * @param {number} flag - * @return {string} - */ -function readFileSyncUtf8(path, flag) { - const response = binding.readFileSync(path, flag); - - if (typeof response === 'string') { - return response; - } - - const { 0: errno, 1: syscall } = response; - handleErrorFromBinding({ errno, syscall, path }); -} - -module.exports = { - readFileSyncUtf8, -}; diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js new file mode 100644 index 00000000000000..7fd356833b11a2 --- /dev/null +++ b/lib/internal/fs/sync.js @@ -0,0 +1,98 @@ +'use strict'; + +const pathModule = require('path'); +const { + getValidatedPath, + stringToFlags, + getValidMode, + getStatsFromBinding, + getStatFsFromBinding, + getValidatedFd, +} = require('internal/fs/utils'); +const { parseFileMode } = require('internal/validators'); + +const binding = internalBinding('fs'); + +/** + * @param {string} path + * @param {number} flag + * @return {string} + */ +function readFileUtf8(path, flag) { + path = pathModule.toNamespacedPath(getValidatedPath(path)); + return binding.readFileUtf8(path, stringToFlags(flag)); +} + +function exists(path) { + try { + path = getValidatedPath(path); + } catch { + return false; + } + + return binding.existsSync(pathModule.toNamespacedPath(path)); +} + +function access(path, mode) { + path = getValidatedPath(path); + mode = getValidMode(mode, 'access'); + + binding.accessSync(pathModule.toNamespacedPath(path), mode); +} + +function copyFile(src, dest, mode) { + src = getValidatedPath(src, 'src'); + dest = getValidatedPath(dest, 'dest'); + + binding.copyFileSync( + pathModule.toNamespacedPath(src), + pathModule.toNamespacedPath(dest), + getValidMode(mode, 'copyFile'), + ); +} + +function stat(path, options = { bigint: false, throwIfNoEntry: true }) { + path = getValidatedPath(path); + const stats = binding.statSync( + pathModule.toNamespacedPath(path), + options.bigint, + options.throwIfNoEntry, + ); + if (stats === undefined) { + return undefined; + } + return getStatsFromBinding(stats); +} + +function statfs(path, options = { bigint: false }) { + path = getValidatedPath(path); + const stats = binding.statfsSync(pathModule.toNamespacedPath(path), options.bigint); + return getStatFsFromBinding(stats); +} + +function open(path, flags, mode) { + path = getValidatedPath(path); + + return binding.openSync( + pathModule.toNamespacedPath(path), + stringToFlags(flags), + parseFileMode(mode, 'mode', 0o666), + ); +} + +function close(fd) { + fd = getValidatedFd(fd); + + return binding.closeSync(fd); +} + +module.exports = { + readFileUtf8, + exists, + access, + copyFile, + stat, + statfs, + open, + close, +}; diff --git a/src/node_file.cc b/src/node_file.cc index f88c23034d1b91..9040e00eb0fdac 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -996,6 +996,31 @@ void Access(const FunctionCallbackInfo& args) { } } +static void AccessSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + + const int argc = args.Length(); + CHECK_GE(argc, 2); + + CHECK(args[1]->IsInt32()); + int mode = args[1].As()->Value(); + + BufferValue path(isolate, args[0]); + CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); + + uv_fs_t req; + FS_SYNC_TRACE_BEGIN(access); + int err = uv_fs_access(nullptr, &req, *path, mode, nullptr); + uv_fs_req_cleanup(&req); + FS_SYNC_TRACE_END(access); + + if (err) { + return env->ThrowUVException(err, "access", nullptr, path.out()); + } +} void Close(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1021,6 +1046,54 @@ void Close(const FunctionCallbackInfo& args) { } } +static void CloseSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + CHECK_GE(args.Length(), 1); + CHECK(args[0]->IsInt32()); + + int fd = args[0].As()->Value(); + env->RemoveUnmanagedFd(fd); + + uv_fs_t req; + FS_SYNC_TRACE_BEGIN(close); + int err = uv_fs_close(nullptr, &req, fd, nullptr); + FS_SYNC_TRACE_END(close); + uv_fs_req_cleanup(&req); + + if (err < 0) { + return env->ThrowUVException(err, "close"); + } +} + +static void ExistsSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + CHECK_GE(args.Length(), 1); + + BufferValue path(isolate, args[0]); + CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); + + uv_fs_t req; + auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + FS_SYNC_TRACE_BEGIN(access); + int err = uv_fs_access(nullptr, &req, path.out(), 0, nullptr); + FS_SYNC_TRACE_END(access); + +#ifdef _WIN32 + // In case of an invalid symlink, `uv_fs_access` on win32 + // will **not** return an error and is therefore not enough. + // Double check with `uv_fs_stat()`. + if (err == 0) { + FS_SYNC_TRACE_BEGIN(stat); + err = uv_fs_stat(nullptr, &req, path.out(), nullptr); + FS_SYNC_TRACE_END(stat); + } +#endif // _WIN32 + + args.GetReturnValue().Set(err == 0); +} // Used to speed up module loading. Returns an array [string, boolean] static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { @@ -1177,6 +1250,41 @@ static void Stat(const FunctionCallbackInfo& args) { } } +static void StatSync(const FunctionCallbackInfo& args) { + Realm* realm = Realm::GetCurrent(args); + BindingData* binding_data = realm->GetBindingData(); + Environment* env = realm->env(); + + CHECK_GE(args.Length(), 3); + + BufferValue path(realm->isolate(), args[0]); + bool use_bigint = args[1]->IsTrue(); + bool do_not_throw_if_no_entry = args[2]->IsFalse(); + CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); + + env->PrintSyncTrace(); + + uv_fs_t req; + auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + + FS_SYNC_TRACE_BEGIN(stat); + int err = uv_fs_stat(nullptr, &req, *path, nullptr); + FS_SYNC_TRACE_END(stat); + + if (err < 0) { + if (err == UV_ENOENT && do_not_throw_if_no_entry) { + return; + } + return env->ThrowUVException(err, "stat", nullptr, path.out()); + } + + Local arr = FillGlobalStatsArray( + binding_data, use_bigint, static_cast(req.ptr)); + args.GetReturnValue().Set(arr); +} + static void LStat(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); @@ -1290,6 +1398,34 @@ static void StatFs(const FunctionCallbackInfo& args) { } } +static void StatFsSync(const FunctionCallbackInfo& args) { + Realm* realm = Realm::GetCurrent(args); + BindingData* binding_data = realm->GetBindingData(); + Environment* env = realm->env(); + + CHECK_GE(args.Length(), 2); + + BufferValue path(realm->isolate(), args[0]); + bool use_bigint = args[1]->IsTrue(); + + CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); + + uv_fs_t req; + auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + FS_SYNC_TRACE_BEGIN(statfs); + int err = uv_fs_statfs(nullptr, &req, *path, nullptr); + FS_SYNC_TRACE_END(statfs); + if (err < 0) { + return env->ThrowUVException(err, "statfs", *path, nullptr); + } + + Local arr = FillGlobalStatFsArray( + binding_data, use_bigint, static_cast(req.ptr)); + args.GetReturnValue().Set(arr); +} + static void Symlink(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -1999,74 +2135,6 @@ static inline Maybe CheckOpenPermissions(Environment* env, return JustVoid(); } -static void ReadFileSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - auto isolate = env->isolate(); - - 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_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. - Local out[] = { - Integer::New(isolate, req.result), // errno - FIXED_ONE_BYTE_STRING(isolate, "open"), // syscall - }; - return args.GetReturnValue().Set(Array::New(isolate, out, arraysize(out))); - } - uv_fs_req_cleanup(&req); - - 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); - }); - - std::string result{}; - char buffer[8192]; - uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer)); - - FS_SYNC_TRACE_BEGIN(read); - while (true) { - auto r = uv_fs_read(nullptr, &req, file, &buf, 1, -1, nullptr); - if (req.result < 0) { - FS_SYNC_TRACE_END(read); - // req will be cleaned up by scope leave. - Local out[] = { - Integer::New(isolate, req.result), // errno - FIXED_ONE_BYTE_STRING(isolate, "read"), // syscall - }; - return args.GetReturnValue().Set( - Array::New(isolate, out, arraysize(out))); - } - uv_fs_req_cleanup(&req); - if (r <= 0) { - break; - } - result.append(buf.base, r); - } - FS_SYNC_TRACE_END(read); - - args.GetReturnValue().Set(String::NewFromUtf8(env->isolate(), - result.data(), - v8::NewStringType::kNormal, - result.size()) - .ToLocalChecked()); -} - static void Open(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -2103,6 +2171,35 @@ static void Open(const FunctionCallbackInfo& args) { } } +static void OpenSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + const int argc = args.Length(); + CHECK_GE(argc, 3); + + BufferValue path(env->isolate(), args[0]); + CHECK_NOT_NULL(*path); + + CHECK(args[1]->IsInt32()); + const int flags = args[1].As()->Value(); + + CHECK(args[2]->IsInt32()); + const int mode = args[2].As()->Value(); + + if (CheckOpenPermissions(env, path, flags).IsNothing()) return; + + 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); + FS_SYNC_TRACE_END(open); + if (err < 0) { + return env->ThrowUVException(err, "open", nullptr, path.out()); + } + env->AddUnmanagedFd(err); + args.GetReturnValue().Set(err); +} + static void OpenFileHandle(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); @@ -2185,6 +2282,38 @@ static void CopyFile(const FunctionCallbackInfo& args) { } } +static void CopyFileSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + + const int argc = args.Length(); + CHECK_GE(argc, 3); + + BufferValue src(isolate, args[0]); + CHECK_NOT_NULL(*src); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemRead, src.ToStringView()); + + BufferValue dest(isolate, args[1]); + CHECK_NOT_NULL(*dest); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView()); + + CHECK(args[2]->IsInt32()); + const int flags = args[2].As()->Value(); + + uv_fs_t req; + FS_SYNC_TRACE_BEGIN(copyfile); + int err = + uv_fs_copyfile(nullptr, &req, src.out(), dest.out(), flags, nullptr); + uv_fs_req_cleanup(&req); + FS_SYNC_TRACE_END(copyfile); + + if (err) { + return env->ThrowUVException( + err, "copyfile", nullptr, src.out(), dest.out()); + } +} // Wrapper for write(2). // @@ -2447,6 +2576,58 @@ static void Read(const FunctionCallbackInfo& args) { } } +static void ReadFileUtf8(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + auto isolate = env->isolate(); + + 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_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()); + } + + 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); + }); + std::string result{}; + char buffer[8192]; + uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer)); + + FS_SYNC_TRACE_BEGIN(read); + while (true) { + auto r = uv_fs_read(nullptr, &req, file, &buf, 1, -1, nullptr); + 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()); + } + if (r <= 0) { + break; + } + result.append(buf.base, r); + } + FS_SYNC_TRACE_END(read); + + args.GetReturnValue().Set( + ToV8Value(env->context(), result, isolate).ToLocalChecked()); +} // Wrapper for readv(2). // @@ -2559,7 +2740,6 @@ static void FChmod(const FunctionCallbackInfo& args) { } } - /* fs.chown(path, uid, gid); * Wrapper for chown(1) / EIO_CHOWN */ @@ -3206,10 +3386,15 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, Isolate* isolate = isolate_data->isolate(); SetMethod(isolate, target, "access", Access); + SetMethodNoSideEffect(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); + SetMethod(isolate, target, "closeSync", CloseSync); + SetMethodNoSideEffect(isolate, target, "existsSync", ExistsSync); SetMethod(isolate, target, "open", Open); + SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); SetMethod(isolate, target, "read", Read); + SetMethodNoSideEffect(isolate, target, "readFileUtf8", ReadFileUtf8); SetMethod(isolate, target, "readBuffers", ReadBuffers); SetMethod(isolate, target, "fdatasync", Fdatasync); SetMethod(isolate, target, "fsync", Fsync); @@ -3221,10 +3406,11 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "internalModuleReadJSON", InternalModuleReadJSON); SetMethod(isolate, target, "internalModuleStat", InternalModuleStat); SetMethod(isolate, target, "stat", Stat); + SetMethod(isolate, target, "statSync", StatSync); SetMethod(isolate, target, "lstat", LStat); SetMethod(isolate, target, "fstat", FStat); - SetMethodNoSideEffect(isolate, target, "readFileSync", ReadFileSync); SetMethod(isolate, target, "statfs", StatFs); + SetMethod(isolate, target, "statfsSync", StatFsSync); SetMethod(isolate, target, "link", Link); SetMethod(isolate, target, "symlink", Symlink); SetMethod(isolate, target, "readlink", ReadLink); @@ -3234,6 +3420,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "writeString", WriteString); SetMethod(isolate, target, "realpath", RealPath); SetMethod(isolate, target, "copyFile", CopyFile); + SetMethodNoSideEffect(isolate, target, "copyFileSync", CopyFileSync); SetMethod(isolate, target, "chmod", Chmod); SetMethod(isolate, target, "fchmod", FChmod); @@ -3321,13 +3508,18 @@ BindingData* FSReqBase::binding_data() { void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(Access); + registry->Register(AccessSync); StatWatcher::RegisterExternalReferences(registry); BindingData::RegisterExternalReferences(registry); registry->Register(Close); + registry->Register(CloseSync); + registry->Register(ExistsSync); registry->Register(Open); + registry->Register(OpenSync); registry->Register(OpenFileHandle); registry->Register(Read); + registry->Register(ReadFileUtf8); registry->Register(ReadBuffers); registry->Register(Fdatasync); registry->Register(Fsync); @@ -3339,10 +3531,11 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(InternalModuleReadJSON); registry->Register(InternalModuleStat); registry->Register(Stat); + registry->Register(StatSync); registry->Register(LStat); registry->Register(FStat); - registry->Register(ReadFileSync); registry->Register(StatFs); + registry->Register(StatFsSync); registry->Register(Link); registry->Register(Symlink); registry->Register(ReadLink); @@ -3352,6 +3545,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(WriteString); registry->Register(RealPath); registry->Register(CopyFile); + registry->Register(CopyFileSync); registry->Register(Chmod); registry->Register(FChmod); diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 0343e8422885ad..2e4097eb270263 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -1,4 +1,3 @@ -// Flags: --expose-internals // Copyright Joyent, Inc. and other Node contributors. // // Permission is hereby granted, free of charge, to any person obtaining a @@ -26,7 +25,6 @@ const common = require('../common'); const assert = require('assert'); const { inspect } = require('util'); const vm = require('vm'); -const { internalBinding } = require('internal/test/binding'); const a = assert; // Disable colored output to prevent color codes from breaking assertion @@ -802,37 +800,6 @@ assert.throws( } ); -{ - // Test caching. - const fs = internalBinding('fs'); - const tmp = fs.close; - fs.close = common.mustCall(tmp, 1); - function throwErr() { - assert( - (Buffer.from('test') instanceof Error) - ); - } - assert.throws( - () => throwErr(), - { - code: 'ERR_ASSERTION', - constructor: assert.AssertionError, - message: 'The expression evaluated to a falsy value:\n\n ' + - "assert(\n (Buffer.from('test') instanceof Error)\n )\n" - } - ); - assert.throws( - () => throwErr(), - { - code: 'ERR_ASSERTION', - constructor: assert.AssertionError, - message: 'The expression evaluated to a falsy value:\n\n ' + - "assert(\n (Buffer.from('test') instanceof Error)\n )\n" - } - ); - fs.close = tmp; -} - assert.throws( () => { a( diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 9abe2dee22c1c7..8e9f6fa0f1535d 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -74,7 +74,7 @@ const expectedModules = new Set([ 'NativeModule internal/webstreams/queuingstrategies', 'NativeModule internal/blob', 'NativeModule internal/fs/utils', - 'NativeModule internal/fs/read/utf8', + 'NativeModule internal/fs/sync', 'NativeModule fs', 'Internal Binding options', 'NativeModule internal/options',