src: use stack-allocated Environment instances

Makes it easier to reason about the lifetime of the Environment object. PR-URL: #7090 Refs: #7082 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
nodejs · bnoordhuis · Jun 2, 2016 · Jun 1, 2016 · Jun 1, 2016 · Jun 2, 2016
commit aac79dfd78a20ac66d99a55f24ddf91f937a2388
diff --git a/src/debug-agent.cc b/src/debug-agent.cc
@@ -174,31 +174,24 @@ void Agent::WorkerRun() {
     Local<Context> context = Context::New(isolate);
 
     Context::Scope context_scope(context);
-    Environment* env = CreateEnvironment(
-        &isolate_data,
-        context,
-        arraysize(argv),
-        argv,
-        arraysize(argv),
-        argv);
+    Environment env(&isolate_data, context);
 
-    child_env_ = env;
+    const bool start_profiler_idle_notifier = false;
+    env.Start(arraysize(argv), argv,
+              arraysize(argv), argv,
+              start_profiler_idle_notifier);
+
+    child_env_ = &env;
 
     // Expose API
-    InitAdaptor(env);
-    LoadEnvironment(env);
+    InitAdaptor(&env);
+    LoadEnvironment(&env);
 
-    CHECK_EQ(&child_loop_, env->event_loop());
+    CHECK_EQ(&child_loop_, env.event_loop());
     uv_run(&child_loop_, UV_RUN_DEFAULT);
 
     // Clean-up peristent
     api_.Reset();
-
-    // Clean-up all running handles
-    env->CleanupHandles();
-
-    env->Dispose();
-    env = nullptr;
   }
   isolate->Dispose();
 }

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -134,13 +134,6 @@ inline void Environment::TickInfo::set_index(uint32_t value) {
   fields_[kIndex] = value;
 }
 
-inline Environment* Environment::New(IsolateData* isolate_data,
-                                     v8::Local<v8::Context> context) {
-  Environment* env = new Environment(isolate_data, context);
-  env->AssignToContext(context);
-  return env;
-}
-
 inline void Environment::AssignToContext(v8::Local<v8::Context> context) {
   context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this);
 }
@@ -184,6 +177,7 @@ inline Environment::Environment(IsolateData* isolate_data,
 #if HAVE_INSPECTOR
       inspector_agent_(this),
 #endif
+      handle_cleanup_waiting_(0),
       http_parser_buffer_(nullptr),
       context_(context->GetIsolate(), context) {
   // We'll be creating new objects so make sure we've entered the context.
@@ -200,24 +194,12 @@ inline Environment::Environment(IsolateData* isolate_data,
   set_generic_internal_field_template(obj);
 
   RB_INIT(&cares_task_list_);
-  handle_cleanup_waiting_ = 0;
+  AssignToContext(context);
 }
 
 inline Environment::~Environment() {
   v8::HandleScope handle_scope(isolate());
 
-  context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
-                                             nullptr);
-#define V(PropertyName, TypeName) PropertyName ## _.Reset();
-  ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
-#undef V
-
-  delete[] heap_statistics_buffer_;
-  delete[] heap_space_statistics_buffer_;
-  delete[] http_parser_buffer_;
-}
-
-inline void Environment::CleanupHandles() {
   while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) {
     handle_cleanup_waiting_++;
     hc->cb_(this, hc->handle_, hc->arg_);
@@ -226,10 +208,16 @@ inline void Environment::CleanupHandles() {
 
   while (handle_cleanup_waiting_ != 0)
     uv_run(event_loop(), UV_RUN_ONCE);
-}
 
-inline void Environment::Dispose() {
-  delete this;
+  context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
+                                             nullptr);
+#define V(PropertyName, TypeName) PropertyName ## _.Reset();
+  ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
+#undef V
+
+  delete[] heap_statistics_buffer_;
+  delete[] heap_space_statistics_buffer_;
+  delete[] http_parser_buffer_;
 }
 
 inline v8::Isolate* Environment::isolate() const {

diff --git a/src/env.h b/src/env.h
@@ -446,17 +446,14 @@ class Environment {
   static inline Environment* GetCurrent(
       const v8::PropertyCallbackInfo<T>& info);
 
-  // See CreateEnvironment() in src/node.cc.
-  static inline Environment* New(IsolateData* isolate_data,
-                                 v8::Local<v8::Context> context);
+  inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
+  inline ~Environment();
+
   void Start(int argc,
              const char* const* argv,
              int exec_argc,
              const char* const* exec_argv,
              bool start_profiler_idle_notifier);
-  inline void CleanupHandles();
-  inline void Dispose();
-
   void AssignToContext(v8::Local<v8::Context> context);
 
   void StartProfilerIdleNotifier();
@@ -472,7 +469,7 @@ class Environment {
   inline uv_check_t* immediate_check_handle();
   inline uv_idle_t* immediate_idle_handle();
 
-  // Register clean-up cb to be called on env->Dispose()
+  // Register clean-up cb to be called on environment destruction.
   inline void RegisterHandleCleanup(uv_handle_t* handle,
                                     HandleCleanupCb cb,
                                     void *arg);
@@ -584,9 +581,6 @@ class Environment {
   static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX;
 
  private:
-  inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
-  inline ~Environment();
-
   v8::Isolate* const isolate_;
   IsolateData* const isolate_data_;
   uv_check_t immediate_check_handle_;

diff --git a/src/node.cc b/src/node.cc
@@ -3363,12 +3363,6 @@ void LoadEnvironment(Environment* env) {
 }
 
 
-void FreeEnvironment(Environment* env) {
-  CHECK_NE(env, nullptr);
-  env->Dispose();
-}
-
-
 static void PrintHelp();
 
 static bool ParseDebugOpt(const char* arg) {
@@ -4256,12 +4250,17 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
   Isolate* isolate = context->GetIsolate();
   HandleScope handle_scope(isolate);
   Context::Scope context_scope(context);
-  Environment* env = Environment::New(isolate_data, context);
+  auto env = new Environment(isolate_data, context);
   env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);
   return env;
 }
 
 
+void FreeEnvironment(Environment* env) {
+  delete env;
+}
+
+
 // Entry point for new node instances, also called directly for the main
 // node instance.
 static void StartNodeInstance(void* arg) {
@@ -4293,60 +4292,60 @@ static void StartNodeInstance(void* arg) {
                              array_buffer_allocator.zero_fill_field());
     Local<Context> context = Context::New(isolate);
     Context::Scope context_scope(context);
-    Environment* env = CreateEnvironment(&isolate_data,
-                                         context,
-                                         instance_data->argc(),
-                                         instance_data->argv(),
-                                         instance_data->exec_argc(),
-                                         instance_data->exec_argv());
+    Environment env(&isolate_data, context);
+    env.Start(instance_data->argc(),
+              instance_data->argv(),
+              instance_data->exec_argc(),
+              instance_data->exec_argv(),
+              v8_is_profiling);
 
     isolate->SetAbortOnUncaughtExceptionCallback(
         ShouldAbortOnUncaughtException);
 
     // Start debug agent when argv has --debug
     if (instance_data->use_debug_agent())
-      StartDebug(env, debug_wait_connect);
+      StartDebug(&env, debug_wait_connect);
 
     {
-      Environment::AsyncCallbackScope callback_scope(env);
-      LoadEnvironment(env);
+      Environment::AsyncCallbackScope callback_scope(&env);
+      LoadEnvironment(&env);
     }
 
-    env->set_trace_sync_io(trace_sync_io);
+    env.set_trace_sync_io(trace_sync_io);
 
     // Enable debugger
     if (instance_data->use_debug_agent())
-      EnableDebug(env);
+      EnableDebug(&env);
 
     {
       SealHandleScope seal(isolate);
       bool more;
       do {
         v8::platform::PumpMessageLoop(default_platform, isolate);
-        more = uv_run(env->event_loop(), UV_RUN_ONCE);
+        more = uv_run(env.event_loop(), UV_RUN_ONCE);
 
         if (more == false) {
           v8::platform::PumpMessageLoop(default_platform, isolate);
-          EmitBeforeExit(env);
+          EmitBeforeExit(&env);
 
           // Emit `beforeExit` if the loop became alive either after emitting
           // event, or after running some callbacks.
-          more = uv_loop_alive(env->event_loop());
-          if (uv_run(env->event_loop(), UV_RUN_NOWAIT) != 0)
+          more = uv_loop_alive(env.event_loop());
+          if (uv_run(env.event_loop(), UV_RUN_NOWAIT) != 0)
             more = true;
         }
       } while (more == true);
     }
 
-    env->set_trace_sync_io(false);
+    env.set_trace_sync_io(false);
 
-    int exit_code = EmitExit(env);
+    int exit_code = EmitExit(&env);
     if (instance_data->is_main())
       instance_data->set_exit_code(exit_code);
-    RunAtExit(env);
+    RunAtExit(&env);
 
 #if HAVE_INSPECTOR
-    if (env->inspector_agent()->connected()) {
+    if (env.inspector_agent()->connected()) {
       // Restore signal dispositions, the app is done and is no longer
       // capable of handling signals.
 #ifdef __POSIX__
@@ -4359,16 +4358,13 @@ static void StartNodeInstance(void* arg) {
         CHECK_EQ(0, sigaction(nr, &act, nullptr));
       }
 #endif
-      env->inspector_agent()->WaitForDisconnect();
+      env.inspector_agent()->WaitForDisconnect();
     }
 #endif
 
 #if defined(LEAK_SANITIZER)
     __lsan_do_leak_check();
 #endif
-
-    env->Dispose();
-    env = nullptr;
   }
 
   uv_mutex_lock(&node_isolate_mutex);