diff --git a/library/Core.cpp b/library/Core.cpp index ca9199c56c..0e183a9878 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,7 +439,14 @@ bool is_builtin(color_ostream &con, const std::string &command) { } void get_commands(color_ostream &con, std::vector &commands) { - CoreSuspender suspend; + ConditionalCoreSuspender suspend{}; + + if (!suspend) { + con.printerr("Cannot acquire core lock in helpdb.get_commands\n"); + commands.clear(); + return; + } + auto L = Lua::Core::State; Lua::StackUnwinder top(L); @@ -460,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); @@ -641,7 +646,12 @@ static std::string sc_event_name (state_change_event id) { } void help_helper(color_ostream &con, const std::string &entry_name) { - CoreSuspender suspend; + ConditionalCoreSuspender suspend{}; + + if (!suspend) { + con.printerr("Failed Lua call to helpdb.help (could not acquire core lock).\n"); + return; + } auto L = Lua::Core::State; Lua::StackUnwinder top(L); @@ -2396,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/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 88ed02ce4d..dc7c414605 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 @@ -339,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; @@ -353,31 +354,41 @@ 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; + using mutex_type = decltype(Core::CoreSuspendMutex); + using parent_t = std::unique_lock< mutex_type >; std::thread::id tid; + Core& core; - CoreSuspenderBase(std::defer_lock_t d) : CoreSuspenderBase{&Core::getInstance(), d} {} + CoreSuspenderBase() : CoreSuspenderBase{ Core::getInstance() } {} - CoreSuspenderBase(Core* core, std::defer_lock_t) : - /* Lock the core */ - parent_t{core->CoreSuspendMutex,std::defer_lock}, + CoreSuspenderBase(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: void lock() { - auto& core = Core::getInstance(); parent_t::lock(); - tid = core.ownerThread.exchange(std::this_thread::get_id(), - std::memory_order_acquire); + complete_lock(); + } + + bool try_lock() + { + bool locked = parent_t::try_lock_for(std::chrono::milliseconds(100)); + if (locked) complete_lock(); + return locked; } void unlock() { - auto& core = Core::getInstance(); + if (!owns_lock()) + return; /* Restore core owner to previous value */ core.ownerThread.store(tid, std::memory_order_release); if (tid == std::thread::id{}) @@ -385,15 +396,18 @@ namespace DFHack parent_t::unlock(); } - bool owns_lock() const noexcept - { - return parent_t::owns_lock(); + ~CoreSuspenderBase() { + unlock(); } - ~CoreSuspenderBase() { - if (owns_lock()) - unlock(); + protected: + void complete_lock() + { + auto& core = Core::getInstance(); + tid = core.ownerThread.exchange(std::this_thread::get_id(), + std::memory_order_acquire); } + friend class MainThread; }; @@ -419,34 +433,61 @@ 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} - {} + + // note that this is needed so the destructor will call CoreSuspender::unlock instead of CoreSuspenderBase::unlock + ~CoreSuspender() { + unlock(); + } + + protected: + // deferred locking is not part of CoreSuspender's public API + // 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 } {} void lock() { - auto& core = Core::getInstance(); - core.toolCount.fetch_add(1, std::memory_order_relaxed); + inc_tool_count(); parent_t::lock(); } + bool try_lock() + { + 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(); + } + + void inc_tool_count() + { + 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 @@ -455,11 +496,24 @@ namespace DFHack if (core.toolCount.fetch_add(-1, std::memory_order_relaxed) == 1) core.CoreWakeup.notify_one(); } + }; - ~CoreSuspender() { - if (owns_lock()) - unlock(); - } + /*! + * 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(); } }; /*! @@ -483,4 +537,5 @@ namespace DFHack }; using CoreSuspendClaimer = CoreSuspender; + }