fs: add fast api for fs.closeSync only

anonrig · anonrig · commit 0c93582c5090 · 2024-07-18T22:09:45.000-04:00
diff --git a/lib/fs.js b/lib/fs.js
@@ -515,7 +515,7 @@ function close(fd, callback = defaultCloseCallback) {
  * @returns {void}
  */
 function closeSync(fd) {
-  binding.close(fd);
+  binding.closeSync(fd);
 }
 
 /**
diff --git a/src/env.cc b/src/env.cc
@@ -1841,12 +1841,29 @@ void Environment::AddUnmanagedFd(int fd) {
   }
 }
 
-void Environment::RemoveUnmanagedFd(int fd) {
+void Environment::RemoveUnmanagedFd(int fd,
+                                    ProcessEmitScheduleType schedule_type) {
   if (!tracks_unmanaged_fds()) return;
   size_t removed_count = unmanaged_fds_.erase(fd);
   if (removed_count == 0) {
-    ProcessEmitWarning(
-        this, "File descriptor %d closed but not opened in unmanaged mode", fd);
+    switch (schedule_type) {
+      case kSync:
+        ProcessEmitWarning(
+            this,
+            "File descriptor %d closed but not opened in unmanaged mode",
+            fd);
+        break;
+      case kSetImmediate:
+        SetImmediateThreadsafe([&](Environment* env) {
+          ProcessEmitWarning(
+              env,
+              "File descriptor %d closed but not opened in unmanaged mode",
+              fd);
+        });
+        break;
+      default:
+        UNREACHABLE();
+    }
   }
 }
 
diff --git a/src/env.h b/src/env.h
@@ -1026,8 +1026,13 @@ class Environment : public MemoryRetainer {
   uv_buf_t allocate_managed_buffer(const size_t suggested_size);
   std::unique_ptr<v8::BackingStore> release_managed_buffer(const uv_buf_t& buf);
 
+  enum ProcessEmitScheduleType {
+    kSync,
+    kSetImmediate,
+  };
+
   void AddUnmanagedFd(int fd);
-  void RemoveUnmanagedFd(int fd);
+  void RemoveUnmanagedFd(int fd, ProcessEmitScheduleType schedule_type = kSync);
 
   template <typename T>
   void ForEachRealm(T&& iterator) const;
diff --git a/src/node_external_reference.h b/src/node_external_reference.h
@@ -55,6 +55,9 @@ using CFunctionWithInt64Fallback = void (*)(v8::Local<v8::Value>,
                                             const int64_t,
                                             v8::FastApiCallbackOptions&);
 using CFunctionWithBool = void (*)(v8::Local<v8::Value>, bool);
+using CFunctionWithUint32AndOptions = void (*)(v8::Local<v8::Object>,
+                                               const uint32_t,
+                                               v8::FastApiCallbackOptions&);
 
 // This class manages the external references from the V8 heap
 // to the C++ addresses in Node.js.
@@ -79,6 +82,7 @@ class ExternalReferenceRegistry {
   V(CFunctionWithDoubleReturnDouble)                                           \
   V(CFunctionWithInt64Fallback)                                                \
   V(CFunctionWithBool)                                                         \
+  V(CFunctionWithUint32AndOptions)                                             \
   V(const v8::CFunctionInfo*)                                                  \
   V(v8::FunctionCallback)                                                      \
   V(v8::AccessorNameGetterCallback)                                            \
diff --git a/src/node_file.cc b/src/node_file.cc
@@ -60,6 +60,7 @@ namespace fs {
 
 using v8::Array;
 using v8::BigInt;
+using v8::CFunction;
 using v8::Context;
 using v8::EscapableHandleScope;
 using v8::FastApiCallbackOptions;
@@ -966,32 +967,67 @@ void Access(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
-void Close(const FunctionCallbackInfo<Value>& args) {
+static void Close(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  const int argc = args.Length();
-  CHECK_GE(argc, 1);
+  CHECK_EQ(args.Length(), 2);  // fd, req
 
   int fd;
   if (!GetValidatedFd(env, args[0]).To(&fd)) {
     return;
   }
   env->RemoveUnmanagedFd(fd);
 
-  if (argc > 1) {  // close(fd, req)
-    FSReqBase* req_wrap_async = GetReqWrap(args, 1);
-    CHECK_NOT_NULL(req_wrap_async);
-    FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async)
-    AsyncCall(env, req_wrap_async, args, "close", UTF8, AfterNoArgs,
-              uv_fs_close, fd);
-  } else {  // close(fd)
-    FSReqWrapSync req_wrap_sync("close");
-    FS_SYNC_TRACE_BEGIN(close);
-    SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd);
-    FS_SYNC_TRACE_END(close);
+  FSReqBase* req_wrap_async = GetReqWrap(args, 1);
+  CHECK_NOT_NULL(req_wrap_async);
+  FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async)
+  AsyncCall(
+      env, req_wrap_async, args, "close", UTF8, AfterNoArgs, uv_fs_close, fd);
+}
+
+// Separate implementations are required to provide fast API for closeSync.
+// If both close and closeSync are implemented using the same function, and
+// if a fast API implementation is added for closeSync, close(fd, req) will
+// also trigger the fast API implementation and cause an incident.
+// Ref: https://github.com/nodejs/node/issues/53902
+static void CloseSync(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  CHECK_EQ(args.Length(), 1);
+
+  int fd;
+  if (!GetValidatedFd(env, args[0]).To(&fd)) {
+    return;
+  }
+  env->RemoveUnmanagedFd(fd);
+  FSReqWrapSync req_wrap_sync("close");
+  FS_SYNC_TRACE_BEGIN(close);
+  SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd);
+  FS_SYNC_TRACE_END(close);
+}
+
+static void FastCloseSync(Local<Object> recv,
+                          const uint32_t fd,
+                          // NOLINTNEXTLINE(runtime/references) This is V8 api.
+                          v8::FastApiCallbackOptions& options) {
+  Environment* env = Environment::GetCurrent(recv->GetCreationContextChecked());
+
+  uv_fs_t req;
+  FS_SYNC_TRACE_BEGIN(close);
+  int err = uv_fs_close(nullptr, &req, fd, nullptr);
+  FS_SYNC_TRACE_END(close);
+
+  if (is_uv_error(err)) {
+    options.fallback = true;
+  } else {
+    // Note: Only remove unmanaged fds if the close was successful.
+    // RemoveUnmanagedFd() can call ProcessEmitWarning() which calls back into
+    // JS process.emitWarning() and violates the fast API protocol.
+    env->RemoveUnmanagedFd(fd, Environment::kSetImmediate);
   }
 }
 
+CFunction fast_close_sync_(CFunction::Make(FastCloseSync));
+
 static void ExistsSync(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
@@ -3408,6 +3444,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
             GetFormatOfExtensionlessFile);
   SetMethod(isolate, target, "access", Access);
   SetMethod(isolate, target, "close", Close);
+  SetFastMethod(isolate, target, "closeSync", CloseSync, &fast_close_sync_);
   SetMethod(isolate, target, "existsSync", ExistsSync);
   SetMethod(isolate, target, "open", Open);
   SetMethod(isolate, target, "openFileHandle", OpenFileHandle);
@@ -3533,6 +3570,9 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
 
   registry->Register(GetFormatOfExtensionlessFile);
   registry->Register(Close);
+  registry->Register(CloseSync);
+  registry->Register(FastCloseSync);
+  registry->Register(fast_close_sync_.GetTypeInfo());
   registry->Register(ExistsSync);
   registry->Register(Open);
   registry->Register(OpenFileHandle);
diff --git a/test/parallel/test-fs-close-fast-api.js b/test/parallel/test-fs-close-fast-api.js
@@ -0,0 +1,31 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+
+// A large number is selected to ensure that the fast path is called.
+// Ref: https://github.com/nodejs/node/issues/53902
+for (let i = 0; i < 100_000; i++) {
+  try {
+    fs.closeSync(20);
+  } catch (e) {
+    // Ignore all errors except EBADF.
+    // EBADF stands for "Bad file descriptor".
+    if (e.code !== 'EBADF') {
+      throw e;
+    }
+  }
+}
+
+// This test is to ensure that fs.close() does not crash under pressure.
+for (let i = 0; i < 100_000; i++) {
+  fs.close(100, common.mustCall());
+}
+
+// This test is to ensure that fs.readFile() does not crash.
+// readFile() calls fs.close() internally if the input is not a file descriptor.
+// Large number is selected to ensure that the fast path is called.
+// Ref: https://github.com/nodejs/node/issues/53902
+for (let i = 0; i < 100_000; i++) {
+  fs.readFile(__filename, common.mustCall());
+}
diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts
@@ -70,7 +70,7 @@ declare namespace InternalFSBinding {
   function chown(path: string, uid: number, gid: number): void;
 
   function close(fd: number, req: FSReqCallback): void;
-  function close(fd: number): void;
+  function closeSync(fd: number): void;
 
   function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: FSReqCallback): void;
   function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: undefined, ctx: FSSyncContext): void;
@@ -256,6 +256,7 @@ export interface FsBinding {
   chmod: typeof InternalFSBinding.chmod;
   chown: typeof InternalFSBinding.chown;
   close: typeof InternalFSBinding.close;
+  closeSync: typeof InternalFSBinding.closeSync;
   copyFile: typeof InternalFSBinding.copyFile;
   fchmod: typeof InternalFSBinding.fchmod;
   fchown: typeof InternalFSBinding.fchown;

Original file line number	Diff line number	Diff line change
`@@ -515,7 +515,7 @@ function close(fd, callback = defaultCloseCallback) {`
`515`	`515`	`* @returns {void}`
`516`	`516`	`*/`
`517`	`517`	`function closeSync(fd) {`
`518`		`- binding.close(fd);`
	`518`	`+ binding.closeSync(fd);`
`519`	`519`	`}`
`520`	`520`
`521`	`521`	`/**`