From 9c334ffbd63ed7ebd069fd82ff0b08065df7344c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Sep 2023 02:10:31 +0200 Subject: [PATCH] fs: throw errors from sync branches instead of separate implementations 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. --- lib/fs.js | 66 +++-- lib/internal/fs/sync.js | 106 -------- src/node_file-inl.h | 32 +++ src/node_file.cc | 316 +++++------------------- src/node_file.h | 29 ++- test/parallel/test-bootstrap-modules.js | 1 - 6 files changed, 169 insertions(+), 381 deletions(-) delete mode 100644 lib/internal/fs/sync.js diff --git a/lib/fs.js b/lib/fs.js index 29f356a57cd22e..c920934e6ba1ad 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, false); 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..ab49729aa46ec9 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(env->event_loop(), &(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 aca7ec82101a60..cf0a3855049d2c 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 == 3) { // 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 (args.Length() == 2) { // 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(); @@ -1179,6 +1134,10 @@ 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(); @@ -1193,63 +1152,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 (argc == 3) { // 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 (result < 0) { + 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(); @@ -1344,15 +1274,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 (result < 0) { + return; } Local arr = FillGlobalStatFsArray( @@ -1363,34 +1292,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(); @@ -1645,49 +1546,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 == 2) { // 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); @@ -2137,54 +2017,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 == 4) { // 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 (result < 0) 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(); @@ -2246,8 +2098,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 == 4) { // copyFile(src, dest, flags, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN2(UV_FS_COPYFILE, req_wrap_async, "src", @@ -2257,49 +2109,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) @@ -3382,12 +3200,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); @@ -3402,22 +3217,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); @@ -3505,15 +3316,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); @@ -3528,22 +3336,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 0fa2da0b4f7f8a..31d0e0803fd85c 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -465,10 +465,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(); } @@ -476,9 +488,6 @@ class FSReqWrapSync { continuation_data_ = std::move(data); } - FSReqWrapSync(const FSReqWrapSync&) = delete; - FSReqWrapSync& operator=(const FSReqWrapSync&) = delete; - private: std::unique_ptr continuation_data_; }; @@ -515,6 +524,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',