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.) PR-URL: #30467 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
nodejs · Mar 21, 2020 · d7bc581 · d7bc581
1 parent 78a2dc7
commit d7bc581
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 33 deletions.
diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -355,7 +355,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
@@ -112,7 +112,8 @@ int NodeMainInstance::Run() {
   HandleScope handle_scope(isolate_);
 
   int exit_code = 0;
-  std::unique_ptr<Environment> env = CreateMainEnvironment(&exit_code);
+  DeleteFnPtr<Environment, FreeEnvironment> env =
+      CreateMainEnvironment(&exit_code);
 
   CHECK_NOT_NULL(env);
   Context::Scope context_scope(env->context());
@@ -151,10 +152,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.
@@ -169,10 +167,6 @@ int NodeMainInstance::Run() {
   }
 #endif
 
-  RunAtExit(env.get());
-
-  per_process::v8_platform.DrainVMTasks(isolate_);
-
 #if defined(LEAK_SANITIZER)
   __lsan_do_leak_check();
 #endif
@@ -182,8 +176,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<Environment> NodeMainInstance::CreateMainEnvironment(
-    int* exit_code) {
+DeleteFnPtr<Environment, FreeEnvironment>
+NodeMainInstance::CreateMainEnvironment(int* exit_code) {
   *exit_code = 0;  // Reset the exit code to 0
 
   HandleScope handle_scope(isolate_);
@@ -208,14 +202,14 @@ std::unique_ptr<Environment> NodeMainInstance::CreateMainEnvironment(
   CHECK(!context.IsEmpty());
   Context::Scope context_scope(context);
 
-  std::unique_ptr<Environment> env = std::make_unique<Environment>(
+  DeleteFnPtr<Environment, FreeEnvironment> env { new Environment(
       isolate_data_.get(),
       context,
       args_,
       exec_args_,
       static_cast<Environment::Flags>(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
@@ -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<Environment> CreateMainEnvironment(int* exit_code);
+  DeleteFnPtr<Environment, FreeEnvironment> 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
@@ -421,6 +421,7 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) {
 
 void NodePlatform::DrainTasks(Isolate* isolate) {
   std::shared_ptr<PerIsolatePlatformData> per_isolate = ForNodeIsolate(isolate);
+  if (!per_isolate) return;
 
   do {
     // Worker tasks aren't associated with an Isolate.
@@ -489,12 +490,14 @@ std::shared_ptr<PerIsolatePlatformData>
 NodePlatform::ForNodeIsolate(Isolate* isolate) {
   Mutex::ScopedLock lock(per_isolate_mutex_);
   auto data = per_isolate_[isolate];
-  CHECK(data.second);
+  CHECK_NOT_NULL(data.first);
   return data.second;
 }
 
 bool NodePlatform::FlushForegroundTasks(Isolate* isolate) {
-  return ForNodeIsolate(isolate)->FlushForegroundTasksInternal();
+  std::shared_ptr<PerIsolatePlatformData> per_isolate = ForNodeIsolate(isolate);
+  if (!per_isolate) return false;
+  return per_isolate->FlushForegroundTasksInternal();
 }
 
 bool NodePlatform::IdleTasksEnabled(Isolate* isolate) {

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -270,26 +270,18 @@ void Worker::Run() {
     auto cleanup_env = OnScopeLeave([&]() {
       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;