From 1c564066bbb86e2db114fa852b0c657693d21105 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 13 Mar 2018 21:07:10 +0800 Subject: [PATCH 1/2] src: call CleanupHandles in FreeEnvironment CleanupHandles() has not been called in our own code base anymore after the v8 debug agent has been removed. It used to be in the ~Environment() destructor but then removed to avoid firing other events after the exit event, given that we were not going to clean up handles for the one environment per process setup. Call it in FreeEnvironment so that embedders can clean up the handles in the loop when creating multiple environments. --- src/node.cc | 1 + test/cctest/node_test_fixture.h | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index c03c753c37cf09..44cc19fa4558e0 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4410,6 +4410,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data, void FreeEnvironment(Environment* env) { + env->CleanupHandles(); delete env; } diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 660111c5a90c6f..e0740a47096460 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -118,7 +118,6 @@ class EnvironmentTestFixture : public NodeTestFixture { } ~Env() { - environment_->CleanupHandles(); node::FreeEnvironment(environment_); node::FreeIsolateData(isolate_data_); context_->Exit(); From 6abbf24d8b079ce6450cfc7a2db664c9c9bd7395 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 13 Mar 2018 21:50:50 +0800 Subject: [PATCH 2/2] src: simplify Environment::HandleCleanup - Make the HandleCleanup a struct, and make the queue a std::list, iterate over it in CleanupHandles() and clear it after that. - Put the handle cleanup registration into a method and document that they will not be called in the one environemt per process setup. --- src/env-inl.h | 2 +- src/env.cc | 53 ++++++++++++++++++++++++++++++--------------------- src/env.h | 44 ++++++++++++++++-------------------------- 3 files changed, 48 insertions(+), 51 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 31f2af402417dc..43e8c16a789b94 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -341,7 +341,7 @@ inline uv_idle_t* Environment::immediate_idle_handle() { inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void *arg) { - handle_cleanup_queue_.PushBack(new HandleCleanup(handle, cb, arg)); + handle_cleanup_queue_.push_back(HandleCleanup{handle, cb, arg}); } inline void Environment::FinishHandleCleanup(uv_handle_t* handle) { diff --git a/src/env.cc b/src/env.cc index 913f0e865beb1d..bcb30668a01e7e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -175,7 +175,34 @@ void Environment::Start(int argc, uv_unref(reinterpret_cast(&idle_prepare_handle_)); uv_unref(reinterpret_cast(&idle_check_handle_)); - auto close_and_finish = [](Environment* env, uv_handle_t* handle, void* arg) { + // Register clean-up cb to be called to clean up the handles + // when the environment is freed, note that they are not cleaned in + // the one environment per process setup, but will be called in + // FreeEnvironment. + RegisterHandleCleanups(); + + if (start_profiler_idle_notifier) { + StartProfilerIdleNotifier(); + } + + auto process_template = FunctionTemplate::New(isolate()); + process_template->SetClassName(FIXED_ONE_BYTE_STRING(isolate(), "process")); + + auto process_object = + process_template->GetFunction()->NewInstance(context()).ToLocalChecked(); + set_process_object(process_object); + + SetupProcessObject(this, argc, argv, exec_argc, exec_argv); + LoadAsyncWrapperInfo(this); + + static uv_once_t init_once = UV_ONCE_INIT; + uv_once(&init_once, InitThreadLocalOnce); + uv_key_set(&thread_local_env, this); +} + +void Environment::RegisterHandleCleanups() { + HandleCleanupCb close_and_finish = [](Environment* env, uv_handle_t* handle, + void* arg) { handle->data = env; uv_close(handle, [](uv_handle_t* handle) { @@ -199,32 +226,14 @@ void Environment::Start(int argc, reinterpret_cast(&idle_check_handle_), close_and_finish, nullptr); - - if (start_profiler_idle_notifier) { - StartProfilerIdleNotifier(); - } - - auto process_template = FunctionTemplate::New(isolate()); - process_template->SetClassName(FIXED_ONE_BYTE_STRING(isolate(), "process")); - - auto process_object = - process_template->GetFunction()->NewInstance(context()).ToLocalChecked(); - set_process_object(process_object); - - SetupProcessObject(this, argc, argv, exec_argc, exec_argv); - LoadAsyncWrapperInfo(this); - - static uv_once_t init_once = UV_ONCE_INIT; - uv_once(&init_once, InitThreadLocalOnce); - uv_key_set(&thread_local_env, this); } void Environment::CleanupHandles() { - while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) { + for (HandleCleanup& hc : handle_cleanup_queue_) { handle_cleanup_waiting_++; - hc->cb_(this, hc->handle_, hc->arg_); - delete hc; + hc.cb_(this, hc.handle_, hc.arg_); } + handle_cleanup_queue_.clear(); while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE); diff --git a/src/env.h b/src/env.h index e6060b9e6faab9..3250e77bd1bb58 100644 --- a/src/env.h +++ b/src/env.h @@ -530,26 +530,6 @@ class Environment { DISALLOW_COPY_AND_ASSIGN(TickInfo); }; - typedef void (*HandleCleanupCb)(Environment* env, - uv_handle_t* handle, - void* arg); - - class HandleCleanup { - private: - friend class Environment; - - HandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void* arg) - : handle_(handle), - cb_(cb), - arg_(arg) { - } - - uv_handle_t* handle_; - HandleCleanupCb cb_; - void* arg_; - ListNode handle_cleanup_queue_; - }; - static inline Environment* GetCurrent(v8::Isolate* isolate); static inline Environment* GetCurrent(v8::Local context); static inline Environment* GetCurrent( @@ -570,7 +550,22 @@ class Environment { int exec_argc, const char* const* exec_argv, bool start_profiler_idle_notifier); + + typedef void (*HandleCleanupCb)(Environment* env, + uv_handle_t* handle, + void* arg); + struct HandleCleanup { + uv_handle_t* handle_; + HandleCleanupCb cb_; + void* arg_; + }; + + void RegisterHandleCleanups(); void CleanupHandles(); + inline void RegisterHandleCleanup(uv_handle_t* handle, + HandleCleanupCb cb, + void *arg); + inline void FinishHandleCleanup(uv_handle_t* handle); inline void AssignToContext(v8::Local context, const ContextInfo& info); @@ -586,12 +581,6 @@ class Environment { inline uv_check_t* immediate_check_handle(); inline uv_idle_t* immediate_idle_handle(); - // 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); - inline AsyncHooks* async_hooks(); inline ImmediateInfo* immediate_info(); inline TickInfo* tick_info(); @@ -809,8 +798,7 @@ class Environment { friend int GenDebugSymbols(); HandleWrapQueue handle_wrap_queue_; ReqWrapQueue req_wrap_queue_; - ListHead handle_cleanup_queue_; + std::list handle_cleanup_queue_; int handle_cleanup_waiting_; double* heap_statistics_buffer_ = nullptr;