From 0d18d20d74e8c092f3f680155b5f0f042f8330fe Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 5 Nov 2025 10:42:01 -0500 Subject: [PATCH] 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. --- src/dataqueue/queue.cc | 1 + src/node_file.cc | 43 +++++++++++++++++++---------- src/node_file.h | 7 ++++- test/parallel/test-fs-filehandle.js | 5 +++- 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/dataqueue/queue.cc b/src/dataqueue/queue.cc index 64643680903a78..537844806d3087 100644 --- a/src/dataqueue/queue.cc +++ b/src/dataqueue/queue.cc @@ -916,6 +916,7 @@ class FdEntry final : public EntryImpl { fs::FileHandle::New(realm->GetBindingData(), file, Local(), + {}, entry->start_, entry->end_ - entry->start_)), entry); diff --git a/src/node_file.cc b/src/node_file.cc index f0d512069c6dc6..00dfa557dd42af 100644 --- a/src/node_file.cc +++ b/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 obj, int fd) + Local 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 obj, + std::string original_name, std::optional maybeOffset, std::optional 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& args) { FileHandle::New(binding_data, args[0].As()->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,17 +372,19 @@ 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 = ""; + 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; } @@ -380,7 +392,10 @@ inline void FileHandle::Close() { 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(req->result)) if (after.Proceed()) { - FileHandle* fd = FileHandle::New(req_wrap->binding_data(), - static_cast(req->result)); + FileHandle* fd = FileHandle::New( + req_wrap->binding_data(), static_cast(req->result), {}, req->path); if (fd == nullptr) return; req_wrap->Resolve(fd->object()); } @@ -2222,7 +2237,7 @@ static void OpenFileHandle(const FunctionCallbackInfo& 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()); } diff --git a/src/node_file.h b/src/node_file.h index 224fa6f7ade375..e028562fef0412 100644 --- a/src/node_file.h +++ b/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 obj = v8::Local(), + std::string original_name = {}, std::optional maybeOffset = std::nullopt, std::optional maybeLength = std::nullopt); ~FileHandle() override; @@ -395,7 +396,10 @@ class FileHandle final : public AsyncWrap, public StreamBase { int fd_; }; - FileHandle(BindingData* binding_data, v8::Local obj, int fd); + FileHandle(BindingData* binding_data, + v8::Local 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 ClosePromise(); + std::string original_name_; int fd_; bool closing_ = false; bool closed_ = false; diff --git a/test/parallel/test-fs-filehandle.js b/test/parallel/test-fs-filehandle.js index 6813ad1c28a1d0..064ec423614d64 100644 --- a/test/parallel/test-fs-filehandle.js +++ b/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); }