[Lldb-commits] [lldb] b76fd1e - [lldb] Avoid deadlock by unlocking before invoking callbacks (#86888)

[via lldb-commits]

Author: Jonas Devlieghere
Date: 2024-03-27T16:37:00-07:00
New Revision: b76fd1eddbafaa02a533245d574048c5ad9e38fa

URL: https://github.com/llvm/llvm-project/commit/b76fd1eddbafaa02a533245d574048c5ad9e38fa
DIFF: https://github.com/llvm/llvm-project/commit/b76fd1eddbafaa02a533245d574048c5ad9e38fa.diff

LOG: [lldb] Avoid deadlock by unlocking before invoking callbacks (#86888)

Avoid deadlocks in the Alarm class by releasing the lock before invoking
callbacks. This deadlock manifested itself in the ProgressManager:

1. On the main thread, the ProgressManager acquires its lock in
ProgressManager::Decrement and calls Alarm::Create.
  2. On the main thread, the Alarm acquires its lock in Alarm::Create.
3. On the alarm thread, the Alarm acquires its lock after waiting on the
condition variable and calls ProgressManager::Expire.
4. On the alarm thread, the ProgressManager acquires its lock in
ProgressManager::Expire.

Note how the two threads are acquiring the locks in different orders.
Deadlocks can be avoided by always acquiring locks in the same order,
but since the two mutexes here are private implementation details,
belong to different classes, that's not straightforward. Luckily, we
don't need to have the Alarm mutex locked when invoking the callbacks.
That exactly how this patch solves the issue.

Added: 
    

Modified: 
    lldb/source/Host/common/Alarm.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Host/common/Alarm.cpp b/lldb/source/Host/common/Alarm.cpp
index 245cdc7ae5c2da..afc770d20d7b1e 100644
--- a/lldb/source/Host/common/Alarm.cpp
+++ b/lldb/source/Host/common/Alarm.cpp
@@ -154,54 +154,60 @@ lldb::thread_result_t Alarm::AlarmThread() {
     //
     // Below we only deal with the timeout expiring and fall through for dealing
     // with the rest.
-    std::unique_lock<std::mutex> alarm_lock(m_alarm_mutex);
-    if (next_alarm) {
-      if (!m_alarm_cv.wait_until(alarm_lock, *next_alarm, predicate)) {
-        // The timeout for the next alarm expired.
-
-        // Clear the next timeout to signal that we need to recompute the next
-        // timeout.
-        next_alarm.reset();
-
-        // Iterate over all the callbacks. Call the ones that have expired
-        // and remove them from the list.
-        const TimePoint now = std::chrono::system_clock::now();
-        auto it = m_entries.begin();
-        while (it != m_entries.end()) {
-          if (it->expiration <= now) {
-            it->callback();
-            it = m_entries.erase(it);
-          } else {
-            it++;
+    llvm::SmallVector<Callback, 1> callbacks;
+    {
+      std::unique_lock<std::mutex> alarm_lock(m_alarm_mutex);
+      if (next_alarm) {
+        if (!m_alarm_cv.wait_until(alarm_lock, *next_alarm, predicate)) {
+          // The timeout for the next alarm expired.
+
+          // Clear the next timeout to signal that we need to recompute the next
+          // timeout.
+          next_alarm.reset();
+
+          // Iterate over all the callbacks. Call the ones that have expired
+          // and remove them from the list.
+          const TimePoint now = std::chrono::system_clock::now();
+          auto it = m_entries.begin();
+          while (it != m_entries.end()) {
+            if (it->expiration <= now) {
+              callbacks.emplace_back(std::move(it->callback));
+              it = m_entries.erase(it);
+            } else {
+              it++;
+            }
           }
         }
+      } else {
+        m_alarm_cv.wait(alarm_lock, predicate);
       }
-    } else {
-      m_alarm_cv.wait(alarm_lock, predicate);
-    }
 
-    // Fall through after waiting on the condition variable. At this point
-    // either the predicate is true or we woke up because an alarm expired.
+      // Fall through after waiting on the condition variable. At this point
+      // either the predicate is true or we woke up because an alarm expired.
 
-    // The alarm thread is shutting down.
-    if (m_exit) {
-      exit = true;
-      if (m_run_callbacks_on_exit) {
-        for (Entry &entry : m_entries)
-          entry.callback();
+      // The alarm thread is shutting down.
+      if (m_exit) {
+        exit = true;
+        if (m_run_callbacks_on_exit) {
+          for (Entry &entry : m_entries)
+            callbacks.emplace_back(std::move(entry.callback));
+        }
       }
-      continue;
-    }
 
-    // A new alarm was added or an alarm expired. Either way we need to
-    // recompute when this thread should wake up for the next alarm.
-    if (m_recompute_next_alarm || !next_alarm) {
-      for (Entry &entry : m_entries) {
-        if (!next_alarm || entry.expiration < *next_alarm)
-          next_alarm = entry.expiration;
+      // A new alarm was added or an alarm expired. Either way we need to
+      // recompute when this thread should wake up for the next alarm.
+      if (m_recompute_next_alarm || !next_alarm) {
+        for (Entry &entry : m_entries) {
+          if (!next_alarm || entry.expiration < *next_alarm)
+            next_alarm = entry.expiration;
+        }
+        m_recompute_next_alarm = false;
       }
-      m_recompute_next_alarm = false;
     }
+
+    // Outside the lock, call the callbacks.
+    for (Callback &callback : callbacks)
+      callback();
   }
   return {};
 }