From 7d92ac7a3584cd11690e2f1d1745f4b5b02e1a17 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Nov 2019 19:47:03 +0000 Subject: [PATCH] src: make `FreeEnvironment()` perform all necessary cleanup Make the calls `stop_sub_worker_contexts()`, `RunCleanup()` part of the public API for easier embedding. (Note that calling `RunAtExit()` is idempotent because the at-exit callback queue is cleared after each call.) Backport-PR-URL: https://github.com/nodejs/node/pull/35241 PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 18 +++++++++++++++++- src/node_main_instance.cc | 18 ++++++------------ src/node_main_instance.h | 3 ++- src/node_platform.cc | 5 ++++- src/node_worker.cc | 26 +++++++++----------------- 5 files changed, 38 insertions(+), 32 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 97d25cba579570..d4fa856b8d59ea 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -342,7 +342,23 @@ Environment* CreateEnvironment(IsolateData* isolate_data, } void FreeEnvironment(Environment* env) { - env->RunCleanup(); + { + // TODO(addaleax): This should maybe rather be in a SealHandleScope. + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); + env->set_stopping(true); + env->stop_sub_worker_contexts(); + env->RunCleanup(); + RunAtExit(env); + } + + // This call needs to be made while the `Environment` is still alive + // because we assume that it is available for async tracking in the + // NodePlatform implementation. + MultiIsolatePlatform* platform = env->isolate_data()->platform(); + if (platform != nullptr) + platform->DrainTasks(env->isolate()); + delete env; } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 91bb30cb4e3ee6..c6533321bd6b64 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -110,7 +110,8 @@ int NodeMainInstance::Run() { HandleScope handle_scope(isolate_); int exit_code = 0; - std::unique_ptr env = CreateMainEnvironment(&exit_code); + DeleteFnPtr env = + CreateMainEnvironment(&exit_code); CHECK_NOT_NULL(env); Context::Scope context_scope(env->context()); @@ -149,10 +150,7 @@ int NodeMainInstance::Run() { exit_code = EmitExit(env.get()); } - env->set_can_call_into_js(false); - env->stop_sub_worker_contexts(); ResetStdio(); - env->RunCleanup(); // TODO(addaleax): Neither NODE_SHARED_MODE nor HAVE_INSPECTOR really // make sense here. @@ -167,10 +165,6 @@ int NodeMainInstance::Run() { } #endif - RunAtExit(env.get()); - - per_process::v8_platform.DrainVMTasks(isolate_); - #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); #endif @@ -180,8 +174,8 @@ int NodeMainInstance::Run() { // TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h // and the environment creation routine in workers somehow. -std::unique_ptr NodeMainInstance::CreateMainEnvironment( - int* exit_code) { +DeleteFnPtr +NodeMainInstance::CreateMainEnvironment(int* exit_code) { *exit_code = 0; // Reset the exit code to 0 HandleScope handle_scope(isolate_); @@ -205,14 +199,14 @@ std::unique_ptr NodeMainInstance::CreateMainEnvironment( CHECK(!context.IsEmpty()); Context::Scope context_scope(context); - std::unique_ptr env = std::make_unique( + DeleteFnPtr env { new Environment( isolate_data_.get(), context, args_, exec_args_, static_cast(Environment::kIsMainThread | Environment::kOwnsProcessState | - Environment::kOwnsInspector)); + Environment::kOwnsInspector)) }; env->InitializeLibuv(per_process::v8_is_profiling); env->InitializeDiagnostics(); diff --git a/src/node_main_instance.h b/src/node_main_instance.h index 5bc18cb3de63c0..cc9f50b9222de3 100644 --- a/src/node_main_instance.h +++ b/src/node_main_instance.h @@ -61,7 +61,8 @@ class NodeMainInstance { // TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h // and the environment creation routine in workers somehow. - std::unique_ptr CreateMainEnvironment(int* exit_code); + DeleteFnPtr CreateMainEnvironment( + int* exit_code); // If nullptr is returned, the binary is not built with embedded // snapshot. diff --git a/src/node_platform.cc b/src/node_platform.cc index 5b878f886a1320..e6e880b87b5380 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -402,6 +402,7 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { void NodePlatform::DrainTasks(Isolate* isolate) { std::shared_ptr per_isolate = ForIsolate(isolate); + if (!per_isolate) return; do { // Worker tasks aren't associated with an Isolate. @@ -468,7 +469,9 @@ NodePlatform::ForIsolate(Isolate* isolate) { } bool NodePlatform::FlushForegroundTasks(Isolate* isolate) { - return ForIsolate(isolate)->FlushForegroundTasksInternal(); + std::shared_ptr per_isolate = ForIsolate(isolate); + if (!per_isolate) return false; + return per_isolate->FlushForegroundTasksInternal(); } bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; } diff --git a/src/node_worker.cc b/src/node_worker.cc index 440f09f7b66e96..ec0fdf1cc0fad1 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -280,26 +280,18 @@ void Worker::Run() { if (!env_) return; env_->set_can_call_into_js(false); - Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, - Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); { - Context::Scope context_scope(env_->context()); - { - Mutex::ScopedLock lock(mutex_); - stopped_ = true; - this->env_ = nullptr; - } - env_->set_stopping(true); - env_->stop_sub_worker_contexts(); - env_->RunCleanup(); - RunAtExit(env_.get()); - - // This call needs to be made while the `Environment` is still alive - // because we assume that it is available for async tracking in the - // NodePlatform implementation. - platform_->DrainTasks(isolate_); + Mutex::ScopedLock lock(mutex_); + stopped_ = true; + this->env_ = nullptr; } + + // TODO(addaleax): Try moving DisallowJavascriptExecutionScope into + // FreeEnvironment(). + Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, + Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); + env_.reset(); }); if (is_stopped()) return;