thread safety checks

library/Core.cpp

@@ -119,26 +119,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 +1528,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 +1689,7 @@ bool Core::InitMainThread() {
 
 bool Core::InitSimulationThread()
 {
+    df_simulation_thread = std::this_thread::get_id();
     if(started)
         return true;
     if(errorstate)
@@ -2108,6 +2091,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 +2395,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 +2508,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<int, bool> hotkey_states;
 
     // do NOT process events before we are ready.

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,23 @@ 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());
+                // holding the access lock while releasing the CoreSuspender creates a deadlock risk
+                access->unlock();
+            }
+            access->lock();
+        }
         // notify plugin about shutdown, if it has a shutdown function
         command_result cr = CR_OK;
         if(plugin_shutdown)

library/include/Core.h

@@ -344,8 +344,12 @@ namespace DFHack
         std::condition_variable_any CoreWakeup;
         std::atomic<std::thread::id> ownerThread;
         std::atomic<size_t> toolCount;
+        std::atomic<bool> 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();
-    };
 }