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)){
 returnnullptr;
  }
-auto handle = newFileHandle(binding_data, obj, fd);
+auto handle = newFileHandle(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,
 
 voidFileHandle::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);
 
-structerr_detail{int ret; int fd};
+structerr_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());
+staticconstexpr 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
 voidClose();
@@ -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');
 constfs=internalBinding('fs');
 const{ stringToFlags }=require('internal/fs/utils');
 
+constfilepath=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,/^AFileHandleobjectwasclosedduring/);
+assert.match(err.message,newRegExp(RegExp.escape(filepath)));
 }));
 
 
 {
 constctx={};
-fs.openFileHandle(path.toNamespacedPath(__filename),
+fs.openFileHandle(filepath,
 stringToFlags('r'),0o666,undefined,ctx);
 assert.strictEqual(ctx.errno,undefined);
 }