fix semantics for conditional locking

this switches the core lock to a recursive timed mutex, uses a `try_lock_for` in `CoreSuspenderBase` and changes `help_helper` and `get_commands` over to using conditional locks to avoid a hang in the event that a core lock cannot be acquired in these methods i also moved some repeated code into protected/private methods in both `CoreSuspenderBase` and `CoreSuspender` to mitigate DRY, which also massively improves readability h/t to @NicksWorld for their help in figuring out the required semantics Co-Authored-By: Nicholas McDaniel <[email protected]>
DFHack · Dec 3, 2024 · 63f983e · 63f983e
1 parent 029d497
commit 63f983e
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 32 deletions.
diff --git a/library/Core.cpp b/library/Core.cpp
@@ -439,10 +439,9 @@ bool is_builtin(color_ostream &con, const std::string &command) {
 }
 
 void get_commands(color_ostream &con, std::vector<std::string> &commands) {
-    // FIXME: this should be a conditional lock but that functionality doesn't work yet
-    CoreSuspender suspend{};
+    CoreSuspender suspend{std::defer_lock};
 
-    if (!suspend.owns_lock()) {
+    if (!suspend.try_lock()) {
         con.printerr("Cannot acquire core lock in helpdb.get_commands\n");
         commands.clear();
         return;
@@ -646,10 +645,9 @@ static std::string sc_event_name (state_change_event id) {
 }
 
 void help_helper(color_ostream &con, const std::string &entry_name) {
-    // FIXME: this should be a conditional lock but that functionality doesn't work yet
-    CoreSuspender suspend{ };
+    CoreSuspender suspend{ std::defer_lock };
 
-    if (!suspend.owns_lock()) {
+    if (!suspend.try_lock()) {
         con.printerr("Failed Lua call to helpdb.help (could not acquire core lock).\n");
         return;
     }

diff --git a/library/include/Core.h b/library/include/Core.h
@@ -340,7 +340,7 @@ namespace DFHack
          * \sa DFHack::CoreSuspender
          * \{
          */
-        std::recursive_mutex CoreSuspendMutex;
+        std::recursive_timed_mutex CoreSuspendMutex;
         std::condition_variable_any CoreWakeup;
         std::atomic<std::thread::id> ownerThread;
         std::atomic<size_t> toolCount;
@@ -372,23 +372,14 @@ namespace DFHack
     public:
         void lock()
         {
-            auto& core = Core::getInstance();
             parent_t::lock();
-            tid = core.ownerThread.exchange(std::this_thread::get_id(),
-                    std::memory_order_acquire);
-            lock_count++;
+            complete_lock();
         }
 
         bool try_lock()
         {
-            auto& core = Core::getInstance();
-            bool locked = parent_t::try_lock();
-            if (locked)
-            {
-                tid = core.ownerThread.exchange(std::this_thread::get_id(),
-                    std::memory_order_acquire);
-                lock_count++;
-            }
+            bool locked = parent_t::try_lock_for(std::chrono::milliseconds(100));
+            if (locked) complete_lock();
             return locked;
         }
 
@@ -413,6 +404,16 @@ namespace DFHack
             while (lock_count > 0)
                 unlock();
         }
+
+    private:
+        void complete_lock()
+        {
+            auto& core = Core::getInstance();
+            tid = core.ownerThread.exchange(std::this_thread::get_id(),
+                std::memory_order_acquire);
+            lock_count++;
+        }
+
         friend class MainThread;
     };
 
@@ -457,37 +458,48 @@ namespace DFHack
 
         void lock()
         {
-            auto& core = Core::getInstance();
-            core.toolCount.fetch_add(1, std::memory_order_relaxed);
+            inc_tool_count();
             parent_t::lock();
         }
 
         bool try_lock()
         {
-            auto& core = Core::getInstance();
-            bool locked = parent_t::try_lock();
-            if (locked)
-                core.toolCount.fetch_add(1, std::memory_order_relaxed);
-            return locked;
+            inc_tool_count();
+            if (parent_t::try_lock())
+                return true;
+            dec_tool_count();
+            return false;
         }
 
         void unlock()
         {
-            auto& core = Core::getInstance();
             parent_t::unlock();
+            dec_tool_count();
+        }
+
+        ~CoreSuspender() {
+            while (lock_count > 0)
+                unlock();
+        }
+
+    protected:
+        void inc_tool_count()
+        {
+            auto& core = Core::getInstance();
+            core.toolCount.fetch_add(1, std::memory_order_relaxed);
+        }
+
+        void dec_tool_count()
+        {
             /* Notify core to continue when all queued tools have completed
              * 0 = None wants to own the core
              * 1+ = There are tools waiting core access
              * fetch_add returns old value before subtraction
              */
+            auto& core = Core::getInstance();
             if (core.toolCount.fetch_add(-1, std::memory_order_relaxed) == 1)
                 core.CoreWakeup.notify_one();
         }
-
-        ~CoreSuspender() {
-            while (lock_count > 0)
-                unlock();
-        }
     };
 
     /*!