src: use RAII to manage the main isolate data

This patch encapsulates the main isolate management into a
NodeMainInstance class that manages the resources with RAII
and controls the Isolate::CreateParams (which is necessary
for deserializing snapshots with external references)

PR-URL: https://github.com/nodejs/node/pull/27220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

7 files changed, 236 insertions, 184 deletions

diff --git a/node.gyp b/node.gyp
index 6e792c907b..ead1d67ff2 100644
--- a/node.gyp
+++ b/node.gyp
@@ -460,6 +460,7 @@
         'src/node_http_parser_traditional.cc',
         'src/node_http2.cc',
         'src/node_i18n.cc',
+        'src/node_main_instance.cc',
         'src/node_messaging.cc',
         'src/node_metadata.cc',
         'src/node_native_module.cc',
@@ -540,6 +541,7 @@
         'src/node_http2_state.h',
         'src/node_i18n.h',
         'src/node_internals.h',
+        'src/node_main_instance.h',
         'src/node_messaging.h',
         'src/node_metadata.h',
         'src/node_mutex.h',
diff --git a/src/api/environment.cc b/src/api/environment.cc
index 70228b77d8..0543e0323d 100644
--- a/src/api/environment.cc
+++ b/src/api/environment.cc
@@ -169,11 +169,7 @@ void FreeArrayBufferAllocator(ArrayBufferAllocator* allocator) {
   delete allocator;
 }
 
-void SetIsolateCreateParams(Isolate::CreateParams* params,
-                            ArrayBufferAllocator* allocator) {
-  if (allocator != nullptr)
-    params->array_buffer_allocator = allocator;
-
+void SetIsolateCreateParamsForNode(Isolate::CreateParams* params) {
   const uint64_t total_memory = uv_get_total_memory();
   if (total_memory > 0) {
     // V8 defaults to 700MB or 1.4GB on 32 and 64 bit platforms respectively.
@@ -204,25 +200,33 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
   return NewIsolate(allocator, event_loop, GetMainThreadMultiIsolatePlatform());
 }
 
-Isolate* NewIsolate(ArrayBufferAllocator* allocator,
+// TODO(joyeecheung): we may want to expose this, but then we need to be
+// careful about what we override in the params.
+Isolate* NewIsolate(Isolate::CreateParams* params,
                     uv_loop_t* event_loop,
                     MultiIsolatePlatform* platform) {
-  Isolate::CreateParams params;
-  SetIsolateCreateParams(&params, allocator);
-
   Isolate* isolate = Isolate::Allocate();
   if (isolate == nullptr) return nullptr;
 
   // Register the isolate on the platform before the isolate gets initialized,
   // so that the isolate can access the platform during initialization.
   platform->RegisterIsolate(isolate, event_loop);
-  Isolate::Initialize(isolate, params);
 
+  SetIsolateCreateParamsForNode(params);
+  Isolate::Initialize(isolate, *params);
   SetIsolateUpForNode(isolate);
 
   return isolate;
 }
 
+Isolate* NewIsolate(ArrayBufferAllocator* allocator,
+                    uv_loop_t* event_loop,
+                    MultiIsolatePlatform* platform) {
+  Isolate::CreateParams params;
+  if (allocator != nullptr) params.array_buffer_allocator = allocator;
+  return NewIsolate(&params, event_loop, platform);
+}
+
 IsolateData* CreateIsolateData(Isolate* isolate,
                                uv_loop_t* loop,
                                MultiIsolatePlatform* platform,
diff --git a/src/env.cc b/src/env.cc
index 88017ab545..ccb3c404f9 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -76,6 +76,8 @@ IsolateData::IsolateData(Isolate* isolate,
   // One byte because our strings are ASCII and we can safely skip V8's UTF-8
   // decoding step.
 
+  HandleScope handle_scope(isolate);
+
 #define V(PropertyName, StringValue)                                        \
     PropertyName ## _.Set(                                                  \
         isolate,                                                            \
diff --git a/src/node.cc b/src/node.cc
index 0e4e361824..53a9ae397b 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -25,16 +25,12 @@
 
 #include "debug_utils.h"
 #include "node_binding.h"
-#include "node_buffer.h"
-#include "node_constants.h"
-#include "node_context_data.h"
-#include "node_errors.h"
 #include "node_internals.h"
+#include "node_main_instance.h"
 #include "node_metadata.h"
 #include "node_native_module_env.h"
 #include "node_options-inl.h"
 #include "node_perf.h"
-#include "node_platform.h"
 #include "node_process.h"
 #include "node_revert.h"
 #include "node_v8_platform-inl.h"
@@ -56,13 +52,6 @@
 #include "node_dtrace.h"
 #endif
 
-#include "async_wrap-inl.h"
-#include "env-inl.h"
-#include "handle_wrap.h"
-#include "req_wrap-inl.h"
-#include "string_bytes.h"
-#include "util.h"
-#include "uv.h"
 #if NODE_USE_V8_PLATFORM
 #include "libplatform/libplatform.h"
 #endif  // NODE_USE_V8_PLATFORM
@@ -122,10 +111,8 @@ using native_module::NativeModuleEnv;
 using options_parser::kAllowedInEnvironment;
 using options_parser::kDisallowedInEnvironment;
 
-using v8::Array;
 using v8::Boolean;
 using v8::Context;
-using v8::DEFAULT;
 using v8::EscapableHandleScope;
 using v8::Exception;
 using v8::Function;
@@ -133,12 +120,10 @@ using v8::FunctionCallbackInfo;
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
-using v8::Locker;
 using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Object;
 using v8::Script;
-using v8::SealHandleScope;
 using v8::String;
 using v8::Undefined;
 using v8::V8;
@@ -767,161 +752,6 @@ void Init(int* argc,
     argv[i] = strdup(argv_[i].c_str());
 }
 
-void RunBeforeExit(Environment* env) {
-  env->RunBeforeExitCallbacks();
-
-  if (!uv_loop_alive(env->event_loop()))
-    EmitBeforeExit(env);
-}
-
-// TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h
-// and the environment creation routine in workers somehow.
-inline std::unique_ptr<Environment> CreateMainEnvironment(
-    IsolateData* isolate_data,
-    const std::vector<std::string>& args,
-    const std::vector<std::string>& exec_args,
-    int* exit_code) {
-  Isolate* isolate = isolate_data->isolate();
-  HandleScope handle_scope(isolate);
-
-  // TODO(addaleax): This should load a real per-Isolate option, currently
-  // this is still effectively per-process.
-  if (isolate_data->options()->track_heap_objects) {
-    isolate->GetHeapProfiler()->StartTrackingHeapObjects(true);
-  }
-
-  Local<Context> context = NewContext(isolate);
-  Context::Scope context_scope(context);
-
-  std::unique_ptr<Environment> env = std::make_unique<Environment>(
-      isolate_data,
-      context,
-      static_cast<Environment::Flags>(Environment::kIsMainThread |
-                                      Environment::kOwnsProcessState |
-                                      Environment::kOwnsInspector));
-  env->InitializeLibuv(per_process::v8_is_profiling);
-  env->ProcessCliArgs(args, exec_args);
-
-#if HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
-  CHECK(!env->inspector_agent()->IsListening());
-  // Inspector agent can't fail to start, but if it was configured to listen
-  // right away on the websocket port and fails to bind/etc, this will return
-  // false.
-  env->inspector_agent()->Start(args.size() > 1 ? args[1].c_str() : "",
-                                env->options()->debug_options(),
-                                env->inspector_host_port(),
-                                true);
-  if (env->options()->debug_options().inspector_enabled &&
-      !env->inspector_agent()->IsListening()) {
-    *exit_code = 12;  // Signal internal error.
-    return env;
-  }
-#else
-  // inspector_enabled can't be true if !HAVE_INSPECTOR or !NODE_USE_V8_PLATFORM
-  // - the option parser should not allow that.
-  CHECK(!env->options()->debug_options().inspector_enabled);
-#endif  // HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
-
-  if (RunBootstrapping(env.get()).IsEmpty()) {
-    *exit_code = 1;
-  }
-
-  return env;
-}
-
-inline int StartNodeWithIsolate(Isolate* isolate,
-                                IsolateData* isolate_data,
-                                const std::vector<std::string>& args,
-                                const std::vector<std::string>& exec_args) {
-  int exit_code = 0;
-  std::unique_ptr<Environment> env =
-      CreateMainEnvironment(isolate_data, args, exec_args, &exit_code);
-  CHECK_NOT_NULL(env);
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
-
-  if (exit_code == 0) {
-    {
-      AsyncCallbackScope callback_scope(env.get());
-      env->async_hooks()->push_async_ids(1, 0);
-      LoadEnvironment(env.get());
-      env->async_hooks()->pop_async_id(1);
-    }
-
-    {
-      SealHandleScope seal(isolate);
-      bool more;
-      env->performance_state()->Mark(
-          node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_START);
-      do {
-        uv_run(env->event_loop(), UV_RUN_DEFAULT);
-
-        per_process::v8_platform.DrainVMTasks(isolate);
-
-        more = uv_loop_alive(env->event_loop());
-        if (more && !env->is_stopping()) continue;
-
-        RunBeforeExit(env.get());
-
-        // Emit `beforeExit` if the loop became alive either after emitting
-        // event, or after running some callbacks.
-        more = uv_loop_alive(env->event_loop());
-      } while (more == true && !env->is_stopping());
-      env->performance_state()->Mark(
-          node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_EXIT);
-    }
-
-    env->set_trace_sync_io(false);
-    exit_code = EmitExit(env.get());
-    WaitForInspectorDisconnect(env.get());
-  }
-
-  env->set_can_call_into_js(false);
-  env->stop_sub_worker_contexts();
-  uv_tty_reset_mode();
-  env->RunCleanup();
-  RunAtExit(env.get());
-
-  per_process::v8_platform.DrainVMTasks(isolate);
-  per_process::v8_platform.CancelVMTasks(isolate);
-
-#if defined(LEAK_SANITIZER)
-  __lsan_do_leak_check();
-#endif
-
-  return exit_code;
-}
-
-inline int StartNodeWithLoopAndArgs(uv_loop_t* event_loop,
-                                    const std::vector<std::string>& args,
-                                    const std::vector<std::string>& exec_args) {
-  std::unique_ptr<ArrayBufferAllocator, decltype(&FreeArrayBufferAllocator)>
-      allocator(CreateArrayBufferAllocator(), &FreeArrayBufferAllocator);
-  Isolate* const isolate = NewIsolate(allocator.get(), event_loop);
-  if (isolate == nullptr)
-    return 12;  // Signal internal error.
-
-  int exit_code;
-  {
-    Locker locker(isolate);
-    Isolate::Scope isolate_scope(isolate);
-    HandleScope handle_scope(isolate);
-    std::unique_ptr<IsolateData, decltype(&FreeIsolateData)> isolate_data(
-        CreateIsolateData(isolate,
-                          event_loop,
-                          per_process::v8_platform.Platform(),
-                          allocator.get()),
-        &FreeIsolateData);
-    exit_code =
-        StartNodeWithIsolate(isolate, isolate_data.get(), args, exec_args);
-  }
-
-  isolate->Dispose();
-  per_process::v8_platform.Platform()->UnregisterIsolate(isolate);
-
-  return exit_code;
-}
-
 int Start(int argc, char** argv) {
   atexit([] () { uv_tty_reset_mode(); });
   PlatformInit();
@@ -981,8 +811,13 @@ int Start(int argc, char** argv) {
   V8::Initialize();
   performance::performance_v8_start = PERFORMANCE_NOW();
   per_process::v8_initialized = true;
-  const int exit_code =
-      StartNodeWithLoopAndArgs(uv_default_loop(), args, exec_args);
+
+  int exit_code = 0;
+  {
+    NodeMainInstance main_instance(uv_default_loop(), args, exec_args);
+    exit_code = main_instance.Run();
+  }
+
   per_process::v8_initialized = false;
   V8::Dispose();
 
diff --git a/src/node_internals.h b/src/node_internals.h
index bfc3a519f6..53f7d3cdba 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -297,7 +297,9 @@ bool SafeGetenv(const char* key, std::string* text, Environment* env = nullptr);
 }  // namespace credentials
 
 void DefineZlibConstants(v8::Local<v8::Object> target);
-
+v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params,
+                        uv_loop_t* event_loop,
+                        MultiIsolatePlatform* platform);
 v8::MaybeLocal<v8::Value> RunBootstrapping(Environment* env);
 v8::MaybeLocal<v8::Value> StartExecution(Environment* env,
                                          const char* main_script_id);
diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
new file mode 100644
index 0000000000..6ef992d006
--- /dev/null
+++ b/src/node_main_instance.cc
@@ -0,0 +1,162 @@
+#include "node_main_instance.h"
+#include "node_internals.h"
+#include "node_options-inl.h"
+#include "node_v8_platform-inl.h"
+
+namespace node {
+
+using v8::Context;
+using v8::HandleScope;
+using v8::Isolate;
+using v8::Local;
+using v8::Locker;
+using v8::SealHandleScope;
+
+NodeMainInstance::NodeMainInstance(uv_loop_t* event_loop,
+                                   const std::vector<std::string>& args,
+                                   const std::vector<std::string>& exec_args)
+    : args_(args),
+      exec_args_(exec_args),
+      array_buffer_allocator_(ArrayBufferAllocator::Create()),
+      isolate_(nullptr),
+      isolate_data_(nullptr) {
+  // TODO(joyeecheung): when we implement snapshot integration this needs to
+  // set params.external_references.
+  Isolate::CreateParams params;
+  params.array_buffer_allocator = array_buffer_allocator_.get();
+  isolate_ =
+      NewIsolate(&params, event_loop, per_process::v8_platform.Platform());
+  CHECK_NOT_NULL(isolate_);
+  isolate_data_.reset(CreateIsolateData(isolate_,
+                                        event_loop,
+                                        per_process::v8_platform.Platform(),
+                                        array_buffer_allocator_.get()));
+}
+
+NodeMainInstance::~NodeMainInstance() {
+  isolate_->Dispose();
+  per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);
+}
+
+int NodeMainInstance::Run() {
+  Locker locker(isolate_);
+  Isolate::Scope isolate_scope(isolate_);
+  HandleScope handle_scope(isolate_);
+
+  int exit_code = 0;
+  std::unique_ptr<Environment> env = CreateMainEnvironment(&exit_code);
+
+  CHECK_NOT_NULL(env);
+  Context::Scope context_scope(env->context());
+
+  if (exit_code == 0) {
+    {
+      AsyncCallbackScope callback_scope(env.get());
+      env->async_hooks()->push_async_ids(1, 0);
+      LoadEnvironment(env.get());
+      env->async_hooks()->pop_async_id(1);
+    }
+
+    {
+      SealHandleScope seal(isolate_);
+      bool more;
+      env->performance_state()->Mark(
+          node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_START);
+      do {
+        uv_run(env->event_loop(), UV_RUN_DEFAULT);
+
+        per_process::v8_platform.DrainVMTasks(isolate_);
+
+        more = uv_loop_alive(env->event_loop());
+        if (more && !env->is_stopping()) continue;
+
+        env->RunBeforeExitCallbacks();
+
+        if (!uv_loop_alive(env->event_loop())) {
+          EmitBeforeExit(env.get());
+        }
+
+        // Emit `beforeExit` if the loop became alive either after emitting
+        // event, or after running some callbacks.
+        more = uv_loop_alive(env->event_loop());
+      } while (more == true && !env->is_stopping());
+      env->performance_state()->Mark(
+          node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_EXIT);
+    }
+
+    env->set_trace_sync_io(false);
+    exit_code = EmitExit(env.get());
+    WaitForInspectorDisconnect(env.get());
+  }
+
+  env->set_can_call_into_js(false);
+  env->stop_sub_worker_contexts();
+  uv_tty_reset_mode();
+  env->RunCleanup();
+  RunAtExit(env.get());
+
+  per_process::v8_platform.DrainVMTasks(isolate_);
+  per_process::v8_platform.CancelVMTasks(isolate_);
+
+#if defined(LEAK_SANITIZER)
+  __lsan_do_leak_check();
+#endif
+
+  return exit_code;
+}
+
+// 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) {
+  *exit_code = 0;  // Reset the exit code to 0
+
+  HandleScope handle_scope(isolate_);
+
+  // TODO(addaleax): This should load a real per-Isolate option, currently
+  // this is still effectively per-process.
+  if (isolate_data_->options()->track_heap_objects) {
+    isolate_->GetHeapProfiler()->StartTrackingHeapObjects(true);
+  }
+
+  Local<Context> context = NewContext(isolate_);
+  Context::Scope context_scope(context);
+
+  std::unique_ptr<Environment> env = std::make_unique<Environment>(
+      isolate_data_.get(),
+      context,
+      static_cast<Environment::Flags>(Environment::kIsMainThread |
+                                      Environment::kOwnsProcessState |
+                                      Environment::kOwnsInspector));
+  env->InitializeLibuv(per_process::v8_is_profiling);
+  env->ProcessCliArgs(args_, exec_args_);
+
+#if HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
+  CHECK(!env->inspector_agent()->IsListening());
+  // Inspector agent can't fail to start, but if it was configured to listen
+  // right away on the websocket port and fails to bind/etc, this will return
+  // false.
+  env->inspector_agent()->Start(args_.size() > 1 ? args_[1].c_str() : "",
+                                env->options()->debug_options(),
+                                env->inspector_host_port(),
+                                true);
+  if (env->options()->debug_options().inspector_enabled &&
+      !env->inspector_agent()->IsListening()) {
+    *exit_code = 12;  // Signal internal error.
+    return env;
+  }
+#else
+  // inspector_enabled can't be true if !HAVE_INSPECTOR or
+  // !NODE_USE_V8_PLATFORM
+  // - the option parser should not allow that.
+  CHECK(!env->options()->debug_options().inspector_enabled);
+#endif  // HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
+
+  if (RunBootstrapping(env.get()).IsEmpty()) {
+    *exit_code = 1;
+  }
+
+  return env;
+}
+
+}  // namespace node
diff --git a/src/node_main_instance.h b/src/node_main_instance.h
new file mode 100644
index 0000000000..f140edcfd7
--- /dev/null
+++ b/src/node_main_instance.h
@@ -0,0 +1,45 @@
+#ifndef SRC_NODE_MAIN_INSTANCE_H_
+#define SRC_NODE_MAIN_INSTANCE_H_
+
+#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#include "node.h"
+#include "util.h"
+#include "uv.h"
+#include "v8.h"
+
+namespace node {
+
+// TODO(joyeecheung): align this with the Worker/WorkerThreadData class.
+// We may be able to create an abstract class to reuse some of the routines.
+class NodeMainInstance {
+ public:
+  NodeMainInstance(const NodeMainInstance&) = delete;
+  NodeMainInstance& operator=(const NodeMainInstance&) = delete;
+  NodeMainInstance(NodeMainInstance&&) = delete;
+  NodeMainInstance& operator=(NodeMainInstance&&) = delete;
+
+  NodeMainInstance(uv_loop_t* event_loop,
+                   const std::vector<std::string>& args,
+                   const std::vector<std::string>& exec_args);
+  ~NodeMainInstance();
+
+  // Start running the Node.js instances, return the exit code when finished.
+  int Run();
+
+ private:
+  // 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);
+
+  std::vector<std::string> args_;
+  std::vector<std::string> exec_args_;
+  std::unique_ptr<ArrayBufferAllocator> array_buffer_allocator_;
+  v8::Isolate* isolate_;
+  std::unique_ptr<IsolateData> isolate_data_;
+};
+
+}  // namespace node
+
+#endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+#endif  // SRC_NODE_MAIN_INSTANCE_H_