src: show original file name in FileHandle GC close errors

Otherwise, it can be virtually impossible to debug where these types
of errors originate from.

PR-URL: #60593
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

Author: addaleax

src/dataqueue/queue.cc

@@ -916,6 +916,7 @@ class FdEntry final : public EntryImpl {
               fs::FileHandle::New(realm->GetBindingData<fs::BindingData>(),
                                   file,
                                   Local<Object>(),
+                                  {},
                                   entry->start_,
                                   entry->end_ - entry->start_)),
           entry);

src/node_file.cc

@@ -222,9 +222,12 @@ void FSReqBase::MemoryInfo(MemoryTracker* tracker) const {
 // collection if necessary. If that happens, a process warning will be
 // emitted (or a fatal exception will occur if the fd cannot be closed.)
 FileHandle::FileHandle(BindingData* binding_data,
-                       Local<Object> obj, int fd)
+                       Local<Object> obj,
+                       int fd,
+                       std::string original_name)
     : AsyncWrap(binding_data->env(), obj, AsyncWrap::PROVIDER_FILEHANDLE),
       StreamBase(env()),
+      original_name_(std::move(original_name)),
       fd_(fd),
       binding_data_(binding_data) {
   MakeWeak();
@@ -234,6 +237,7 @@ FileHandle::FileHandle(BindingData* binding_data,
 FileHandle* FileHandle::New(BindingData* binding_data,
                             int fd,
                             Local<Object> obj,
+                            std::string original_name,
                             std::optional<int64_t> maybeOffset,
                             std::optional<int64_t> maybeLength) {
   Environment* env = binding_data->env();
@@ -242,7 +246,7 @@ FileHandle* FileHandle::New(BindingData* binding_data,
                             .ToLocal(&obj)) {
     return nullptr;
   }
-  auto handle = new FileHandle(binding_data, obj, fd);
+  auto handle = new FileHandle(binding_data, obj, fd, original_name);
   if (maybeOffset.has_value()) handle->read_offset_ = maybeOffset.value();
   if (maybeLength.has_value()) handle->read_length_ = maybeLength.value();
   return handle;
@@ -274,6 +278,7 @@ void FileHandle::New(const FunctionCallbackInfo<Value>& args) {
   FileHandle::New(binding_data,
                   args[0].As<Int32>()->Value(),
                   args.This(),
+                  {},
                   maybeOffset,
                   maybeLength);
 }
@@ -293,6 +298,7 @@ int FileHandle::DoWrite(WriteWrap* w,
 
 void FileHandle::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackField("current_read", current_read_);
+  tracker->TrackField("original_name", original_name_);
 }
 
 BaseObject::TransferMode FileHandle::GetTransferMode() const {
@@ -344,9 +350,13 @@ inline void FileHandle::Close() {
   FS_SYNC_TRACE_END(close);
   uv_fs_req_cleanup(&req);
 
-  struct err_detail { int ret; int fd; };
+  struct err_detail {
+    int ret;
+    int fd;
+    std::string name;
+  };
 
-  err_detail detail { ret, fd_ };
+  err_detail detail{ret, fd_, original_name_};
 
   AfterClose();
 
@@ -362,25 +372,30 @@ inline void FileHandle::Close() {
   // down the process is the only reasonable thing we can do here.
   env()->SetImmediate([detail](Environment* env) {
     HandleScope handle_scope(env->isolate());
+    static constexpr std::string_view unknown_path = "<unknown path>";
+    std::string_view filename =
+        detail.name.empty() ? unknown_path : detail.name;
 
     // If there was an error while trying to close the file descriptor,
     // we will throw that instead.
     if (detail.ret < 0) {
-      char msg[70];
-      snprintf(msg,
-               arraysize(msg),
-               "Closing file descriptor %d on garbage collection failed",
-               detail.fd);
+      auto formatted = SPrintF(
+          "Closing file descriptor %d on garbage collection failed (%s)",
+          detail.fd,
+          filename);
       HandleScope handle_scope(env->isolate());
-      env->ThrowUVException(detail.ret, "close", msg);
+      env->ThrowUVException(detail.ret, "close", formatted.c_str());
       return;
     }
 
     THROW_ERR_INVALID_STATE(
         env,
         "A FileHandle object was closed during garbage collection. "
         "This used to be allowed with a deprecation warning but is now "
-        "considered an error. Please close FileHandle objects explicitly.");
+        "considered an error. Please close FileHandle objects explicitly. "
+        "File descriptor: %d (%s)",
+        detail.fd,
+        filename);
   });
 }
 
@@ -824,8 +839,8 @@ void AfterOpenFileHandle(uv_fs_t* req) {
   FS_ASYNC_TRACE_END1(
       req->fs_type, req_wrap, "result", static_cast<int>(req->result))
   if (after.Proceed()) {
-    FileHandle* fd = FileHandle::New(req_wrap->binding_data(),
-                                     static_cast<int>(req->result));
+    FileHandle* fd = FileHandle::New(
+        req_wrap->binding_data(), static_cast<int>(req->result), {}, req->path);
     if (fd == nullptr) return;
     req_wrap->Resolve(fd->object());
   }
@@ -2222,7 +2237,7 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
     if (result < 0) {
       return;  // syscall failed, no need to continue, error info is in ctx
     }
-    FileHandle* fd = FileHandle::New(binding_data, result);
+    FileHandle* fd = FileHandle::New(binding_data, result, {}, path.ToString());
     if (fd == nullptr) return;
     args.GetReturnValue().Set(fd->object());
   }

src/node_file.h

@@ -329,6 +329,7 @@ class FileHandle final : public AsyncWrap, public StreamBase {
   static FileHandle* New(BindingData* binding_data,
                          int fd,
                          v8::Local<v8::Object> obj = v8::Local<v8::Object>(),
+                         std::string original_name = {},
                          std::optional<int64_t> maybeOffset = std::nullopt,
                          std::optional<int64_t> maybeLength = std::nullopt);
   ~FileHandle() override;
@@ -395,7 +396,10 @@ class FileHandle final : public AsyncWrap, public StreamBase {
     int fd_;
   };
 
-  FileHandle(BindingData* binding_data, v8::Local<v8::Object> obj, int fd);
+  FileHandle(BindingData* binding_data,
+             v8::Local<v8::Object> obj,
+             int fd,
+             std::string original_name);
 
   // Synchronous close that emits a warning
   void Close();
@@ -437,6 +441,7 @@ class FileHandle final : public AsyncWrap, public StreamBase {
   // Asynchronous close
   v8::MaybeLocal<v8::Promise> ClosePromise();
 
+  std::string original_name_;
   int fd_;
   bool closing_ = false;
   bool closed_ = false;

test/parallel/test-fs-filehandle.js

@@ -8,17 +8,20 @@ const { internalBinding } = require('internal/test/binding');
 const fs = internalBinding('fs');
 const { stringToFlags } = require('internal/fs/utils');
 
+const filepath = path.toNamespacedPath(__filename);
+
 // Verifies that the FileHandle object is garbage collected and that an
 // error is thrown if it is not closed.
 process.on('uncaughtException', common.mustCall((err) => {
   assert.strictEqual(err.code, 'ERR_INVALID_STATE');
   assert.match(err.message, /^A FileHandle object was closed during/);
+  assert.match(err.message, new RegExp(RegExp.escape(filepath)));
 }));
 
 
 {
   const ctx = {};
-  fs.openFileHandle(path.toNamespacedPath(__filename),
+  fs.openFileHandle(filepath,
                     stringToFlags('r'), 0o666, undefined, ctx);
   assert.strictEqual(ctx.errno, undefined);
 }