Skip to content

Commit 0f6805d

Browse files
addaleaxMylesBorins
authored andcommitted
src: add option to track unmanaged file descriptors
Add the ability to track “raw” file descriptors, i.e. integers returned by `fs.open()`, and close them on `Environment` shutdown, to match the behavior for all other resource types (which are also closed on shutdown). PR-URL: #34303 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
1 parent 8bafba2 commit 0f6805d

File tree

6 files changed

+48
-1
lines changed

6 files changed

+48
-1
lines changed

src/env-inl.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -817,6 +817,10 @@ inline bool Environment::owns_inspector() const {
817817
return flags_ & EnvironmentFlags::kOwnsInspector;
818818
}
819819

820+
inline bool Environment::tracks_unmanaged_fds() const {
821+
return flags_ & EnvironmentFlags::kTrackUnmanagedFds;
822+
}
823+
820824
bool Environment::filehandle_close_warning() const {
821825
return emit_filehandle_warning_;
822826
}

src/env.cc

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,12 @@ void Environment::RunCleanup() {
619619
}
620620
CleanupHandles();
621621
}
622+
623+
for (const int fd : unmanaged_fds_) {
624+
uv_fs_t close_req;
625+
uv_fs_close(nullptr, &close_req, fd, nullptr);
626+
uv_fs_req_cleanup(&close_req);
627+
}
622628
}
623629

624630
void Environment::RunAtExitCallbacks() {
@@ -981,6 +987,24 @@ Environment* Environment::worker_parent_env() const {
981987
return worker_context()->env();
982988
}
983989

990+
void Environment::AddUnmanagedFd(int fd) {
991+
if (!tracks_unmanaged_fds()) return;
992+
auto result = unmanaged_fds_.insert(fd);
993+
if (!result.second) {
994+
ProcessEmitWarning(
995+
this, "File descriptor %d opened in unmanaged mode twice", fd);
996+
}
997+
}
998+
999+
void Environment::RemoveUnmanagedFd(int fd) {
1000+
if (!tracks_unmanaged_fds()) return;
1001+
size_t removed_count = unmanaged_fds_.erase(fd);
1002+
if (removed_count == 0) {
1003+
ProcessEmitWarning(
1004+
this, "File descriptor %d closed but not opened in unmanaged mode", fd);
1005+
}
1006+
}
1007+
9841008
void Environment::BuildEmbedderGraph(Isolate* isolate,
9851009
EmbedderGraph* graph,
9861010
void* data) {

src/env.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,6 +1015,7 @@ class Environment : public MemoryRetainer {
10151015
inline bool should_not_register_esm_loader() const;
10161016
inline bool owns_process_state() const;
10171017
inline bool owns_inspector() const;
1018+
inline bool tracks_unmanaged_fds() const;
10181019
inline uint64_t thread_id() const;
10191020
inline worker::Worker* worker_context() const;
10201021
Environment* worker_parent_env() const;
@@ -1226,6 +1227,9 @@ class Environment : public MemoryRetainer {
12261227
inline std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>*
12271228
released_allocated_buffers();
12281229

1230+
void AddUnmanagedFd(int fd);
1231+
void RemoveUnmanagedFd(int fd);
1232+
12291233
private:
12301234
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
12311235
const char* errmsg);
@@ -1363,6 +1367,8 @@ class Environment : public MemoryRetainer {
13631367
int64_t initial_base_object_count_ = 0;
13641368
std::atomic_bool is_stopping_ { false };
13651369

1370+
std::unordered_set<int> unmanaged_fds_;
1371+
13661372
std::function<void(Environment*, int)> process_exit_handler_ {
13671373
DefaultProcessExitHandler };
13681374

src/node.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,10 @@ enum Flags : uint64_t {
417417
// Set if Node.js should not run its own esm loader. This is needed by some
418418
// embedders, because it's possible for the Node.js esm loader to conflict
419419
// with another one in an embedder environment, e.g. Blink's in Chromium.
420-
kNoRegisterESMLoader = 1 << 3
420+
kNoRegisterESMLoader = 1 << 3,
421+
// Set this flag to make Node.js track "raw" file descriptors, i.e. managed
422+
// by fs.open() and fs.close(), and close them during FreeEnvironment().
423+
kTrackUnmanagedFds = 1 << 4
421424
};
422425
} // namespace EnvironmentFlags
423426

src/node_file.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,9 @@ void AfterInteger(uv_fs_t* req) {
653653
FSReqBase* req_wrap = FSReqBase::from_req(req);
654654
FSReqAfterScope after(req_wrap, req);
655655

656+
if (req->result >= 0 && req_wrap->is_plain_open())
657+
req_wrap->env()->AddUnmanagedFd(req->result);
658+
656659
if (after.Proceed())
657660
req_wrap->Resolve(Integer::New(req_wrap->env()->isolate(), req->result));
658661
}
@@ -862,6 +865,7 @@ void Close(const FunctionCallbackInfo<Value>& args) {
862865

863866
CHECK(args[0]->IsInt32());
864867
int fd = args[0].As<Int32>()->Value();
868+
env->RemoveUnmanagedFd(fd);
865869

866870
FSReqBase* req_wrap_async = GetReqWrap(args, 1);
867871
if (req_wrap_async != nullptr) { // close(fd, req)
@@ -1706,6 +1710,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
17061710

17071711
FSReqBase* req_wrap_async = GetReqWrap(args, 3);
17081712
if (req_wrap_async != nullptr) { // open(path, flags, mode, req)
1713+
req_wrap_async->set_is_plain_open(true);
17091714
AsyncCall(env, req_wrap_async, args, "open", UTF8, AfterInteger,
17101715
uv_fs_open, *path, flags, mode);
17111716
} else { // open(path, flags, mode, undefined, ctx)
@@ -1715,6 +1720,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
17151720
int result = SyncCall(env, args[4], &req_wrap_sync, "open",
17161721
uv_fs_open, *path, flags, mode);
17171722
FS_SYNC_TRACE_END(open);
1723+
if (result >= 0) env->AddUnmanagedFd(result);
17181724
args.GetReturnValue().Set(result);
17191725
}
17201726
}

src/node_file.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,9 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
8989
const char* data() const { return has_data_ ? *buffer_ : nullptr; }
9090
enum encoding encoding() const { return encoding_; }
9191
bool use_bigint() const { return use_bigint_; }
92+
bool is_plain_open() const { return is_plain_open_; }
93+
94+
void set_is_plain_open(bool value) { is_plain_open_ = value; }
9295

9396
FSContinuationData* continuation_data() const {
9497
return continuation_data_.get();
@@ -113,6 +116,7 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
113116
enum encoding encoding_ = UTF8;
114117
bool has_data_ = false;
115118
bool use_bigint_ = false;
119+
bool is_plain_open_ = false;
116120
const char* syscall_ = nullptr;
117121

118122
BaseObjectPtr<BindingData> binding_data_;

0 commit comments

Comments
 (0)