From cd6fe14deadd3181126cc9a49efab9da7cc4a238 Mon Sep 17 00:00:00 2001 From: Kelly Kinkade Date: Sun, 22 Dec 2024 13:10:38 -0600 Subject: [PATCH] thread safety checks also removed some dead code and fixed a problem with shutdown --- library/Core.cpp | 41 ++++++++++++++++++--------------------- library/PluginManager.cpp | 22 ++++++++++++--------- library/include/Core.h | 32 +++++++++--------------------- 3 files changed, 41 insertions(+), 54 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index d14da178117..8b6253d8f49 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -104,6 +104,7 @@ namespace DFHack { DBG_DECLARE(core,keybinding,DebugCategory::LINFO); DBG_DECLARE(core,script,DebugCategory::LINFO); +DBG_DECLARE(core,threads); static const std::string CONFIG_PATH = "dfhack-config/"; static const std::string CONFIG_DEFAULTS_PATH = "hack/data/dfhack-config-defaults/"; @@ -119,26 +120,6 @@ class MainThread { }; } -CoreSuspendReleaseMain::CoreSuspendReleaseMain() -{ - MainThread::suspend().unlock(); -} - -CoreSuspendReleaseMain::~CoreSuspendReleaseMain() -{ - MainThread::suspend().lock(); -} - -CoreSuspendClaimMain::CoreSuspendClaimMain() -{ - MainThread::suspend().lock(); -} - -CoreSuspendClaimMain::~CoreSuspendClaimMain() -{ - MainThread::suspend().unlock(); -} - struct Core::Private { std::thread iothread; @@ -1548,6 +1529,8 @@ df::viewscreen * Core::getTopViewscreen() { } bool Core::InitMainThread() { + df_render_thread = std::this_thread::get_id(); + Filesystem::init(); // Re-route stdout and stderr again - DF seems to set up stdout and @@ -1707,6 +1690,7 @@ bool Core::InitMainThread() { bool Core::InitSimulationThread() { + df_simulation_thread = std::this_thread::get_id(); if(started) return true; if(errorstate) @@ -2108,6 +2092,13 @@ void Core::doUpdate(color_ostream &out) // should always be from simulation thread! int Core::Update() { + if (shutdown) + { + // release the suspender + MainThread::suspend().unlock(); + return -1; + } + if(errorstate) return -1; @@ -2405,8 +2396,7 @@ int Core::Shutdown ( void ) return true; errorstate = 1; - // Make sure we release main thread if this is called from main thread - MainThread::suspend().unlock(); + shutdown = true; // Make sure the console thread shutdowns before clean up to avoid any // unlikely data races. @@ -2519,6 +2509,13 @@ bool Core::DFH_SDL_Event(SDL_Event* ev) { bool Core::doSdlInputEvent(SDL_Event* ev) { + // DF only calls the sdl input event hook from the render thread + // we use this to identify the render thread + // + // first time this is called, set df_render_thread to calling thread + // afterwards, assert that it's never changed + assert(std::this_thread::get_id() == df_render_thread); + static std::map hotkey_states; // do NOT process events before we are ready. diff --git a/library/PluginManager.cpp b/library/PluginManager.cpp index 270e1f12dea..240986e0c42 100644 --- a/library/PluginManager.cpp +++ b/library/PluginManager.cpp @@ -388,7 +388,7 @@ bool Plugin::unload(color_ostream &con) // get the mutex access->lock(); // if we are actually loaded - if(state == PS_LOADED) + if (state == PS_LOADED) { if (Screen::hasActiveScreens(this)) { @@ -408,14 +408,18 @@ bool Plugin::unload(color_ostream &con) // wait for all calls to finish access->wait(); state = PS_UNLOADING; - access->unlock(); - // enter suspend - CoreSuspender suspend; - access->lock(); - if (Core::getInstance().isMapLoaded() && plugin_save_site_data && World::IsSiteLoaded() && plugin_save_site_data(con) != CR_OK) - con.printerr("Plugin %s has failed to save site data.\n", name.c_str()); - if (Core::getInstance().isWorldLoaded() && plugin_save_world_data && plugin_save_world_data(con) != CR_OK) - con.printerr("Plugin %s has failed to save world data.\n", name.c_str()); + // only attempt to unload site or world data if the core is in a valid state + if (Core::getInstance().isValid()) + { + // enter suspend + access->unlock(); + CoreSuspender suspend; + access->lock(); + if (Core::getInstance().isMapLoaded() && plugin_save_site_data && World::IsSiteLoaded() && plugin_save_site_data(con) != CR_OK) + con.printerr("Plugin %s has failed to save site data.\n", name.c_str()); + if (Core::getInstance().isWorldLoaded() && plugin_save_world_data && plugin_save_world_data(con) != CR_OK) + con.printerr("Plugin %s has failed to save world data.\n", name.c_str()); + } // notify plugin about shutdown, if it has a shutdown function command_result cr = CR_OK; if(plugin_shutdown) diff --git a/library/include/Core.h b/library/include/Core.h index 3eb36b9bb74..1d3631548a8 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -344,8 +344,12 @@ namespace DFHack std::condition_variable_any CoreWakeup; std::atomic ownerThread; std::atomic toolCount; + std::atomic shutdown; //! \} + std::thread::id df_render_thread; + std::thread::id df_simulation_thread; + friend class CoreService; friend class ServerConnection; friend class CoreSuspender; @@ -369,8 +373,9 @@ namespace DFHack /* Mark this thread to be the core owner */ tid{}, core{ core } - {} - + { + assert(core.df_render_thread != std::thread::id{} && core.df_render_thread != std::this_thread::get_id()); + } public: void lock() { @@ -440,7 +445,7 @@ namespace DFHack class CoreSuspender : protected CoreSuspenderBase { using parent_t = CoreSuspenderBase; public: - CoreSuspender() : CoreSuspender{Core::getInstance()} { } + CoreSuspender() : CoreSuspender{Core::getInstance()} {} CoreSuspender(Core& core) : CoreSuspenderBase{core} { @@ -515,23 +520,4 @@ namespace DFHack operator bool() const { return owns_lock(); } }; - /*! - * Temporary release main thread ownership to allow alternative thread - * implement DF logic thread loop - */ - struct DFHACK_EXPORT CoreSuspendReleaseMain { - CoreSuspendReleaseMain(); - ~CoreSuspendReleaseMain(); - }; - - /*! - * Temporary claim main thread ownership. This allows caller to call - * Core::Update from a different thread than original DF logic thread if - * logic thread has released main thread ownership with - * CoreSuspendReleaseMain - */ - struct DFHACK_EXPORT CoreSuspendClaimMain { - CoreSuspendClaimMain(); - ~CoreSuspendClaimMain(); - }; -} +} \ No newline at end of file