fs: improve readFileSync with file descriptors

PR-URL: #49691 Reviewed-By: Stephen Belanger <[email protected]>
nodejs · Sep 28, 2023 · 31702c9 · 31702c9
1 parent cd9754d
commit 31702c9
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 27 deletions.
diff --git a/benchmark/fs/readFileSync.js b/benchmark/fs/readFileSync.js
@@ -6,12 +6,21 @@ const fs = require('fs');
 const bench = common.createBenchmark(main, {
   encoding: ['undefined', 'utf8'],
   path: ['existing', 'non-existing'],
-  n: [60e1],
+  hasFileDescriptor: ['true', 'false'],
+  n: [1e4],
 });
 
-function main({ n, encoding, path }) {
+function main({ n, encoding, path, hasFileDescriptor }) {
   const enc = encoding === 'undefined' ? undefined : encoding;
-  const file = path === 'existing' ? __filename : '/tmp/not-found';
+  let file;
+  let shouldClose = false;
+
+  if (hasFileDescriptor === 'true') {
+    shouldClose = path === 'existing';
+    file = path === 'existing' ? fs.openSync(__filename) : -1;
+  } else {
+    file = path === 'existing' ? __filename : '/tmp/not-found';
+  }
   bench.start();
   for (let i = 0; i < n; ++i) {
     try {
@@ -21,4 +30,7 @@ function main({ n, encoding, path }) {
     }
   }
   bench.end(n);
+  if (shouldClose) {
+    fs.closeSync(file);
+  }
 }
diff --git a/lib/fs.js b/lib/fs.js
@@ -437,13 +437,11 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) {
 function readFileSync(path, options) {
   options = getOptions(options, { flag: 'r' });
 
-  const isUserFd = isFd(path); // File descriptor ownership
-
-  // TODO(@anonrig): Do not handle file descriptor ownership for now.
-  if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
+  if (options.encoding === 'utf8' || options.encoding === 'utf-8') {
     return syncFs.readFileUtf8(path, options.flag);
   }
 
+  const isUserFd = isFd(path); // File descriptor ownership
   const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666);
 
   const stats = tryStatSync(fd, isUserFd);

diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js
@@ -9,7 +9,7 @@ const {
   getStatFsFromBinding,
   getValidatedFd,
 } = require('internal/fs/utils');
-const { parseFileMode } = require('internal/validators');
+const { parseFileMode, isInt32 } = require('internal/validators');
 
 const binding = internalBinding('fs');
 
@@ -19,7 +19,9 @@ const binding = internalBinding('fs');
  * @return {string}
  */
 function readFileUtf8(path, flag) {
-  path = pathModule.toNamespacedPath(getValidatedPath(path));
+  if (!isInt32(path)) {
+    path = pathModule.toNamespacedPath(getValidatedPath(path));
+  }
   return binding.readFileUtf8(path, stringToFlags(flag));
 }
 

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -2190,7 +2190,7 @@ static void OpenSync(const FunctionCallbackInfo<Value>& args) {
   uv_fs_t req;
   auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
   FS_SYNC_TRACE_BEGIN(open);
-  int err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr);
+  auto err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr);
   FS_SYNC_TRACE_END(open);
   if (err < 0) {
     return env->ThrowUVException(err, "open", nullptr, path.out());
@@ -2581,30 +2581,41 @@ static void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) {
 
   CHECK_GE(args.Length(), 2);
 
-  BufferValue path(env->isolate(), args[0]);
-  CHECK_NOT_NULL(*path);
-
   CHECK(args[1]->IsInt32());
   const int flags = args[1].As<Int32>()->Value();
 
-  if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
-
+  uv_file file;
   uv_fs_t req;
-  auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
 
-  FS_SYNC_TRACE_BEGIN(open);
-  uv_file file = uv_fs_open(nullptr, &req, *path, flags, 438, nullptr);
-  FS_SYNC_TRACE_END(open);
-  if (req.result < 0) {
-    // req will be cleaned up by scope leave.
-    return env->ThrowUVException(req.result, "open", nullptr, path.out());
+  bool is_fd = args[0]->IsInt32();
+
+  // Check for file descriptor
+  if (is_fd) {
+    file = args[0].As<Int32>()->Value();
+  } else {
+    BufferValue path(env->isolate(), args[0]);
+    CHECK_NOT_NULL(*path);
+    if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
+
+    FS_SYNC_TRACE_BEGIN(open);
+    file = uv_fs_open(nullptr, &req, *path, flags, O_RDONLY, nullptr);
+    FS_SYNC_TRACE_END(open);
+    if (req.result < 0) {
+      uv_fs_req_cleanup(&req);
+      // req will be cleaned up by scope leave.
+      return env->ThrowUVException(req.result, "open", nullptr, path.out());
+    }
   }
 
-  auto defer_close = OnScopeLeave([file]() {
-    uv_fs_t close_req;
-    CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr));
-    uv_fs_req_cleanup(&close_req);
+  auto defer_close = OnScopeLeave([file, is_fd, &req]() {
+    if (!is_fd) {
+      FS_SYNC_TRACE_BEGIN(close);
+      CHECK_EQ(0, uv_fs_close(nullptr, &req, file, nullptr));
+      FS_SYNC_TRACE_END(close);
+    }
+    uv_fs_req_cleanup(&req);
   });
+
   std::string result{};
   char buffer[8192];
   uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer));
@@ -2615,7 +2626,7 @@ static void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) {
     if (req.result < 0) {
       FS_SYNC_TRACE_END(read);
       // req will be cleaned up by scope leave.
-      return env->ThrowUVException(req.result, "read", nullptr, path.out());
+      return env->ThrowUVException(req.result, "read", nullptr);
     }
     if (r <= 0) {
       break;