Skip to content

Commit

Permalink
fs: improve error perf of sync lstat+fstat
Browse files Browse the repository at this point in the history 
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@CanadaHonk @UlisesGascon
CanadaHonk authored and UlisesGascon committed Jan 9, 2024
1 parent 0be84e5 commit 05e25e0
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 77 deletions.
22 changes: 15 additions & 7 deletions benchmark/fs/bench-statSync-failure.js
View file
Expand Up @@ -5,21 +5,29 @@ const fs = require('fs');
const path = require('path');

const bench = common.createBenchmark(main, {
n: [1e6],
statSyncType: ['throw', 'noThrow'],
n: [1e4],
statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
throwType: [ 'throw', 'noThrow' ],
}, {
// fstatSync does not support throwIfNoEntry
combinationFilter: ({ statSyncType, throwType }) => !(statSyncType === 'fstatSync' && throwType === 'noThrow'),
});


function main({ n, statSyncType }) {
const arg = path.join(__dirname, 'non.existent');
function main({ n, statSyncType, throwType }) {
const arg = (statSyncType === 'fstatSync' ?
(1 << 30) :
path.join(__dirname, 'non.existent'));

const fn = fs[statSyncType];

bench.start();
for (let i = 0; i < n; i++) {
if (statSyncType === 'noThrow') {
fs.statSync(arg, { throwIfNoEntry: false });
if (throwType === 'noThrow') {
fn(arg, { throwIfNoEntry: false });
} else {
try {
fs.statSync(arg);
fn(arg);
} catch {
// Continue regardless of error.
}
Expand Down
2 changes: 1 addition & 1 deletion benchmark/fs/bench-statSync.js
View file
Expand Up @@ -4,7 +4,7 @@ const common = require('../common');
const fs = require('fs');

const bench = common.createBenchmark(main, {
n: [1e6],
n: [1e4],
statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
});

Expand Down
68 changes: 29 additions & 39 deletions lib/fs.js
View file
Expand Up @@ -74,8 +74,6 @@ const {
ERR_INVALID_ARG_VALUE,
},
AbortError,
uvErrmapGet,
uvException,
} = require('internal/errors');

const {
Expand Down Expand Up @@ -402,11 +400,9 @@ function readFile(path, options, callback) {
}

function tryStatSync(fd, isUserFd) {
const ctx = {};
const stats = binding.fstat(fd, false, undefined, ctx);
if (ctx.errno !== undefined && !isUserFd) {
const stats = binding.fstat(fd, false, undefined, true /* shouldNotThrow */);
if (stats === undefined && !isUserFd) {
fs.closeSync(fd);
throw uvException(ctx);
}
return stats;
}
Expand Down Expand Up @@ -1614,33 +1610,21 @@ function statfs(path, options = { bigint: false }, callback) {
binding.statfs(pathModule.toNamespacedPath(path), options.bigint, req);
}

function hasNoEntryError(ctx) {
if (ctx.errno) {
const uvErr = uvErrmapGet(ctx.errno);
return uvErr?.[0] === 'ENOENT';
}

if (ctx.error) {
return ctx.error.code === 'ENOENT';
}

return false;
}

/**
* Synchronously retrieves the `fs.Stats` for
* the file descriptor.
* @param {number} fd
* @param {{
* bigint?: boolean;
* }} [options]
* @returns {Stats}
* @returns {Stats | undefined}
*/
function fstatSync(fd, options = { bigint: false }) {
fd = getValidatedFd(fd);
const ctx = { fd };
const stats = binding.fstat(fd, options.bigint, undefined, ctx);
handleErrorFromBinding(ctx);
const stats = binding.fstat(fd, options.bigint, undefined, false);
if (stats === undefined) {
return;
}
return getStatsFromBinding(stats);
}

Expand All @@ -1652,17 +1636,20 @@ function fstatSync(fd, options = { bigint: false }) {
* bigint?: boolean;
* throwIfNoEntry?: boolean;
* }} [options]
* @returns {Stats}
* @returns {Stats | undefined}
*/
function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
path = getValidatedPath(path);
const ctx = { path };
const stats = binding.lstat(pathModule.toNamespacedPath(path),
options.bigint, undefined, ctx);
if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
return undefined;
const stats = binding.lstat(
pathModule.toNamespacedPath(path),
options.bigint,
undefined,
options.throwIfNoEntry,
);

if (stats === undefined) {
return;
}
handleErrorFromBinding(ctx);
return getStatsFromBinding(stats);
}

Expand Down Expand Up @@ -2665,9 +2652,10 @@ function realpathSync(p, options) {

// On windows, check that the root exists. On unix there is no need.
if (isWindows) {
const ctx = { path: base };
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
handleErrorFromBinding(ctx);
const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
if (out === undefined) {
return;
}
knownHard.add(base);
}

Expand Down Expand Up @@ -2707,9 +2695,10 @@ function realpathSync(p, options) {
// for our internal use.

const baseLong = pathModule.toNamespacedPath(base);
const ctx = { path: base };
const stats = binding.lstat(baseLong, true, undefined, ctx);
handleErrorFromBinding(ctx);
const stats = binding.lstat(baseLong, true, undefined, true /* throwIfNoEntry */);
if (stats === undefined) {
return;
}

if (!isFileType(stats, S_IFLNK)) {
knownHard.add(base);
Expand Down Expand Up @@ -2748,9 +2737,10 @@ function realpathSync(p, options) {

// On windows, check that the root exists. On unix there is no need.
if (isWindows && !knownHard.has(base)) {
const ctx = { path: base };
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
handleErrorFromBinding(ctx);
const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
if (out === undefined) {
return;
}
knownHard.add(base);
}
}
Expand Down
43 changes: 26 additions & 17 deletions src/node_file.cc
View file
Expand Up @@ -1215,21 +1215,26 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
CHECK_NOT_NULL(*path);

bool use_bigint = args[1]->IsTrue();
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
if (req_wrap_async != nullptr) { // lstat(path, use_bigint, req)
if (!args[2]->IsUndefined()) { // lstat(path, use_bigint, req)
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
FS_ASYNC_TRACE_BEGIN1(
UV_FS_LSTAT, req_wrap_async, "path", TRACE_STR_COPY(*path))
AsyncCall(env, req_wrap_async, args, "lstat", UTF8, AfterStat,
uv_fs_lstat, *path);
} else { // lstat(path, use_bigint, undefined, ctx)
CHECK_EQ(argc, 4);
FSReqWrapSync req_wrap_sync;
} else { // lstat(path, use_bigint, undefined, throw_if_no_entry)
bool do_not_throw_if_no_entry = args[3]->IsFalse();
FSReqWrapSync req_wrap_sync("lstat", *path);
FS_SYNC_TRACE_BEGIN(lstat);
int err = SyncCall(env, args[3], &req_wrap_sync, "lstat", uv_fs_lstat,
*path);
int result;
if (do_not_throw_if_no_entry) {
result = SyncCallAndThrowIf(
is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_lstat, *path);
} else {
result = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_lstat, *path);
}
FS_SYNC_TRACE_END(lstat);
if (err != 0) {
return; // error info is in ctx
if (is_uv_error(result)) {
return;
}

Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
Expand All @@ -1250,19 +1255,23 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
int fd = args[0].As<Int32>()->Value();

bool use_bigint = args[1]->IsTrue();
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
if (req_wrap_async != nullptr) { // fstat(fd, use_bigint, req)
if (!args[2]->IsUndefined()) { // fstat(fd, use_bigint, req)
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
FS_ASYNC_TRACE_BEGIN0(UV_FS_FSTAT, req_wrap_async)
AsyncCall(env, req_wrap_async, args, "fstat", UTF8, AfterStat,
uv_fs_fstat, fd);
} else { // fstat(fd, use_bigint, undefined, ctx)
CHECK_EQ(argc, 4);
FSReqWrapSync req_wrap_sync;
} else { // fstat(fd, use_bigint, undefined, do_not_throw_error)
bool do_not_throw_error = args[2]->IsTrue();
const auto should_throw = [do_not_throw_error](int result) {
return is_uv_error(result) && !do_not_throw_error;
};
FSReqWrapSync req_wrap_sync("fstat");
FS_SYNC_TRACE_BEGIN(fstat);
int err = SyncCall(env, args[3], &req_wrap_sync, "fstat", uv_fs_fstat, fd);
int err =
SyncCallAndThrowIf(should_throw, env, &req_wrap_sync, uv_fs_fstat, fd);
FS_SYNC_TRACE_END(fstat);
if (err != 0) {
return; // error info is in ctx
if (is_uv_error(err)) {
return;
}

Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
Expand Down
9 changes: 2 additions & 7 deletions test/parallel/test-fs-sync-fd-leak.js
View file
Expand Up @@ -25,7 +25,6 @@ require('../common');
const assert = require('assert');
const fs = require('fs');
const { internalBinding } = require('internal/test/binding');
const { UV_EBADF } = internalBinding('uv');

// Ensure that (read|write|append)FileSync() closes the file descriptor
fs.openSync = function() {
Expand All @@ -42,15 +41,11 @@ fs.writeSync = function() {
throw new Error('BAM');
};

internalBinding('fs').fstat = function(fd, bigint, _, ctx) {
ctx.errno = UV_EBADF;
ctx.syscall = 'fstat';
internalBinding('fs').fstat = function() {
throw new Error('EBADF: bad file descriptor, fstat');
};

let close_called = 0;
ensureThrows(function() {
fs.readFileSync('dummy');
}, 'EBADF: bad file descriptor, fstat');
ensureThrows(function() {
fs.writeFileSync('dummy', 'xxx');
}, 'BAM');
Expand Down
12 changes: 6 additions & 6 deletions typings/internalBinding/fs.d.ts
View file
Expand Up @@ -92,9 +92,9 @@ declare namespace InternalFSBinding {
function fstat(fd: number, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
function fstat(fd: number, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
function fstat(fd: number, useBigint: false, req: FSReqCallback<Float64Array>): void;
function fstat(fd: number, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
function fstat(fd: number, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
function fstat(fd: number, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
function fstat(fd: number, useBigint: boolean, req: undefined, shouldNotThrow: boolean): Float64Array | BigUint64Array;
function fstat(fd: number, useBigint: true, req: undefined, shouldNotThrow: boolean): BigUint64Array;
function fstat(fd: number, useBigint: false, req: undefined, shouldNotThrow: boolean): Float64Array;
function fstat(fd: number, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
function fstat(fd: number, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
function fstat(fd: number, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;
Expand Down Expand Up @@ -127,9 +127,9 @@ declare namespace InternalFSBinding {
function lstat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
function lstat(path: StringOrBuffer, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
function lstat(path: StringOrBuffer, useBigint: false, req: FSReqCallback<Float64Array>): void;
function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, throwIfNoEntry: boolean): Float64Array | BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: true, req: undefined, throwIfNoEntry: boolean): BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: false, req: undefined, throwIfNoEntry: boolean): Float64Array;
function lstat(path: StringOrBuffer, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
function lstat(path: StringOrBuffer, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
function lstat(path: StringOrBuffer, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;
Expand Down

0 comments on commit 05e25e0

Please sign in to comment.