Skip to content

Commit 0dfcfa4

Browse files
committed
fs: improve readFileSync with file descriptors
1 parent 7e12d0e commit 0dfcfa4

File tree

4 files changed

+51
-28
lines changed

4 files changed

+51
-28
lines changed

benchmark/fs/readFileSync.js

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,21 @@ const fs = require('fs');
66
const bench = common.createBenchmark(main, {
77
encoding: ['undefined', 'utf8'],
88
path: ['existing', 'non-existing'],
9-
n: [60e1],
9+
hasFileDescriptor: ['true', 'false'],
10+
n: [1e4],
1011
});
1112

12-
function main({ n, encoding, path }) {
13+
function main({ n, encoding, path, hasFileDescriptor }) {
1314
const enc = encoding === 'undefined' ? undefined : encoding;
14-
const file = path === 'existing' ? __filename : '/tmp/not-found';
15+
let file;
16+
let shouldClose = false;
17+
18+
if (hasFileDescriptor === 'true') {
19+
shouldClose = path === 'existing';
20+
file = path === 'existing' ? fs.openSync(__filename) : -1;
21+
} else {
22+
file = path === 'existing' ? __filename : '/tmp/not-found';
23+
}
1524
bench.start();
1625
for (let i = 0; i < n; ++i) {
1726
try {
@@ -21,4 +30,7 @@ function main({ n, encoding, path }) {
2130
}
2231
}
2332
bench.end(n);
33+
if (shouldClose) {
34+
fs.closeSync(file);
35+
}
2436
}

lib/fs.js

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ const {
131131
CHAR_BACKWARD_SLASH,
132132
} = require('internal/constants');
133133
const {
134-
isUint32,
134+
isInt32,
135135
parseFileMode,
136136
validateBoolean,
137137
validateBuffer,
@@ -201,7 +201,7 @@ function makeStatsCallback(cb) {
201201
};
202202
}
203203

204-
const isFd = isUint32;
204+
const isFd = isInt32;
205205

206206
function isFileType(stats, fileType) {
207207
// Use stats array directly to avoid creating an fs.Stats instance just for
@@ -437,13 +437,11 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) {
437437
function readFileSync(path, options) {
438438
options = getOptions(options, { flag: 'r' });
439439

440-
const isUserFd = isFd(path); // File descriptor ownership
441-
442-
// TODO(@anonrig): Do not handle file descriptor ownership for now.
443-
if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
440+
if (options.encoding === 'utf8' || options.encoding === 'utf-8') {
444441
return syncFs.readFileUtf8(path, options.flag);
445442
}
446443

444+
const isUserFd = isFd(path); // File descriptor ownership
447445
const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666);
448446

449447
const stats = tryStatSync(fd, isUserFd);

lib/internal/fs/sync.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const {
99
getStatFsFromBinding,
1010
getValidatedFd,
1111
} = require('internal/fs/utils');
12-
const { parseFileMode } = require('internal/validators');
12+
const { parseFileMode, isInt32 } = require('internal/validators');
1313

1414
const binding = internalBinding('fs');
1515

@@ -19,7 +19,9 @@ const binding = internalBinding('fs');
1919
* @return {string}
2020
*/
2121
function readFileUtf8(path, flag) {
22-
path = pathModule.toNamespacedPath(getValidatedPath(path));
22+
if (!isInt32(path)) {
23+
path = pathModule.toNamespacedPath(getValidatedPath(path));
24+
}
2325
return binding.readFileUtf8(path, stringToFlags(flag));
2426
}
2527

src/node_file.cc

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ using v8::Object;
7676
using v8::ObjectTemplate;
7777
using v8::Promise;
7878
using v8::String;
79+
using v8::Uint32;
7980
using v8::Undefined;
8081
using v8::Value;
8182

@@ -2156,7 +2157,7 @@ static void OpenSync(const FunctionCallbackInfo<Value>& args) {
21562157
uv_fs_t req;
21572158
auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
21582159
FS_SYNC_TRACE_BEGIN(open);
2159-
int err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr);
2160+
auto err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr);
21602161
FS_SYNC_TRACE_END(open);
21612162
if (err < 0) {
21622163
return env->ThrowUVException(err, "open", nullptr, path.out());
@@ -2547,30 +2548,40 @@ static void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) {
25472548

25482549
CHECK_GE(args.Length(), 2);
25492550

2550-
BufferValue path(env->isolate(), args[0]);
2551-
CHECK_NOT_NULL(*path);
2552-
25532551
CHECK(args[1]->IsInt32());
25542552
const int flags = args[1].As<Int32>()->Value();
25552553

2556-
if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
2557-
2554+
uv_file file;
25582555
uv_fs_t req;
25592556
auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
25602557

2561-
FS_SYNC_TRACE_BEGIN(open);
2562-
uv_file file = uv_fs_open(nullptr, &req, *path, flags, 438, nullptr);
2563-
FS_SYNC_TRACE_END(open);
2564-
if (req.result < 0) {
2565-
// req will be cleaned up by scope leave.
2566-
return env->ThrowUVException(req.result, "open", nullptr, path.out());
2558+
bool is_fd = args[0]->IsInt32();
2559+
2560+
// Check for file descriptor
2561+
if (is_fd) {
2562+
file = args[0].As<Int32>()->Value();
2563+
} else {
2564+
BufferValue path(env->isolate(), args[0]);
2565+
CHECK_NOT_NULL(*path);
2566+
if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
2567+
2568+
FS_SYNC_TRACE_BEGIN(open);
2569+
file = uv_fs_open(nullptr, &req, *path, flags, O_RDONLY, nullptr);
2570+
FS_SYNC_TRACE_END(open);
2571+
if (req.result < 0) {
2572+
// req will be cleaned up by scope leave.
2573+
return env->ThrowUVException(req.result, "open", nullptr, path.out());
2574+
}
25672575
}
25682576

2569-
auto defer_close = OnScopeLeave([file]() {
2570-
uv_fs_t close_req;
2571-
CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr));
2572-
uv_fs_req_cleanup(&close_req);
2577+
auto defer_close = OnScopeLeave([file, is_fd]() {
2578+
if (!is_fd) {
2579+
uv_fs_t close_req;
2580+
CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr));
2581+
uv_fs_req_cleanup(&close_req);
2582+
}
25732583
});
2584+
25742585
std::string result{};
25752586
char buffer[8192];
25762587
uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer));
@@ -2581,7 +2592,7 @@ static void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) {
25812592
if (req.result < 0) {
25822593
FS_SYNC_TRACE_END(read);
25832594
// req will be cleaned up by scope leave.
2584-
return env->ThrowUVException(req.result, "read", nullptr, path.out());
2595+
return env->ThrowUVException(req.result, "read", nullptr);
25852596
}
25862597
if (r <= 0) {
25872598
break;

0 commit comments

Comments
 (0)