Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 813713f

Browse filesBrowse files
joyeecheungjoyeecheung
authored
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. 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>
1 parent d37b0d2 commit 813713f
Copy full SHA for 813713f

File tree

Expand file treeCollapse file tree

6 files changed

+173
-384
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+173
-384
lines changed
Open diff view settings
Collapse file

‎lib/fs.js‎

Copy file name to clipboardExpand all lines: lib/fs.js
+52-14Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,6 @@ const {
141141
validateObject,
142142
validateString,
143143
} = require('internal/validators');
144-
const syncFs = require('internal/fs/sync');
145144

146145
let truncateWarn = true;
147146
let fs;
@@ -243,7 +242,10 @@ function access(path, mode, callback) {
243242
* @returns {void}
244243
*/
245244
function accessSync(path, mode) {
246-
syncFs.access(path, mode);
245+
path = getValidatedPath(path);
246+
mode = getValidMode(mode, 'access');
247+
248+
binding.access(pathModule.toNamespacedPath(path), mode);
247249
}
248250

249251
/**
@@ -285,7 +287,13 @@ ObjectDefineProperty(exists, kCustomPromisifiedSymbol, {
285287
* @returns {boolean}
286288
*/
287289
function existsSync(path) {
288-
return syncFs.exists(path);
290+
try {
291+
path = getValidatedPath(path);
292+
} catch {
293+
return false;
294+
}
295+
296+
return binding.existsSync(pathModule.toNamespacedPath(path));
289297
}
290298

291299
function readFileAfterOpen(err, fd) {
@@ -438,7 +446,10 @@ function readFileSync(path, options) {
438446
options = getOptions(options, { flag: 'r' });
439447

440448
if (options.encoding === 'utf8' || options.encoding === 'utf-8') {
441-
return syncFs.readFileUtf8(path, options.flag);
449+
if (!isInt32(path)) {
450+
path = pathModule.toNamespacedPath(getValidatedPath(path));
451+
}
452+
return binding.readFileUtf8(path, stringToFlags(options.flag));
442453
}
443454

444455
const isUserFd = isFd(path); // File descriptor ownership
@@ -516,7 +527,9 @@ function close(fd, callback = defaultCloseCallback) {
516527
* @returns {void}
517528
*/
518529
function closeSync(fd) {
519-
return syncFs.close(fd);
530+
fd = getValidatedFd(fd);
531+
532+
return binding.close(fd);
520533
}
521534

522535
/**
@@ -562,7 +575,13 @@ function open(path, flags, mode, callback) {
562575
* @returns {number}
563576
*/
564577
function openSync(path, flags, mode) {
565-
return syncFs.open(path, flags, mode);
578+
path = getValidatedPath(path);
579+
580+
return binding.open(
581+
pathModule.toNamespacedPath(path),
582+
stringToFlags(flags),
583+
parseFileMode(mode, 'mode', 0o666),
584+
);
566585
}
567586

568587
/**
@@ -1667,12 +1686,24 @@ function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
16671686
* }} [options]
16681687
* @returns {Stats}
16691688
*/
1670-
function statSync(path, options) {
1671-
return syncFs.stat(path, options);
1689+
function statSync(path, options = { bigint: false, throwIfNoEntry: true }) {
1690+
path = getValidatedPath(path);
1691+
const stats = binding.stat(
1692+
pathModule.toNamespacedPath(path),
1693+
options.bigint,
1694+
undefined,
1695+
options.throwIfNoEntry,
1696+
);
1697+
if (stats === undefined) {
1698+
return undefined;
1699+
}
1700+
return getStatsFromBinding(stats);
16721701
}
16731702

1674-
function statfsSync(path, options) {
1675-
return syncFs.statfs(path, options);
1703+
function statfsSync(path, options = { bigint: false }) {
1704+
path = getValidatedPath(path);
1705+
const stats = binding.statfs(pathModule.toNamespacedPath(path), options.bigint);
1706+
return getStatFsFromBinding(stats);
16761707
}
16771708

16781709
/**
@@ -1852,7 +1883,8 @@ function unlink(path, callback) {
18521883
* @returns {void}
18531884
*/
18541885
function unlinkSync(path) {
1855-
return syncFs.unlink(path);
1886+
path = pathModule.toNamespacedPath(getValidatedPath(path));
1887+
return binding.unlink(path);
18561888
}
18571889

18581890
/**
@@ -2652,8 +2684,7 @@ function realpathSync(p, options) {
26522684
}
26532685
if (linkTarget === null) {
26542686
const ctx = { path: base };
2655-
binding.stat(baseLong, false, undefined, ctx);
2656-
handleErrorFromBinding(ctx);
2687+
binding.stat(baseLong, false, undefined, true);
26572688
linkTarget = binding.readlink(baseLong, undefined, undefined, ctx);
26582689
handleErrorFromBinding(ctx);
26592690
}
@@ -2948,7 +2979,14 @@ function copyFile(src, dest, mode, callback) {
29482979
* @returns {void}
29492980
*/
29502981
function copyFileSync(src, dest, mode) {
2951-
syncFs.copyFile(src, dest, mode);
2982+
src = getValidatedPath(src, 'src');
2983+
dest = getValidatedPath(dest, 'dest');
2984+
2985+
binding.copyFile(
2986+
pathModule.toNamespacedPath(src),
2987+
pathModule.toNamespacedPath(dest),
2988+
getValidMode(mode, 'copyFile'),
2989+
);
29522990
}
29532991

29542992
/**
Collapse file

‎lib/internal/fs/sync.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/sync.js
-106Lines changed: 0 additions & 106 deletions
This file was deleted.
Collapse file

‎src/node_file-inl.h‎

Copy file name to clipboardExpand all lines: src/node_file-inl.h
+32Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,38 @@ int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
349349
return err;
350350
}
351351

352+
// Similar to SyncCall but throws immediately if there is an error.
353+
template <typename Predicate, typename Func, typename... Args>
354+
int SyncCallAndThrowIf(Predicate should_throw,
355+
Environment* env,
356+
FSReqWrapSync* req_wrap,
357+
Func fn,
358+
Args... args) {
359+
env->PrintSyncTrace();
360+
int result = fn(nullptr, &(req_wrap->req), args..., nullptr);
361+
if (should_throw(result)) {
362+
env->ThrowUVException(result,
363+
req_wrap->syscall_p,
364+
nullptr,
365+
req_wrap->path_p,
366+
req_wrap->dest_p);
367+
}
368+
return result;
369+
}
370+
371+
constexpr bool is_uv_error(int result) {
372+
return result < 0;
373+
}
374+
375+
// Similar to SyncCall but throws immediately if there is an error.
376+
template <typename Func, typename... Args>
377+
int SyncCallAndThrowOnError(Environment* env,
378+
FSReqWrapSync* req_wrap,
379+
Func fn,
380+
Args... args) {
381+
return SyncCallAndThrowIf(is_uv_error, env, req_wrap, fn, args...);
382+
}
383+
352384
} // namespace fs
353385
} // namespace node
354386

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.