From ca31a280a29ca1fb24864de6cbb18c82ea0a32d3 Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Mon, 2 Dec 2024 21:55:46 -0600 Subject: [PATCH 1/7] adjustments to core suspender this is an attempted step to improve semantics around core suspenders. this does not actually resolve #5074 but it is intended as a step toward doing so --- library/Core.cpp | 18 ++++++++++++++++-- library/include/Core.h | 41 +++++++++++++++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index ca9199c56c..7900faa346 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -439,7 +439,15 @@ bool is_builtin(color_ostream &con, const std::string &command) { } void get_commands(color_ostream &con, std::vector &commands) { - CoreSuspender suspend; + // FIXME: this should be a conditional lock but that functionality doesn't work yet + CoreSuspender suspend{}; + + if (!suspend.owns_lock()) { + con.printerr("Cannot acquire core lock in helpdb.get_commands\n"); + commands.clear(); + return; + } + auto L = Lua::Core::State; Lua::StackUnwinder top(L); @@ -641,7 +649,13 @@ static std::string sc_event_name (state_change_event id) { } void help_helper(color_ostream &con, const std::string &entry_name) { - CoreSuspender suspend; + // FIXME: this should be a conditional lock but that functionality doesn't work yet + CoreSuspender suspend{ }; + + if (!suspend.owns_lock()) { + con.printerr("Failed Lua call to helpdb.help (could not acquire core lock).\n"); + return; + } auto L = Lua::Core::State; Lua::StackUnwinder top(L); diff --git a/library/include/Core.h b/library/include/Core.h index 88ed02ce4d..9671cb1892 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -31,6 +31,7 @@ distribution. #include "modules/Graphic.h" #include +#include #include #include #include @@ -353,15 +354,17 @@ namespace DFHack friend struct CoreSuspendReleaseMain; }; - class CoreSuspenderBase : protected std::unique_lock { + class CoreSuspenderBase : protected std::unique_lock< decltype(Core::CoreSuspendMutex) > { protected: - using parent_t = std::unique_lock; + size_t lock_count = 0; + using mutex_type = decltype(Core::CoreSuspendMutex); + using parent_t = std::unique_lock< mutex_type >; std::thread::id tid; CoreSuspenderBase(std::defer_lock_t d) : CoreSuspenderBase{&Core::getInstance(), d} {} CoreSuspenderBase(Core* core, std::defer_lock_t) : - /* Lock the core */ + /* Lock the core (jk not really) */ parent_t{core->CoreSuspendMutex,std::defer_lock}, /* Mark this thread to be the core owner */ tid{} @@ -373,6 +376,20 @@ namespace DFHack parent_t::lock(); tid = core.ownerThread.exchange(std::this_thread::get_id(), std::memory_order_acquire); + lock_count++; + } + + 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++; + } + return locked; } void unlock() @@ -383,15 +400,17 @@ namespace DFHack if (tid == std::thread::id{}) Lua::Core::Reset(core.getConsole(), "suspend"); parent_t::unlock(); + lock_count--; } bool owns_lock() const noexcept { - return parent_t::owns_lock(); + return lock_count > 0; +// return parent_t::owns_lock(); } ~CoreSuspenderBase() { - if (owns_lock()) + while (lock_count > 0) unlock(); } friend class MainThread; @@ -443,6 +462,15 @@ namespace DFHack 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; + } + void unlock() { auto& core = Core::getInstance(); @@ -457,7 +485,7 @@ namespace DFHack } ~CoreSuspender() { - if (owns_lock()) + while (lock_count > 0) unlock(); } }; @@ -483,4 +511,5 @@ namespace DFHack }; using CoreSuspendClaimer = CoreSuspender; + } From 0f7e1088c5726184e1541e99617ff5341d7f8d35 Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Tue, 3 Dec 2024 11:49:36 -0600 Subject: [PATCH 2/7] 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 <29175042+NicksWorld@users.noreply.github.com> --- library/Core.cpp | 10 +++---- library/include/Core.h | 64 +++++++++++++++++++++++++----------------- 2 files changed, 42 insertions(+), 32 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index 7900faa346..443840ae8c 100644 --- 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 &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; @@ -649,10 +648,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 index 9671cb1892..765f4adc36 100644 --- 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 ownerThread; std::atomic 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(); - } }; /*! From 73ab644dc9e923f1230b888cc4e5dc769ea67398 Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Thu, 5 Dec 2024 21:04:34 -0600 Subject: [PATCH 3/7] revert #5080, reinstating try_autocomplete --- library/Core.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index 443840ae8c..9aa015ca6e 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -467,9 +467,7 @@ static bool try_autocomplete(color_ostream &con, const std::string &first, std:: { std::vector commands, possible; - // restore call to get_commands once we have updated the core lock to a deferred lock - // so calling Lua from the console thread won't deadlock if Lua is currently busy - //get_commands(con, commands); + get_commands(con, commands); for (auto &command : commands) if (command.substr(0, first.size()) == first) possible.push_back(command); From e1866c2e73f302ec00c2e412eed7e40cc4c127bc Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Fri, 13 Dec 2024 20:12:09 -0600 Subject: [PATCH 4/7] rework CoreSuspender API Drastically simplify CoreSuspender API: * remove access to state manipulation * add ConditionalCoreSuspender subclass for conditional locks * fix various errors around use of an explicit core argument when creating a CoreSuspender --- library/Core.cpp | 10 ++--- library/PluginManager.cpp | 2 +- library/include/Core.h | 81 ++++++++++++++++++++++++--------------- 3 files changed, 57 insertions(+), 36 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index 9aa015ca6e..c4795c09ea 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -113,7 +113,7 @@ class MainThread { //! MainThread::suspend keeps the main DF thread suspended from Core::Init to //! thread exit. static CoreSuspenderBase& suspend() { - static thread_local CoreSuspenderBase lock(std::defer_lock); + static thread_local CoreSuspenderBase lock{}; return lock; } }; @@ -439,9 +439,9 @@ bool is_builtin(color_ostream &con, const std::string &command) { } void get_commands(color_ostream &con, std::vector &commands) { - CoreSuspender suspend{std::defer_lock}; + ConditionalCoreSuspender suspend{}; - if (!suspend.try_lock()) { + if (!suspend) { con.printerr("Cannot acquire core lock in helpdb.get_commands\n"); commands.clear(); return; @@ -646,9 +646,9 @@ static std::string sc_event_name (state_change_event id) { } void help_helper(color_ostream &con, const std::string &entry_name) { - CoreSuspender suspend{ std::defer_lock }; + ConditionalCoreSuspender suspend{}; - if (!suspend.try_lock()) { + if (!suspend) { con.printerr("Failed Lua call to helpdb.help (could not acquire core lock).\n"); return; } diff --git a/library/PluginManager.cpp b/library/PluginManager.cpp index 18d08ae6c0..c79b4a0ce0 100644 --- a/library/PluginManager.cpp +++ b/library/PluginManager.cpp @@ -488,7 +488,7 @@ command_result Plugin::invoke(color_ostream &out, const std::string & command, s // expect their guard conditions to be matched, // so as to avoid duplicating checks. // This means suspending the core beforehand. - CoreSuspender suspend(&c); + CoreSuspender suspend(c); df::viewscreen *top = c.getTopViewscreen(); if (!cmd.guard(top)) diff --git a/library/include/Core.h b/library/include/Core.h index 765f4adc36..ff0d56837a 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -356,19 +356,22 @@ namespace DFHack class CoreSuspenderBase : protected std::unique_lock< decltype(Core::CoreSuspendMutex) > { protected: - size_t lock_count = 0; + Core& core; + bool isLocked = false; using mutex_type = decltype(Core::CoreSuspendMutex); using parent_t = std::unique_lock< mutex_type >; std::thread::id tid; - CoreSuspenderBase(std::defer_lock_t d) : CoreSuspenderBase{&Core::getInstance(), d} {} + CoreSuspenderBase() : CoreSuspenderBase{ Core::getInstance() } {} - CoreSuspenderBase(Core* core, std::defer_lock_t) : + CoreSuspenderBase(Core& core) : + core{ core }, /* Lock the core (jk not really) */ - parent_t{core->CoreSuspendMutex,std::defer_lock}, + parent_t{core.CoreSuspendMutex,std::defer_lock}, /* Mark this thread to be the core owner */ tid{} {} + public: void lock() { @@ -385,33 +388,32 @@ namespace DFHack void unlock() { - auto& core = Core::getInstance(); + if (!isLocked) + return; /* Restore core owner to previous value */ core.ownerThread.store(tid, std::memory_order_release); if (tid == std::thread::id{}) Lua::Core::Reset(core.getConsole(), "suspend"); parent_t::unlock(); - lock_count--; + isLocked = false; } bool owns_lock() const noexcept { - return lock_count > 0; -// return parent_t::owns_lock(); + return isLocked; } ~CoreSuspenderBase() { - while (lock_count > 0) - unlock(); + if (isLocked) unlock(); } - private: + protected: void complete_lock() { auto& core = Core::getInstance(); tid = core.ownerThread.exchange(std::this_thread::get_id(), std::memory_order_acquire); - lock_count++; + isLocked = true; } friend class MainThread; @@ -439,22 +441,31 @@ namespace DFHack * The last step is to decrement Core::toolCount and wakeup main thread if * no more tools are queued trying to acquire the * Core::CoreSuspenderMutex. + * + * The public API for CoreSuspender only supports construction and destruction; + * all other locking operations are reserved */ - class CoreSuspender : public CoreSuspenderBase { + + class CoreSuspender : protected CoreSuspenderBase { using parent_t = CoreSuspenderBase; public: - CoreSuspender() : CoreSuspender{&Core::getInstance()} { } - CoreSuspender(std::defer_lock_t d) : CoreSuspender{&Core::getInstance(),d} { } - CoreSuspender(bool) : CoreSuspender{&Core::getInstance()} { } - CoreSuspender(Core* core, bool) : CoreSuspender{core} { } - CoreSuspender(Core* core) : - CoreSuspenderBase{core, std::defer_lock} + CoreSuspender() : CoreSuspender{Core::getInstance()} { } + + CoreSuspender(Core& core) : CoreSuspenderBase{core} { lock(); } - CoreSuspender(Core* core, std::defer_lock_t) : - CoreSuspenderBase{core, std::defer_lock} - {} + + ~CoreSuspender() { + if (owns_lock()) unlock(); + } + + protected: + // deferred locking is not part of CoreSuspender's public API + // these constructors are onmly for use in derived classes, + // specifically ConditionalCoreSuspender + CoreSuspender(std::defer_lock_t d) : CoreSuspender{ Core::getInstance(),d } {} + CoreSuspender(Core& core, std::defer_lock_t d) : CoreSuspenderBase{ core } {} void lock() { @@ -477,15 +488,8 @@ namespace DFHack 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); } @@ -496,12 +500,29 @@ namespace DFHack * 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(); } }; + /*! + * ConditionalCoreSuspender attempts to acquire a CoreSuspender, but will fail if doing so + * would cause a thread wait. The caller can determine if the suspender was acquired by casting + * the created object to bool: + * ConditionalCoreSuspender suspend; + * if (suspend) { + * ... + * } + */ + + class ConditionalCoreSuspender : protected CoreSuspender { + public: + ConditionalCoreSuspender() : ConditionalCoreSuspender{ Core::getInstance() } { } + ConditionalCoreSuspender(Core& core) : CoreSuspender{ core, std::defer_lock } { try_lock(); } + + operator bool() const { return owns_lock(); } + }; + /*! * Temporary release main thread ownership to allow alternative thread * implement DF logic thread loop From 36fa4a6218ff7053f7cdcbe513b33d7f5dccf117 Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Fri, 13 Dec 2024 20:21:07 -0600 Subject: [PATCH 5/7] reorder constructor to satisfy gcc's order warning i will shortly submit a PR to add `/w15038` to the msvc configuration so msvc will also catch these warnings going forward --- library/include/Core.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/include/Core.h b/library/include/Core.h index ff0d56837a..78c84557b2 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -356,20 +356,20 @@ namespace DFHack class CoreSuspenderBase : protected std::unique_lock< decltype(Core::CoreSuspendMutex) > { protected: - Core& core; bool isLocked = false; using mutex_type = decltype(Core::CoreSuspendMutex); using parent_t = std::unique_lock< mutex_type >; std::thread::id tid; + Core& core; CoreSuspenderBase() : CoreSuspenderBase{ Core::getInstance() } {} CoreSuspenderBase(Core& core) : - core{ core }, /* Lock the core (jk not really) */ parent_t{core.CoreSuspendMutex,std::defer_lock}, /* Mark this thread to be the core owner */ - tid{} + tid{}, + core{ core } {} public: From ff6d7a2c00fcf5022f2f2e2c6da5236936794ad9 Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Fri, 13 Dec 2024 21:53:04 -0600 Subject: [PATCH 6/7] minor improvement removed the local `isLocked` member from `CoreSuspenderBase` as it is redundant with `owns_lock()` from `std::unique_lock` h/t to @NicksWorld for pointing this out Co-Authored-By: Nicholas McDaniel <29175042+NicksWorld@users.noreply.github.com> --- library/Core.cpp | 3 +-- library/include/Core.h | 12 ++---------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index c4795c09ea..0e183a9878 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -2406,8 +2406,7 @@ int Core::Shutdown ( void ) errorstate = 1; // Make sure we release main thread if this is called from main thread - if (MainThread::suspend().owns_lock()) - MainThread::suspend().unlock(); + MainThread::suspend().unlock(); // Make sure the console thread shutdowns before clean up to avoid any // unlikely data races. diff --git a/library/include/Core.h b/library/include/Core.h index 78c84557b2..c0817ee4f4 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -356,7 +356,6 @@ namespace DFHack class CoreSuspenderBase : protected std::unique_lock< decltype(Core::CoreSuspendMutex) > { protected: - bool isLocked = false; using mutex_type = decltype(Core::CoreSuspendMutex); using parent_t = std::unique_lock< mutex_type >; std::thread::id tid; @@ -388,23 +387,17 @@ namespace DFHack void unlock() { - if (!isLocked) + if (!owns_lock()) return; /* Restore core owner to previous value */ core.ownerThread.store(tid, std::memory_order_release); if (tid == std::thread::id{}) Lua::Core::Reset(core.getConsole(), "suspend"); parent_t::unlock(); - isLocked = false; - } - - bool owns_lock() const noexcept - { - return isLocked; } ~CoreSuspenderBase() { - if (isLocked) unlock(); + if (owns_lock()) unlock(); } protected: @@ -413,7 +406,6 @@ namespace DFHack auto& core = Core::getInstance(); tid = core.ownerThread.exchange(std::this_thread::get_id(), std::memory_order_acquire); - isLocked = true; } friend class MainThread; From 0f326c1413ec5d835a6a5f1f4ef731506f23574e Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Wed, 18 Dec 2024 16:32:14 -0600 Subject: [PATCH 7/7] simplifications no point in calling `owns_lock` twice --- library/include/Core.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/include/Core.h b/library/include/Core.h index c0817ee4f4..dc7c414605 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -397,7 +397,7 @@ namespace DFHack } ~CoreSuspenderBase() { - if (owns_lock()) unlock(); + unlock(); } protected: @@ -448,13 +448,14 @@ namespace DFHack lock(); } + // note that this is needed so the destructor will call CoreSuspender::unlock instead of CoreSuspenderBase::unlock ~CoreSuspender() { - if (owns_lock()) unlock(); + unlock(); } protected: // deferred locking is not part of CoreSuspender's public API - // these constructors are onmly for use in derived classes, + // these constructors are only for use in derived classes, // specifically ConditionalCoreSuspender CoreSuspender(std::defer_lock_t d) : CoreSuspender{ Core::getInstance(),d } {} CoreSuspender(Core& core, std::defer_lock_t d) : CoreSuspenderBase{ core } {}