Skip to content

Commit

Permalink
lib: add throws option to fs.f/l/statSync
Browse files Browse the repository at this point in the history
For consumers that aren't interested in *why* a `statSync` call failed,
allocating and throwing an exception is an unnecessary expense.  This PR
adds an option that will cause it to return `undefined` in such cases
instead.

As a motivating example, the JavaScript & TypeScript language service
shared between Visual Studio and Visual Studio Code is stuck with
synchronous file IO for architectural and backward-compatibility
reasons.  It frequently needs to speculatively check for the existence
of files and directories that may not exist (and cares about file vs
directory, so `existsSync` is insufficient), but ignores file system
entries it can't access, regardless of the reason.

Benchmarking the language service is difficult because it's so hard to
get good coverage of both code bases and user behaviors, but, as a
representative metric, we measured batch compilation of a few hundred
popular projects (by star count) from GitHub and found that, on average,
we saved about 1-2% of total compilation time.  We speculate that the
savings could be even more significant in interactive (language service
or watch mode) scenarios, where the same (non-existent) files need to be
polled over and over again.  It's not a huge improvement, but it's a
very small change and it will affect a lot of users (and CI runs).

For reference, our measurements were against `v12.x` (3637a06 at the
time) on an Ubuntu Server desktop with an SSD.

PR-URL: nodejs#33716
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
  • Loading branch information
amcasey authored and aduh95 committed Jan 14, 2021
1 parent 9c438b5 commit 91c40b8
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 3 deletions.
28 changes: 28 additions & 0 deletions benchmark/fs/bench-statSync-failure.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

const common = require('../common');
const fs = require('fs');
const path = require('path');

const bench = common.createBenchmark(main, {
n: [1e6],
statSyncType: ['throw', 'noThrow']
});


function main({ n, statSyncType }) {
const arg = path.join(__dirname, 'non.existent');

bench.start();
for (let i = 0; i < n; i++) {
if (statSyncType === 'noThrow') {
fs.statSync(arg, { throwIfNoEntry: false });
} else {
try {
fs.statSync(arg);
} catch {
}
}
}
bench.end(n);
}
6 changes: 6 additions & 0 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -2560,6 +2560,9 @@ changes:
* `options` {Object}
* `bigint` {boolean} Whether the numeric values in the returned
[`fs.Stats`][] object should be `bigint`. **Default:** `false`.
* `throwIfNoEntry` {boolean} Whether an exception will be thrown
if no file system entry exists, rather than returning `undefined`.
**Default:** `true`.
* Returns: {fs.Stats}

Synchronous lstat(2).
Expand Down Expand Up @@ -3765,6 +3768,9 @@ changes:
* `options` {Object}
* `bigint` {boolean} Whether the numeric values in the returned
[`fs.Stats`][] object should be `bigint`. **Default:** `false`.
* `throwIfNoEntry` {boolean} Whether an exception will be thrown
if no file system entry exists, rather than returning `undefined`.
**Default:** `true`.
* Returns: {fs.Stats}

Synchronous stat(2).
Expand Down
26 changes: 23 additions & 3 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const {
ERR_INVALID_CALLBACK,
ERR_FEATURE_UNAVAILABLE_ON_PLATFORM
},
uvErrmapGet,
uvException
} = require('internal/errors');

Expand Down Expand Up @@ -1061,28 +1062,47 @@ function stat(path, options = { bigint: false }, callback) {
binding.stat(pathModule.toNamespacedPath(path), options.bigint, req);
}

function fstatSync(fd, options = { bigint: false }) {
function hasNoEntryError(ctx) {
if (ctx.errno) {
const uvErr = uvErrmapGet(ctx.errno);
return uvErr && uvErr[0] === 'ENOENT';
}

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

return false;
}

function fstatSync(fd, options = { bigint: false, throwIfNoEntry: true }) {
validateInt32(fd, 'fd', 0);
const ctx = { fd };
const stats = binding.fstat(fd, options.bigint, undefined, ctx);
handleErrorFromBinding(ctx);
return getStatsFromBinding(stats);
}

function lstatSync(path, options = { bigint: false }) {
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;
}
handleErrorFromBinding(ctx);
return getStatsFromBinding(stats);
}

function statSync(path, options = { bigint: false }) {
function statSync(path, options = { bigint: false, throwIfNoEntry: true }) {
path = getValidatedPath(path);
const ctx = { path };
const stats = binding.stat(pathModule.toNamespacedPath(path),
options.bigint, undefined, ctx);
if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
return undefined;
}
handleErrorFromBinding(ctx);
return getStatsFromBinding(stats);
}
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-fs-stat-bigint.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,33 @@ if (!common.isWindows) {
fs.closeSync(fd);
}

{
assert.throws(
() => fs.statSync('does_not_exist'),
{ code: 'ENOENT' });
assert.strictEqual(
fs.statSync('does_not_exist', { throwIfNoEntry: false }),
undefined);
}

{
assert.throws(
() => fs.lstatSync('does_not_exist'),
{ code: 'ENOENT' });
assert.strictEqual(
fs.lstatSync('does_not_exist', { throwIfNoEntry: false }),
undefined);
}

{
assert.throws(
() => fs.fstatSync(9999),
{ code: 'EBADF' });
assert.throws(
() => fs.fstatSync(9999, { throwIfNoEntry: false }),
{ code: 'EBADF' });
}

const runCallbackTest = (func, arg, done) => {
const startTime = process.hrtime.bigint();
func(arg, { bigint: true }, common.mustCall((err, bigintStats) => {
Expand Down

0 comments on commit 91c40b8

Please sign in to comment.