@sentry-internal/node-native-stacktrace - npm Package Compare versions

Comparing version 0.4.0 to 0.4.1

module.cc

@@ -41,13 +41,29 @@ #include <chrono>
 struct ThreadInfo {
-  // Thread name
+  // Mutex protecting this thread's mutable data (poll_state, last_seen)
+  // Does NOT protect async_store (immutable after creation)
+  mutable std::mutex mutex;
+  // Thread name (immutable after creation)
   std::string thread_name;
   // Last time this thread was seen in milliseconds since epoch
-  milliseconds last_seen;
+  milliseconds last_seen; // Protected by mutex
   // Optional async local storage associated with this thread
-  std::optional<AsyncLocalStorageLookup> async_store;
+  // Using shared_ptr to safely share with async tasks even if ThreadInfo is
+  // erased
+  std::shared_ptr<std::optional<AsyncLocalStorageLookup>> async_store;
   // Some JSON serialized state sent via threadPoll
-  std::string poll_state;
+  std::string poll_state; // Protected by mutex
+
+  // Constructor needed because std::mutex is not movable/copyable
+  ThreadInfo(std::string name, milliseconds seen,
+             std::shared_ptr<std::optional<AsyncLocalStorageLookup>> store,
+             std::string state)
+      : thread_name(std::move(name)), last_seen(seen),
+        async_store(std::move(store)), poll_state(std::move(state)) {}
 };
 
-static std::mutex threads_mutex;
+// Separate mutexes for different concerns:
+// - threads_map_mutex: protects the threads map structure
+// (insert/erase/iteration)
+// - ThreadInfo::mutex: protects each thread's mutable data
+static std::mutex threads_map_mutex;
 // Map to hold all registered threads and their information
@@ -320,3 +336,3 @@ static std::unordered_map<v8::Isolate *, ThreadInfo> threads = {};
   std::promise<JsStackTrace> promise;
-  const std::optional<AsyncLocalStorageLookup> *store;
+  std::shared_ptr<std::optional<AsyncLocalStorageLookup>> store;
 };
@@ -351,5 +367,5 @@
 // Function to capture the stack trace of a single isolate
-JsStackTrace
-CaptureStackTrace(Isolate *isolate,
-                  const std::optional<AsyncLocalStorageLookup> &store) {
+JsStackTrace CaptureStackTrace(
+    Isolate *isolate,
+    const std::shared_ptr<std::optional<AsyncLocalStorageLookup>> &store) {
   if (isolate->IsExecutionTerminating()) {
@@ -363,5 +379,17 @@ return JsStackTrace{{}, ""};
   // The v8 isolate must be interrupted to capture the stack trace
+  // Note: Even if we timeout below, the interrupt may still fire later.
+  // The InterruptArgs holds a shared_ptr to keep data alive until the callback
+  // executes.
   isolate->RequestInterrupt(ExecutionInterrupted,
-                            new InterruptArgs{std::move(promise), &store});
+                            new InterruptArgs{std::move(promise), store});
 
+  // Wait with timeout to prevent infinite hang if isolate never processes
+  // interrupt (e.g., stuck in native code or terminating)
+  if (future.wait_for(std::chrono::seconds(5)) == std::future_status::timeout) {
+    // Timeout occurred. The InterruptArgs is intentionally leaked - it will be
+    // deleted by ExecutionInterrupted if/when the callback eventually fires.
+    // The shared_ptr keeps the store data alive.
+    return JsStackTrace{{}, ""};
+  }
+
   return future.get();
@@ -376,5 +404,7 @@ }
 
+  std::vector<std::future<ThreadResult>> futures;
   {
-    std::vector<std::future<ThreadResult>> futures;
-    std::lock_guard<std::mutex> lock(threads_mutex);
+    // Only need map lock to safely iterate and copy shared_ptrs
+    // No deadlock risk because we release lock before fut.get()
+    std::lock_guard<std::mutex> lock(threads_map_mutex);
     for (auto &thread : threads) {
@@ -387,20 +417,29 @@ auto thread_isolate = thread.first;
 
+      // Copy immutable data and shared_ptrs (no individual thread lock needed)
+      // thread_name and async_store are immutable after creation
       auto thread_name = thread_info.thread_name;
-      auto poll_state = thread_info.poll_state;
+      auto async_store_ptr = thread_info.async_store;
 
+      // For poll_state, we need to lock the thread's mutex briefly
+      std::string poll_state;
+      {
+        std::lock_guard<std::mutex> thread_lock(thread_info.mutex);
+        poll_state = thread_info.poll_state;
+      }
+
       futures.emplace_back(std::async(
           std::launch::async,
-          [thread_isolate, thread_name, poll_state](
-              const std::optional<AsyncLocalStorageLookup> &async_store)
-              -> ThreadResult {
-            return ThreadResult{thread_name,
-                                CaptureStackTrace(thread_isolate, async_store),
-                                poll_state};
-          },
-          std::cref(thread_info.async_store)));
+          [thread_isolate, thread_name, poll_state,
+           async_store_ptr]() -> ThreadResult {
+            return ThreadResult{
+                thread_name, CaptureStackTrace(thread_isolate, async_store_ptr),
+                poll_state};
+          }));
     }
+  }
 
-    for (auto &fut : futures) {
-      results.emplace_back(fut.get());
-    }
+  // Wait for all futures to complete AFTER releasing the lock
+  // No deadlock because ThreadPoll uses a different lock (thread's own mutex)
+  for (auto &fut : futures) {
+    results.emplace_back(fut.get());
   }
@@ -511,3 +550,3 @@
   auto isolate = static_cast<Isolate *>(arg);
-  std::lock_guard<std::mutex> lock(threads_mutex);
+  std::lock_guard<std::mutex> lock(threads_map_mutex);
   threads.erase(isolate);
@@ -520,7 +559,13 @@ }
 
-  std::lock_guard<std::mutex> lock(threads_mutex);
-  auto found = threads.find(isolate);
-  if (found == threads.end()) {
-    threads.emplace(isolate, ThreadInfo{thread_name, milliseconds::zero(),
-                                        std::move(async_store), ""});
+  std::lock_guard<std::mutex> lock(threads_map_mutex);
+  // try_emplace constructs the ThreadInfo in-place if the key doesn't exist
+  // The mutex will be default-constructed automatically by ThreadInfo
+  // constructor
+  auto [iter, inserted] = threads.try_emplace(
+      isolate, thread_name, milliseconds::zero(),
+      std::make_shared<std::optional<AsyncLocalStorageLookup>>(
+          std::move(async_store)),
+      "");
+
+  if (inserted) {
     // Register a cleanup hook to remove this thread when the isolate is
@@ -657,15 +702,24 @@ // destroyed
 
+  // First, find the thread without holding any lock (map reads are safe)
+  // Then lock only that specific thread's mutex (not the global map mutex)
+  ThreadInfo *thread_info_ptr = nullptr;
   {
-    std::lock_guard<std::mutex> lock(threads_mutex);
+    std::lock_guard<std::mutex> map_lock(threads_map_mutex);
     auto found = threads.find(isolate);
     if (found != threads.end()) {
-      auto &thread_info = found->second;
-      thread_info.poll_state = std::move(poll_state);
+      thread_info_ptr = &found->second;
+    }
+  }
 
-      if (enable_last_seen) {
-        thread_info.last_seen = duration_cast<milliseconds>(
-            GetUnbiasedMonotonicTime().time_since_epoch());
-      } else {
-        thread_info.last_seen = milliseconds::zero();
-      }
+  // Update thread-specific data with only the thread's mutex held
+  // This prevents deadlock with CaptureStackTraces which holds map mutex
+  if (thread_info_ptr != nullptr) {
+    std::lock_guard<std::mutex> thread_lock(thread_info_ptr->mutex);
+    thread_info_ptr->poll_state = std::move(poll_state);
+
+    if (enable_last_seen) {
+      thread_info_ptr->last_seen = duration_cast<milliseconds>(
+          GetUnbiasedMonotonicTime().time_since_epoch());
+    } else {
+      thread_info_ptr->last_seen = milliseconds::zero();
     }
@@ -683,8 +737,15 @@ }
   {
-    std::lock_guard<std::mutex> lock(threads_mutex);
+    std::lock_guard<std::mutex> map_lock(threads_map_mutex);
     for (const auto &[thread_isolate, info] : threads) {
-      if (info.last_seen == milliseconds::zero())
+      // Lock each thread's mutex briefly to read last_seen
+      milliseconds last_seen;
+      {
+        std::lock_guard<std::mutex> thread_lock(info.mutex);
+        last_seen = info.last_seen;
+      }
+
+      if (last_seen == milliseconds::zero())
         continue; // Skip threads that have not registered more than once
 
-      int64_t ms_since = (now - info.last_seen).count();
+      int64_t ms_since = (now - last_seen).count();
       result
@@ -691,0 +752,0 @@ ->Set(isolate->GetCurrentContext(),

package.json

 {
   "name": "@sentry-internal/node-native-stacktrace",
-  "version": "0.4.0",
+  "version": "0.4.1",
   "main": "lib/index.js",
@@ -5,0 +5,0 @@ "types": "lib/index.d.ts",

New alerts

AI-detected potential code anomaly

Supply chain risk

AI has identified unusual behaviors that may pose a security risk.

Fixed alerts

AI-detected potential code anomaly

Supply chain risk

AI has identified unusual behaviors that may pose a security risk.

Improved metrics

Total package byte prevSize

2903099

3.08%

No dependency changes