Skip to content

Commit

Permalink
fs: throw errors from sync branches instead of separate implementations
Browse files Browse the repository at this point in the history
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: #49913
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
  • Loading branch information
joyeecheung authored and targos committed Nov 11, 2023
1 parent b86e1f5 commit bc2c041
Show file tree
Hide file tree
Showing 6 changed files with 173 additions and 384 deletions.
66 changes: 52 additions & 14 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ const {
validateObject,
validateString,
} = require('internal/validators');
const syncFs = require('internal/fs/sync');

let truncateWarn = true;
let fs;
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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),
);
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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'),
);
}

/**
Expand Down
106 changes: 0 additions & 106 deletions lib/internal/fs/sync.js

This file was deleted.

32 changes: 32 additions & 0 deletions src/node_file-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,38 @@ int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
return err;
}

// Similar to SyncCall but throws immediately if there is an error.
template <typename Predicate, typename Func, typename... Args>
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 <typename Func, typename... Args>
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

Expand Down
Loading

0 comments on commit bc2c041

Please sign in to comment.