From bc2c0410d34c9a56b1b9c1a38ef47ae0871f9a06 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 30 Sep 2023 03:17:00 +0200 Subject: [PATCH] fs: throw errors from sync branches instead of separate implementations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously to throw errors from C++ land, sync versions of the fs were created by copying C++ code from the original implementation and moving JS code to a separate file. This can lead to several problems: 1. By moving code to a new file for the sake of moving, it would be harder to use git blame to trace changes and harder to backport changes to older branches. 2. Scattering the async and sync versions of fs methods in different files makes it harder to keep them in sync and share code in the prologues and epilogues. 3. Having two copies of code doing almost the same thing results in duplication and can be prone to out-of-sync problems when the prologue and epilogue get updated. 4. There is a minor cost to startup in adding an additional file. This can add up even with the help of snapshots. This patch moves the JS code back to lib/fs.js to stop 1, 2 & 4 and introduces C++ helpers SyncCallAndThrowIf() and SyncCallAndThrowOnError() so that the original implementations can be easily tweaked to allow throwing from C++ and stop 3. PR-URL: https://github.com/nodejs/node/pull/49913 Reviewed-By: Stephen Belanger Reviewed-By: Colin Ihrig Reviewed-By: Darshan Sen Reviewed-By: Benjamin Gruenbaum Reviewed-By: Tobias Nießen Reviewed-By: Yagiz Nizipli --- lib/fs.js | 66 ++++- lib/internal/fs/sync.js | 106 -------- src/node_file-inl.h | 32 +++ src/node_file.cc | 323 +++++------------------- src/node_file.h | 29 ++- test/parallel/test-bootstrap-modules.js | 1 - 6 files changed, 173 insertions(+), 384 deletions(-) delete mode 100644 lib/internal/fs/sync.js diff --git a/lib/fs.js b/lib/fs.js index 29f356a57cd22e..86895cf84170d5 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -141,7 +141,6 @@ const { validateObject, validateString, } = require('internal/validators'); -const syncFs = require('internal/fs/sync'); let truncateWarn = true; let fs; @@ -243,7 +242,10 @@ function access(path, mode, callback) { * @returns {void} */ function accessSync(path, mode) { - syncFs.access(path, mode); + path = getValidatedPath(path); + mode = getValidMode(mode, 'access'); + + binding.access(pathModule.toNamespacedPath(path), mode); } /** @@ -285,7 +287,13 @@ ObjectDefineProperty(exists, kCustomPromisifiedSymbol, { * @returns {boolean} */ function existsSync(path) { - return syncFs.exists(path); + try { + path = getValidatedPath(path); + } catch { + return false; + } + + return binding.existsSync(pathModule.toNamespacedPath(path)); } function readFileAfterOpen(err, fd) { @@ -438,7 +446,10 @@ function readFileSync(path, options) { options = getOptions(options, { flag: 'r' }); if (options.encoding === 'utf8' || options.encoding === 'utf-8') { - return syncFs.readFileUtf8(path, options.flag); + if (!isInt32(path)) { + path = pathModule.toNamespacedPath(getValidatedPath(path)); + } + return binding.readFileUtf8(path, stringToFlags(options.flag)); } const isUserFd = isFd(path); // File descriptor ownership @@ -516,7 +527,9 @@ function close(fd, callback = defaultCloseCallback) { * @returns {void} */ function closeSync(fd) { - return syncFs.close(fd); + fd = getValidatedFd(fd); + + return binding.close(fd); } /** @@ -562,7 +575,13 @@ function open(path, flags, mode, callback) { * @returns {number} */ function openSync(path, flags, mode) { - return syncFs.open(path, flags, mode); + path = getValidatedPath(path); + + return binding.open( + pathModule.toNamespacedPath(path), + stringToFlags(flags), + parseFileMode(mode, 'mode', 0o666), + ); } /** @@ -1665,12 +1684,24 @@ function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) { * }} [options] * @returns {Stats} */ -function statSync(path, options) { - return syncFs.stat(path, options); +function statSync(path, options = { bigint: false, throwIfNoEntry: true }) { + path = getValidatedPath(path); + const stats = binding.stat( + pathModule.toNamespacedPath(path), + options.bigint, + undefined, + options.throwIfNoEntry, + ); + if (stats === undefined) { + return undefined; + } + return getStatsFromBinding(stats); } -function statfsSync(path, options) { - return syncFs.statfs(path, options); +function statfsSync(path, options = { bigint: false }) { + path = getValidatedPath(path); + const stats = binding.statfs(pathModule.toNamespacedPath(path), options.bigint); + return getStatFsFromBinding(stats); } /** @@ -1850,7 +1881,8 @@ function unlink(path, callback) { * @returns {void} */ function unlinkSync(path) { - return syncFs.unlink(path); + path = pathModule.toNamespacedPath(getValidatedPath(path)); + return binding.unlink(path); } /** @@ -2650,8 +2682,7 @@ function realpathSync(p, options) { } if (linkTarget === null) { const ctx = { path: base }; - binding.stat(baseLong, false, undefined, ctx); - handleErrorFromBinding(ctx); + binding.stat(baseLong, false, undefined, true); linkTarget = binding.readlink(baseLong, undefined, undefined, ctx); handleErrorFromBinding(ctx); } @@ -2946,7 +2977,14 @@ function copyFile(src, dest, mode, callback) { * @returns {void} */ function copyFileSync(src, dest, mode) { - syncFs.copyFile(src, dest, mode); + src = getValidatedPath(src, 'src'); + dest = getValidatedPath(dest, 'dest'); + + binding.copyFile( + pathModule.toNamespacedPath(src), + pathModule.toNamespacedPath(dest), + getValidMode(mode, 'copyFile'), + ); } /** diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js deleted file mode 100644 index fbcc2ad2e25b2a..00000000000000 --- a/lib/internal/fs/sync.js +++ /dev/null @@ -1,106 +0,0 @@ -'use strict'; - -const pathModule = require('path'); -const { - getValidatedPath, - stringToFlags, - getValidMode, - getStatsFromBinding, - getStatFsFromBinding, - getValidatedFd, -} = require('internal/fs/utils'); -const { parseFileMode, isInt32 } = require('internal/validators'); - -const binding = internalBinding('fs'); - -/** - * @param {string} path - * @param {number} flag - * @return {string} - */ -function readFileUtf8(path, flag) { - if (!isInt32(path)) { - 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); -} - -function unlink(path) { - path = pathModule.toNamespacedPath(getValidatedPath(path)); - return binding.unlinkSync(path); -} - -module.exports = { - readFileUtf8, - exists, - access, - copyFile, - stat, - statfs, - open, - close, - unlink, -}; diff --git a/src/node_file-inl.h b/src/node_file-inl.h index cdf21a4b3a6c22..6c059add3bfc02 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -349,6 +349,38 @@ int SyncCall(Environment* env, v8::Local ctx, return err; } +// Similar to SyncCall but throws immediately if there is an error. +template +int SyncCallAndThrowIf(Predicate should_throw, + Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args) { + env->PrintSyncTrace(); + int result = fn(nullptr, &(req_wrap->req), args..., nullptr); + if (should_throw(result)) { + env->ThrowUVException(result, + req_wrap->syscall_p, + nullptr, + req_wrap->path_p, + req_wrap->dest_p); + } + return result; +} + +constexpr bool is_uv_error(int result) { + return result < 0; +} + +// Similar to SyncCall but throws immediately if there is an error. +template +int SyncCallAndThrowOnError(Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args) { + return SyncCallAndThrowIf(is_uv_error, env, req_wrap, fn, args...); +} + } // namespace fs } // namespace node diff --git a/src/node_file.cc b/src/node_file.cc index 8284ab744d829c..b31dcc168486d8 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -981,90 +981,45 @@ void Access(const FunctionCallbackInfo& args) { THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); - FSReqBase* req_wrap_async = GetReqWrap(args, 2); - if (req_wrap_async != nullptr) { // access(path, mode, req) + if (argc > 2) { // access(path, mode, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_ACCESS, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "access", UTF8, AfterNoArgs, uv_fs_access, *path, mode); - } else { // access(path, mode, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // access(path, mode) + FSReqWrapSync req_wrap_sync("access", *path); FS_SYNC_TRACE_BEGIN(access); - SyncCall(env, args[3], &req_wrap_sync, "access", uv_fs_access, *path, mode); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_access, *path, mode); FS_SYNC_TRACE_END(access); } } -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); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 1); CHECK(args[0]->IsInt32()); int fd = args[0].As()->Value(); env->RemoveUnmanagedFd(fd); - FSReqBase* req_wrap_async = GetReqWrap(args, 1); - if (req_wrap_async != nullptr) { // close(fd, req) + if (argc > 1) { // close(fd, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 1); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async) AsyncCall(env, req_wrap_async, args, "close", UTF8, AfterNoArgs, uv_fs_close, fd); - } else { // close(fd, undefined, ctx) - CHECK_EQ(argc, 3); - FSReqWrapSync req_wrap_sync; + } else { // close(fd) + FSReqWrapSync req_wrap_sync("close"); FS_SYNC_TRACE_BEGIN(close); - SyncCall(env, args[2], &req_wrap_sync, "close", uv_fs_close, fd); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd); FS_SYNC_TRACE_END(close); } } -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(); @@ -1214,13 +1169,17 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(rc); } +constexpr bool is_uv_error_except_no_entry(int result) { + return result < 0 && result != UV_ENOENT; +} + static void Stat(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); Environment* env = realm->env(); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 3); BufferValue path(realm->isolate(), args[0]); CHECK_NOT_NULL(*path); @@ -1228,63 +1187,34 @@ static void Stat(const FunctionCallbackInfo& args) { env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); bool use_bigint = args[1]->IsTrue(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); - if (req_wrap_async != nullptr) { // stat(path, use_bigint, req) + if (!args[2]->IsUndefined()) { // stat(path, use_bigint, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_STAT, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "stat", UTF8, AfterStat, uv_fs_stat, *path); - } else { // stat(path, use_bigint, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // stat(path, use_bigint, undefined, do_not_throw_if_no_entry) + bool do_not_throw_if_no_entry = args[3]->IsFalse(); + FSReqWrapSync req_wrap_sync("stat", *path); FS_SYNC_TRACE_BEGIN(stat); - int err = SyncCall(env, args[3], &req_wrap_sync, "stat", uv_fs_stat, *path); + int result; + if (do_not_throw_if_no_entry) { + result = SyncCallAndThrowIf( + is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_stat, *path); + } else { + result = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_stat, *path); + } FS_SYNC_TRACE_END(stat); - if (err != 0) { - return; // error info is in ctx + if (is_uv_error(result)) { + return; } - Local arr = FillGlobalStatsArray(binding_data, use_bigint, static_cast(req_wrap_sync.req.ptr)); args.GetReturnValue().Set(arr); } } -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(); @@ -1367,8 +1297,9 @@ static void StatFs(const FunctionCallbackInfo& args) { env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); bool use_bigint = args[1]->IsTrue(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); - if (req_wrap_async != nullptr) { // statfs(path, use_bigint, req) + if (argc > 2) { // statfs(path, use_bigint, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_STATFS, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, @@ -1379,15 +1310,14 @@ static void StatFs(const FunctionCallbackInfo& args) { AfterStatFs, uv_fs_statfs, *path); - } else { // statfs(path, use_bigint, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // statfs(path, use_bigint) + FSReqWrapSync req_wrap_sync("statfs", *path); FS_SYNC_TRACE_BEGIN(statfs); - int err = - SyncCall(env, args[3], &req_wrap_sync, "statfs", uv_fs_statfs, *path); + int result = + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_statfs, *path); FS_SYNC_TRACE_END(statfs); - if (err != 0) { - return; // error info is in ctx + if (is_uv_error(result)) { + return; } Local arr = FillGlobalStatFsArray( @@ -1398,34 +1328,6 @@ 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(); @@ -1680,49 +1582,28 @@ static void Unlink(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 1); BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); - FSReqBase* req_wrap_async = GetReqWrap(args, 1); - if (req_wrap_async != nullptr) { + if (argc > 1) { // unlink(path, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 1); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_UNLINK, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "unlink", UTF8, AfterNoArgs, uv_fs_unlink, *path); - } else { - CHECK_EQ(argc, 3); - FSReqWrapSync req_wrap_sync; + } else { // unlink(path) + FSReqWrapSync req_wrap_sync("unlink", *path); FS_SYNC_TRACE_BEGIN(unlink); - SyncCall(env, args[2], &req_wrap_sync, "unlink", uv_fs_unlink, *path); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_unlink, *path); FS_SYNC_TRACE_END(unlink); } } -static void UnlinkSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - const int argc = args.Length(); - CHECK_GE(argc, 1); - - BufferValue path(env->isolate(), args[0]); - CHECK_NOT_NULL(*path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); - - uv_fs_t req; - auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - FS_SYNC_TRACE_BEGIN(unlink); - int err = uv_fs_unlink(nullptr, &req, *path, nullptr); - FS_SYNC_TRACE_END(unlink); - if (err < 0) { - return env->ThrowUVException(err, "unlink", nullptr, *path); - } -} - static void RMDir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -2172,54 +2053,26 @@ static void Open(const FunctionCallbackInfo& args) { if (CheckOpenPermissions(env, path, flags).IsNothing()) return; - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // open(path, flags, mode, req) + if (argc > 3) { // open(path, flags, mode, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); + CHECK_NOT_NULL(req_wrap_async); req_wrap_async->set_is_plain_open(true); FS_ASYNC_TRACE_BEGIN1( UV_FS_OPEN, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "open", UTF8, AfterInteger, uv_fs_open, *path, flags, mode); - } else { // open(path, flags, mode, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // open(path, flags, mode) + FSReqWrapSync req_wrap_sync("open", *path); FS_SYNC_TRACE_BEGIN(open); - int result = SyncCall(env, args[4], &req_wrap_sync, "open", - uv_fs_open, *path, flags, mode); + int result = SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_open, *path, flags, mode); FS_SYNC_TRACE_END(open); - if (result >= 0) env->AddUnmanagedFd(result); + if (is_uv_error(result)) return; + env->AddUnmanagedFd(result); args.GetReturnValue().Set(result); } } -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); - 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()); - } - env->AddUnmanagedFd(err); - args.GetReturnValue().Set(err); -} - static void OpenFileHandle(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); @@ -2281,8 +2134,8 @@ static void CopyFile(const FunctionCallbackInfo& args) { CHECK(args[2]->IsInt32()); const int flags = args[2].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // copyFile(src, dest, flags, req) + if (argc > 3) { // copyFile(src, dest, flags, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN2(UV_FS_COPYFILE, req_wrap_async, "src", @@ -2292,49 +2145,15 @@ static void CopyFile(const FunctionCallbackInfo& args) { AsyncDestCall(env, req_wrap_async, args, "copyfile", *dest, dest.length(), UTF8, AfterNoArgs, uv_fs_copyfile, *src, *dest, flags); - } else { // copyFile(src, dest, flags, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // copyFile(src, dest, flags) + FSReqWrapSync req_wrap_sync("copyfile", *src, *dest); FS_SYNC_TRACE_BEGIN(copyfile); - SyncCall(env, args[4], &req_wrap_sync, "copyfile", - uv_fs_copyfile, *src, *dest, flags); + SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_copyfile, *src, *dest, flags); FS_SYNC_TRACE_END(copyfile); } } -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). // // bytesWritten = write(fd, buffer, offset, length, position, callback) @@ -3426,12 +3245,9 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, Isolate* isolate = isolate_data->isolate(); SetMethod(isolate, target, "access", Access); - SetMethod(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); - SetMethod(isolate, target, "closeSync", CloseSync); SetMethod(isolate, target, "existsSync", ExistsSync); SetMethod(isolate, target, "open", Open); - SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); SetMethod(isolate, target, "read", Read); SetMethod(isolate, target, "readFileUtf8", ReadFileUtf8); @@ -3446,22 +3262,18 @@ 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); 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); SetMethod(isolate, target, "unlink", Unlink); - SetMethod(isolate, target, "unlinkSync", UnlinkSync); SetMethod(isolate, target, "writeBuffer", WriteBuffer); SetMethod(isolate, target, "writeBuffers", WriteBuffers); SetMethod(isolate, target, "writeString", WriteString); SetMethod(isolate, target, "realpath", RealPath); SetMethod(isolate, target, "copyFile", CopyFile); - SetMethod(isolate, target, "copyFileSync", CopyFileSync); SetMethod(isolate, target, "chmod", Chmod); SetMethod(isolate, target, "fchmod", FChmod); @@ -3549,15 +3361,12 @@ 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); @@ -3572,22 +3381,18 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(InternalModuleReadJSON); registry->Register(InternalModuleStat); registry->Register(Stat); - registry->Register(StatSync); registry->Register(LStat); registry->Register(FStat); registry->Register(StatFs); - registry->Register(StatFsSync); registry->Register(Link); registry->Register(Symlink); registry->Register(ReadLink); registry->Register(Unlink); - registry->Register(UnlinkSync); registry->Register(WriteBuffer); registry->Register(WriteBuffers); registry->Register(WriteString); registry->Register(RealPath); registry->Register(CopyFile); - registry->Register(CopyFileSync); registry->Register(Chmod); registry->Register(FChmod); diff --git a/src/node_file.h b/src/node_file.h index dad50996f1b003..6f1b55284db0f4 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -457,10 +457,22 @@ int MKDirpSync(uv_loop_t* loop, class FSReqWrapSync { public: - FSReqWrapSync() = default; + FSReqWrapSync(const char* syscall = nullptr, + const char* path = nullptr, + const char* dest = nullptr) + : syscall_p(syscall), path_p(path), dest_p(dest) {} ~FSReqWrapSync() { uv_fs_req_cleanup(&req); } + uv_fs_t req; + const char* syscall_p; + const char* path_p; + const char* dest_p; + + FSReqWrapSync(const FSReqWrapSync&) = delete; + FSReqWrapSync& operator=(const FSReqWrapSync&) = delete; + // TODO(joyeecheung): move these out of FSReqWrapSync and into a special + // class for mkdirp FSContinuationData* continuation_data() const { return continuation_data_.get(); } @@ -468,9 +480,6 @@ class FSReqWrapSync { continuation_data_ = std::move(data); } - FSReqWrapSync(const FSReqWrapSync&) = delete; - FSReqWrapSync& operator=(const FSReqWrapSync&) = delete; - private: std::unique_ptr continuation_data_; }; @@ -507,6 +516,18 @@ inline int SyncCall(Environment* env, v8::Local ctx, FSReqWrapSync* req_wrap, const char* syscall, Func fn, Args... args); +// Similar to SyncCall but throws immediately if there is an error. +template +int SyncCallAndThrowIf(Predicate should_throw, + Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args); +template +int SyncCallAndThrowOnError(Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args); } // namespace fs } // namespace node diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 8e9f6fa0f1535d..78db466a95b38e 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -74,7 +74,6 @@ const expectedModules = new Set([ 'NativeModule internal/webstreams/queuingstrategies', 'NativeModule internal/blob', 'NativeModule internal/fs/utils', - 'NativeModule internal/fs/sync', 'NativeModule fs', 'Internal Binding options', 'NativeModule internal/options',