fix: uninstall gc callbacks at exit

PR-URL: #168
X-Profiler · Apr 27, 2022 · 3918eff · 3918eff
1 parent f2c1109
commit 3918eff
Show file tree

Hide file tree

Showing 12 changed files with 95 additions and 46 deletions.
diff --git a/src/commands/gcprofiler/gc_profiler.cc b/src/commands/gcprofiler/gc_profiler.cc
@@ -7,10 +7,6 @@
 #include "nan.h"
 
 namespace xprofiler {
-using Nan::AddGCEpilogueCallback;
-using Nan::AddGCPrologueCallback;
-using Nan::RemoveGCEpilogueCallback;
-using Nan::RemoveGCPrologueCallback;
 using std::ios;
 using std::ofstream;
 using v8::GCType;
@@ -82,8 +78,8 @@ void GcProfiler::StartGCProfiling(v8::Isolate* isolate, std::string filename) {
   }
   env_data->gc_profiler = std::move(gc_profiler);
 
-  AddGCPrologueCallback(GCTracerPrologueCallback);
-  AddGCEpilogueCallback(GCTracerEpilogueCallback);
+  env_data->AddGCPrologueCallback(GCTracerPrologueCallback);
+  env_data->AddGCEpilogueCallback(GCTracerEpilogueCallback);
 
   JSONWriter* writer = env_data->gc_profiler->writer();
   writer->json_start();
@@ -92,9 +88,10 @@ void GcProfiler::StartGCProfiling(v8::Isolate* isolate, std::string filename) {
 }
 
 void GcProfiler::StopGCProfiling(v8::Isolate* isolate) {
-  RemoveGCPrologueCallback(GCTracerPrologueCallback);
-  RemoveGCEpilogueCallback(GCTracerEpilogueCallback);
   EnvironmentData* env_data = EnvironmentData::GetCurrent(isolate);
+  env_data->RemoveGCPrologueCallback(GCTracerPrologueCallback);
+  env_data->RemoveGCEpilogueCallback(GCTracerEpilogueCallback);
+
   if (env_data->gc_profiler == nullptr) {
     return;
   }

diff --git a/src/commands/listener.cc b/src/commands/listener.cc
@@ -1,9 +1,10 @@
-#include "../logger.h"
-#include "../platform/platform.h"
-#include "dump.h"
+#include "commands/dump.h"
+#include "commands/parser.h"
+#include "logger.h"
 #include "nan.h"
-#include "parser.h"
+#include "platform/platform.h"
 #include "uv.h"
+#include "xpf_mutex-inl.h"
 
 namespace xprofiler {
 using Nan::False;
@@ -12,16 +13,25 @@ using Nan::ThrowTypeError;
 using Nan::True;
 using v8::Value;
 
-static uv_thread_t uv_commands_listener_thread;
+namespace per_process {
+Mutex command_listener_mutex;
+bool command_listener_thread_created;
+uv_thread_t uv_commands_listener_thread;
+}  // namespace per_process
 
 static void CreateCommandsListenerThread(void* unused) {
   CreateIpcServer(ParseCmd);
 }
 
 void RunCommandsListener(const FunctionCallbackInfo<Value>& info) {
+  Mutex::ScopedLock lock(per_process::command_listener_mutex);
+  if (per_process::command_listener_thread_created) {
+    info.GetReturnValue().Set(True());
+    return;
+  }
   int rc = 0;
   // init commands listener thread
-  rc = uv_thread_create(&uv_commands_listener_thread,
+  rc = uv_thread_create(&per_process::uv_commands_listener_thread,
                         CreateCommandsListenerThread, nullptr);
   if (rc != 0) {
     ThrowTypeError("xprofiler: create uv commands listener thread failed!");

diff --git a/src/environment_data.cc b/src/environment_data.cc
@@ -67,11 +67,23 @@ void EnvironmentData::AtExit(void* arg) {
   EnvironmentRegistry* registry = ProcessData::Get()->environment_registry();
   EnvironmentRegistry::NoExitScope scope(registry);
   std::unique_ptr<EnvironmentData> env_data = registry->Unregister(isolate);
+
+  for (auto callback : env_data->gc_epilogue_callbacks_) {
+    Nan::RemoveGCEpilogueCallback(callback);
+  }
+  env_data->gc_epilogue_callbacks_.clear();
+
+  for (auto callback : env_data->gc_prologue_callbacks_) {
+    Nan::RemoveGCPrologueCallback(callback);
+  }
+  env_data->gc_prologue_callbacks_.clear();
+
   uv_close(reinterpret_cast<uv_handle_t*>(&env_data->interrupt_async_),
            nullptr);
   uv_close(reinterpret_cast<uv_handle_t*>(&env_data->statistics_async_),
            CloseCallback);
   per_thread::environment_data = nullptr;
+  // Release the unique_ptr, but delete it in CloseCallback.
   env_data.release();
 }
 
@@ -96,6 +108,34 @@ void EnvironmentData::RequestInterrupt(InterruptCallback interrupt) {
   uv_async_send(&interrupt_async_);
 }
 
+void EnvironmentData::AddGCEpilogueCallback(Nan::GCEpilogueCallback callback,
+                                            v8::GCType gc_type_filter) {
+  gc_epilogue_callbacks_.push_back(callback);
+  CHECK_EQ(isolate_, Isolate::GetCurrent());
+  Nan::AddGCEpilogueCallback(callback, gc_type_filter);
+}
+
+void EnvironmentData::RemoveGCEpilogueCallback(
+    Nan::GCEpilogueCallback callback) {
+  gc_epilogue_callbacks_.remove(callback);
+  CHECK_EQ(isolate_, Isolate::GetCurrent());
+  Nan::RemoveGCEpilogueCallback(callback);
+}
+
+void EnvironmentData::AddGCPrologueCallback(Nan::GCPrologueCallback callback,
+                                            v8::GCType gc_type_filter) {
+  gc_prologue_callbacks_.push_back(callback);
+  CHECK_EQ(isolate_, Isolate::GetCurrent());
+  Nan::AddGCPrologueCallback(callback, gc_type_filter);
+}
+
+void EnvironmentData::RemoveGCPrologueCallback(
+    Nan::GCPrologueCallback callback) {
+  gc_prologue_callbacks_.remove(callback);
+  CHECK_EQ(isolate_, Isolate::GetCurrent());
+  Nan::RemoveGCPrologueCallback(callback);
+}
+
 uint64_t EnvironmentData::GetUptime() const {
   uint64_t now = uv_hrtime();
   return (now - time_origin_) / kNanosecondsPerSecond;

diff --git a/src/environment_data.h b/src/environment_data.h
@@ -39,6 +39,12 @@ class EnvironmentData {
   void SendCollectStatistics();
 
   void RequestInterrupt(InterruptCallback interrupt);
+  void AddGCEpilogueCallback(Nan::GCEpilogueCallback callback,
+                             v8::GCType gc_type_filter = v8::kGCTypeAll);
+  void RemoveGCEpilogueCallback(Nan::GCEpilogueCallback callback);
+  void AddGCPrologueCallback(Nan::GCPrologueCallback callback,
+                             v8::GCType gc_type_filter = v8::kGCTypeAll);
+  void RemoveGCPrologueCallback(Nan::GCPrologueCallback callback);
 
   uint64_t GetUptime() const;
 
@@ -82,6 +88,8 @@ class EnvironmentData {
   Mutex interrupt_mutex_;
   std::list<InterruptCallback> interrupt_requests_;
   uv_async_t interrupt_async_;
+  std::list<Nan::GCEpilogueCallback> gc_epilogue_callbacks_;
+  std::list<Nan::GCPrologueCallback> gc_prologue_callbacks_;
 
   GcStatistics gc_statistics_;
   MemoryStatistics memory_statistics_;

diff --git a/src/logbypass/gc.cc b/src/logbypass/gc.cc
@@ -5,8 +5,6 @@
 #include "logger.h"
 
 namespace xprofiler {
-using Nan::AddGCEpilogueCallback;
-using Nan::AddGCPrologueCallback;
 using v8::GCType;
 using v8::Isolate;
 
@@ -33,9 +31,6 @@ uint32_t TotalGcDuration() {
 // gc prologue hook
 NAN_GC_CALLBACK(GCPrologueCallback) {
   EnvironmentData* env_data = EnvironmentData::GetCurrent(isolate);
-  if (env_data == nullptr) {
-    return;
-  }
   GcStatistics* gc_statistics = env_data->gc_statistics();
   Mutex::ScopedLock lock(gc_statistics->mutex);
   gc_statistics->start = uv_hrtime();
@@ -44,9 +39,6 @@ NAN_GC_CALLBACK(GCPrologueCallback) {
 // gc epilogue hook
 NAN_GC_CALLBACK(GCEpilogueCallback) {
   EnvironmentData* env_data = EnvironmentData::GetCurrent(isolate);
-  if (env_data == nullptr) {
-    return;
-  }
   GcStatistics* gc_statistics = env_data->gc_statistics();
   Mutex::ScopedLock lock(gc_statistics->mutex);
 
@@ -86,9 +78,9 @@ NAN_GC_CALLBACK(GCEpilogueCallback) {
   }
 }
 
-void InitGcStatusHooks() {
-  AddGCPrologueCallback(GCPrologueCallback);
-  AddGCEpilogueCallback(GCEpilogueCallback);
+void InitGcStatusHooks(EnvironmentData* env_data) {
+  env_data->AddGCPrologueCallback(GCPrologueCallback);
+  env_data->AddGCEpilogueCallback(GCEpilogueCallback);
 }
 
 void WriteGcStatusToLog(EnvironmentData* env_data, bool log_format_alinode) {

diff --git a/src/logbypass/gc.h b/src/logbypass/gc.h
@@ -35,7 +35,7 @@ class GcStatistics {
   }
 };
 
-void InitGcStatusHooks();
+void InitGcStatusHooks(EnvironmentData* env_data);
 void WriteGcStatusToLog(EnvironmentData* env_data, bool log_format_alinode);
 
 uint32_t TotalGcTimes();

diff --git a/src/logbypass/log.cc b/src/logbypass/log.cc
@@ -102,14 +102,19 @@ void LogByPass::Write(EnvironmentData* env_data, bool log_format_alinode) {
 }
 
 void RunLogBypass(const FunctionCallbackInfo<Value>& info) {
+  EnvironmentData* env_data = EnvironmentData::GetCurrent(info);
   // init gc hooks
-  InitGcStatusHooks();
+  InitGcStatusHooks(env_data);
   Info("init", "logbypass: gc hooks setted.");
 
   // init log thread
-  ProcessData::Get()->log_by_pass = std::unique_ptr<LogByPass>(new LogByPass());
-  ProcessData::Get()->log_by_pass->StartIfNeeded();
-  Info("init", "logbypass: log thread created.");
+  ProcessData* data = ProcessData::Get();
+  Mutex::ScopedLock lock(data->mutex);
+  if (data->log_by_pass == nullptr) {
+    data->log_by_pass = std::unique_ptr<LogByPass>(new LogByPass());
+    data->log_by_pass->StartIfNeeded();
+    Info("init", "logbypass: log thread created.");
+  }
 
   info.GetReturnValue().Set(True());
 }

diff --git a/src/logger.cc b/src/logger.cc
@@ -12,6 +12,7 @@
 #include "platform/platform.h"
 #include "util.h"
 #include "uv.h"
+#include "xpf_mutex-inl.h"
 
 namespace xprofiler {
 using Nan::FunctionCallbackInfo;
@@ -29,7 +30,7 @@ static const int kMaxMessageLength = 2048;
 static const int kMaxFormatLength = 2048;
 
 namespace per_process {
-uv_mutex_t logger_mutex;
+Mutex logger_mutex;
 }
 
 static void WriteToFile(const LOG_LEVEL output_level, char* log) {
@@ -61,12 +62,11 @@ static void WriteToFile(const LOG_LEVEL output_level, char* log) {
       UNREACHABLE();
   }
 
-  uv_mutex_lock(&per_process::logger_mutex);
   {
+    Mutex::ScopedLock lock(per_process::logger_mutex);
     std::ofstream ostream(filepath, std::ios::app);
     ostream << log;
   }
-  uv_mutex_unlock(&per_process::logger_mutex);
 }
 
 static void Log(const LOG_LEVEL output_level, const char* type,
@@ -150,10 +150,6 @@ static void Log(const LOG_LEVEL output_level, const char* type,
   }
 }
 
-void InitOnceLogger() {
-  CHECK_EQ(uv_mutex_init(&per_process::logger_mutex), 0);
-}
-
 /* native logger */
 
 #define NATIVE_LOGGERS(V) \

diff --git a/src/logger.h b/src/logger.h
@@ -9,8 +9,6 @@ namespace xprofiler {
 enum LOG_LEVEL { LOG_INFO, LOG_ERROR, LOG_DEBUG };
 enum LOG_TYPE { LOG_TO_FILE, LOG_TO_TTY };
 
-void InitOnceLogger();
-
 // normal external
 void Info(const char* component, const char* format, ...);
 void Error(const char* component, const char* format, ...);

diff --git a/src/process_data.h b/src/process_data.h
@@ -4,6 +4,7 @@
 #include "environment_data.h"
 #include "environment_registry.h"
 #include "logbypass/log.h"
+#include "xpf_mutex-inl.h"
 
 namespace xprofiler {
 
@@ -28,6 +29,7 @@ class ProcessData {
     return &environment_registry_;
   };
   std::unique_ptr<LogByPass> log_by_pass;
+  Mutex mutex;
 
  private:
   EnvironmentRegistry environment_registry_;

diff --git a/src/xprofiler.cc b/src/xprofiler.cc
@@ -21,7 +21,6 @@ using v8::String;
 NODE_C_CTOR(Main) {
   // init global variables
   InitOnceLoadTime();
-  InitOnceLogger();
 }
 
 #define CREATE_JS_BINDING(js_func, native_func)       \

diff --git a/xprofiler.js b/xprofiler.js
@@ -70,17 +70,19 @@ function start(config = {}) {
     const logdir = finalConfig.log_dir;
     clean(logdir);
     utils.setLogDirToFile(logdir);
-
     if (process.env.XPROFILER_UNIT_TEST_SINGLE_MODULE !== 'YES') {
-      // start performance log thread
-      exports.runLogBypass();
-      // start commands listener thread
+      // start commands listener thread if needed
       exports.runCommandsListener();
-      // set hooks
-      exports.setHooks();
     }
   }
 
+  if (process.env.XPROFILER_UNIT_TEST_SINGLE_MODULE !== 'YES') {
+    // start performance log thread if needed
+    exports.runLogBypass();
+    // set hooks
+    exports.setHooks();
+  }
+
   // patch modules
   patch(finalConfig, {
     // http status