From e253edb48a154aeb5464d342b1db5a8e8fda6add Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 4 Sep 2017 22:02:55 +0200 Subject: [PATCH] src: make CleanupHandles() tear down handles/reqs Previously, handles would not be closed when the current `Environment` stopped, which is acceptable in a single-`Environment`-per-process situation, but would otherwise create memory and file descriptor leaks. Also, introduce a generic way to close handles via the `Environment::CloseHandle()` function, which automatically keeps track of whether a close callback has been called yet or not. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/85 PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/cares_wrap.cc | 18 ++++++------------ src/connection_wrap.h | 2 -- src/env-inl.h | 22 ++++++++++++++++++++-- src/env.cc | 20 ++++++++++++-------- src/env.h | 6 +++++- src/fs_event_wrap.cc | 6 ++++-- src/handle_wrap.cc | 37 +++++++++++++++++++++++++------------ src/handle_wrap.h | 6 ++++++ src/node_stat_watcher.cc | 7 +------ src/process_wrap.cc | 2 ++ src/req_wrap-inl.h | 6 ++++++ src/req_wrap.h | 1 + src/tty_wrap.cc | 2 ++ 13 files changed, 90 insertions(+), 45 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 4208c02d4fe445..ae253d40ca94b2 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -267,9 +267,8 @@ void ares_poll_cb(uv_poll_t* watcher, int status, int events) { } -void ares_poll_close_cb(uv_handle_t* watcher) { - node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, - reinterpret_cast(watcher)); +void ares_poll_close_cb(uv_poll_t* watcher) { + node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, watcher); free(task); } @@ -347,8 +346,7 @@ void ares_sockstate_cb(void* data, "When an ares socket is closed we should have a handle for it"); channel->task_list()->erase(it); - uv_close(reinterpret_cast(&task->poll_watcher), - ares_poll_close_cb); + channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb); if (channel->task_list()->empty()) { uv_timer_stop(channel->timer_handle()); @@ -517,10 +515,7 @@ ChannelWrap::~ChannelWrap() { void ChannelWrap::CleanupTimer() { if (timer_handle_ == nullptr) return; - uv_close(reinterpret_cast(timer_handle_), - [](uv_handle_t* handle) { - delete reinterpret_cast(handle); - }); + env()->CloseHandle(timer_handle_, [](uv_timer_t* handle){ delete handle; }); timer_handle_ = nullptr; } @@ -610,8 +605,7 @@ class QueryWrap : public AsyncWrap { static_cast(this)); } - static void CaresAsyncClose(uv_handle_t* handle) { - uv_async_t* async = reinterpret_cast(handle); + static void CaresAsyncClose(uv_async_t* async) { auto data = static_cast(async->data); delete data->wrap; delete data; @@ -636,7 +630,7 @@ class QueryWrap : public AsyncWrap { free(host); } - uv_close(reinterpret_cast(handle), CaresAsyncClose); + wrap->env()->CloseHandle(handle, CaresAsyncClose); } static void Callback(void *arg, int status, int timeouts, diff --git a/src/connection_wrap.h b/src/connection_wrap.h index afb168c614aa97..72030a00901daf 100644 --- a/src/connection_wrap.h +++ b/src/connection_wrap.h @@ -23,8 +23,6 @@ class ConnectionWrap : public LibuvStreamWrap { ConnectionWrap(Environment* env, v8::Local object, ProviderType provider); - ~ConnectionWrap() { - } UVType handle_; }; diff --git a/src/env-inl.h b/src/env-inl.h index d3c0c211d97328..917ddd1b6bcb75 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -349,8 +349,26 @@ inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, handle_cleanup_queue_.push_back(HandleCleanup{handle, cb, arg}); } -inline void Environment::FinishHandleCleanup(uv_handle_t* handle) { - handle_cleanup_waiting_--; +template +inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { + handle_cleanup_waiting_++; + static_assert(sizeof(T) >= sizeof(uv_handle_t), "T is a libuv handle"); + static_assert(offsetof(T, data) == offsetof(uv_handle_t, data), + "T is a libuv handle"); + static_assert(offsetof(T, close_cb) == offsetof(uv_handle_t, close_cb), + "T is a libuv handle"); + struct CloseData { + Environment* env; + OnCloseCallback callback; + void* original_data; + }; + handle->data = new CloseData { this, callback, handle->data }; + uv_close(reinterpret_cast(handle), [](uv_handle_t* handle) { + std::unique_ptr data { static_cast(handle->data) }; + data->env->handle_cleanup_waiting_--; + handle->data = data->original_data; + data->callback(reinterpret_cast(handle)); + }); } inline uv_loop_t* Environment::event_loop() const { diff --git a/src/env.cc b/src/env.cc index aadb81092e507c..6526c680ac1792 100644 --- a/src/env.cc +++ b/src/env.cc @@ -209,9 +209,7 @@ void Environment::RegisterHandleCleanups() { void* arg) { handle->data = env; - uv_close(handle, [](uv_handle_t* handle) { - static_cast(handle->data)->FinishHandleCleanup(handle); - }); + env->CloseHandle(handle, [](uv_handle_t* handle) {}); }; RegisterHandleCleanup( @@ -233,13 +231,17 @@ void Environment::RegisterHandleCleanups() { } void Environment::CleanupHandles() { - for (HandleCleanup& hc : handle_cleanup_queue_) { - handle_cleanup_waiting_++; + for (ReqWrap* request : req_wrap_queue_) + request->Cancel(); + + for (HandleWrap* handle : handle_wrap_queue_) + handle->Close(); + + for (HandleCleanup& hc : handle_cleanup_queue_) hc.cb_(this, hc.handle_, hc.arg_); - } handle_cleanup_queue_.clear(); - while (handle_cleanup_waiting_ != 0) + while (handle_cleanup_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) uv_run(event_loop(), UV_RUN_ONCE); } @@ -306,6 +308,8 @@ void Environment::PrintSyncTrace() const { } void Environment::RunCleanup() { + CleanupHandles(); + while (!cleanup_hooks_.empty()) { // Copy into a vector, since we can't sort an unordered_set in-place. std::vector callbacks( @@ -329,8 +333,8 @@ void Environment::RunCleanup() { cb.fn_(cb.arg_); cleanup_hooks_.erase(cb); - CleanupHandles(); } + CleanupHandles(); } } diff --git a/src/env.h b/src/env.h index 3acb27c9545525..79351666c1182e 100644 --- a/src/env.h +++ b/src/env.h @@ -577,10 +577,14 @@ class Environment { void RegisterHandleCleanups(); void CleanupHandles(); + + // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void *arg); - inline void FinishHandleCleanup(uv_handle_t* handle); + + template + inline void CloseHandle(T* handle, OnCloseCallback callback); inline void AssignToContext(v8::Local context, const ContextInfo& info); diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index ed74f36719db79..579e446fc5c485 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -78,11 +78,12 @@ FSEventWrap::FSEventWrap(Environment* env, Local object) : HandleWrap(env, object, reinterpret_cast(&handle_), - AsyncWrap::PROVIDER_FSEVENTWRAP) {} + AsyncWrap::PROVIDER_FSEVENTWRAP) { + MarkAsUninitialized(); +} FSEventWrap::~FSEventWrap() { - CHECK_EQ(initialized_, false); } void FSEventWrap::GetInitialized(const FunctionCallbackInfo& args) { @@ -153,6 +154,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { } err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags); + wrap->MarkAsInitialized(); wrap->initialized_ = true; if (err != 0) { diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 49bf0c55bea0a1..20356b94a5775a 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -61,29 +61,40 @@ void HandleWrap::HasRef(const FunctionCallbackInfo& args) { void HandleWrap::Close(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - HandleWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - // Guard against uninitialized handle or double close. - if (!IsAlive(wrap)) - return; + wrap->Close(args[0]); +} - if (wrap->state_ != kInitialized) +void HandleWrap::Close(v8::Local close_callback) { + if (state_ != kInitialized) return; - CHECK_EQ(false, wrap->persistent().IsEmpty()); - uv_close(wrap->handle_, OnClose); - wrap->state_ = kClosing; + CHECK_EQ(false, persistent().IsEmpty()); + uv_close(handle_, OnClose); + state_ = kClosing; - if (args[0]->IsFunction()) { - wrap->object()->Set(env->onclose_string(), args[0]); - wrap->state_ = kClosingWithCallback; + if (!close_callback.IsEmpty() && close_callback->IsFunction()) { + object()->Set(env()->context(), env()->onclose_string(), close_callback) + .FromJust(); + state_ = kClosingWithCallback; } } +void HandleWrap::MarkAsInitialized() { + env()->handle_wrap_queue()->PushBack(this); + state_ = kInitialized; +} + + +void HandleWrap::MarkAsUninitialized() { + handle_wrap_queue_.Remove(); + state_ = kClosed; +} + + HandleWrap::HandleWrap(Environment* env, Local object, uv_handle_t* handle, @@ -110,6 +121,8 @@ void HandleWrap::OnClose(uv_handle_t* handle) { const bool have_close_callback = (wrap->state_ == kClosingWithCallback); wrap->state_ = kClosed; + wrap->OnClose(); + if (have_close_callback) wrap->MakeCallback(env->onclose_string(), 0, nullptr); diff --git a/src/handle_wrap.h b/src/handle_wrap.h index e7a335f5140253..fd2d002dce0338 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -70,11 +70,17 @@ class HandleWrap : public AsyncWrap { inline uv_handle_t* GetHandle() const { return handle_; } + void Close(v8::Local close_callback = v8::Local()); + protected: HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); + virtual void OnClose() {} + + void MarkAsInitialized(); + void MarkAsUninitialized(); private: friend class Environment; diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index a2cfb1088c9d25..d8f8a6a362237d 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -75,11 +75,6 @@ void StatWatcher::Initialize(Environment* env, Local target) { } -static void Delete(uv_handle_t* handle) { - delete reinterpret_cast(handle); -} - - StatWatcher::StatWatcher(Environment* env, Local wrap) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER), watcher_(new uv_fs_poll_t) { @@ -93,7 +88,7 @@ StatWatcher::~StatWatcher() { if (IsActive()) { Stop(); } - uv_close(reinterpret_cast(watcher_), Delete); + env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; }); } diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 96d60cc900583c..6d421fe7c4d4de 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -88,6 +88,7 @@ class ProcessWrap : public HandleWrap { object, reinterpret_cast(&process_), AsyncWrap::PROVIDER_PROCESSWRAP) { + MarkAsUninitialized(); } static void ParseStdioOptions(Environment* env, @@ -256,6 +257,7 @@ class ProcessWrap : public HandleWrap { } int err = uv_spawn(env->event_loop(), &wrap->process_, &options); + wrap->MarkAsInitialized(); if (err == 0) { CHECK_EQ(wrap->process_.data, wrap); diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index 11b1389fa0e771..ab5051e41d8e89 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -7,6 +7,7 @@ #include "async_wrap-inl.h" #include "env-inl.h" #include "util-inl.h" +#include "uv.h" namespace node { @@ -37,6 +38,11 @@ ReqWrap* ReqWrap::from_req(T* req) { return ContainerOf(&ReqWrap::req_, req); } +template +void ReqWrap::Cancel() { + uv_cancel(reinterpret_cast(&req_)); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req_wrap.h b/src/req_wrap.h index 656be38dcea943..4d6a89d743c591 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -19,6 +19,7 @@ class ReqWrap : public AsyncWrap { inline ~ReqWrap() override; inline void Dispatched(); // Call this after the req has been dispatched. T* req() { return &req_; } + inline void Cancel(); static ReqWrap* from_req(T* req); diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index c5abc6bf9b9b91..d01caba4a558f2 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -172,6 +172,8 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); + if (*init_err != 0) + MarkAsUninitialized(); } } // namespace node