Merge pull request #5112 from myk002/myk_coresuspender

Suspend core by default when calling plugin command functions
DFHack · Dec 21, 2024 · 5750159 · 5750159
2 parents 936a84d + e1ec639
commit 5750159
Show file tree

Hide file tree

Showing 98 changed files with 211 additions and 437 deletions.
diff --git a/docs/changelog.txt b/docs/changelog.txt
@@ -71,6 +71,9 @@ Template for new versions:
 
 ## Removed
 
+## Internals
+- Plugin command callbacks are now called with the core suspended by default so DF memory is always safe to access
+
 # 50.15-r1.2
 
 ## Misc Improvements

diff --git a/library/Core.cpp b/library/Core.cpp
@@ -2416,7 +2416,7 @@ int Core::Shutdown ( void )
     d->hotkeythread.join();
     d->iothread.join();
 
-    CoreSuspendClaimer suspend;
+    CoreSuspender suspend;
     if(plug_mgr)
     {
         delete plug_mgr;

diff --git a/library/PluginManager.cpp b/library/PluginManager.cpp
@@ -469,43 +469,31 @@ bool Plugin::reload(color_ostream &out)
 
 command_result Plugin::invoke(color_ostream &out, const std::string & command, std::vector <std::string> & parameters)
 {
-    Core & c = Core::getInstance();
     command_result cr = CR_NOT_IMPLEMENTED;
     access->lock_add();
-    if(state == PS_LOADED)
-    {
-        for (size_t i = 0; i < commands.size();i++)
+    if (state == PS_LOADED) {
+        if (auto cmdIt = std::ranges::find_if(commands, [&](const PluginCommand &cmd) { return cmd.name == command; });
+            commands.end() != cmdIt)
         {
-            PluginCommand &cmd = commands[i];
-            if(cmd.name == command)
-            {
-                // running interactive things from some other source than the console would break it
-                if(!out.is_console() && cmd.interactive)
-                    cr = CR_NEEDS_CONSOLE;
-                else if (cmd.guard)
-                {
-                    // Execute hotkey commands in a way where they can
-                    // expect their guard conditions to be matched,
-                    // so as to avoid duplicating checks.
-                    // This means suspending the core beforehand.
-                    CoreSuspender suspend(&c);
-                    df::viewscreen *top = c.getTopViewscreen();
-
-                    if (!cmd.guard(top))
-                    {
-                        out.printerr("Could not invoke %s: unsuitable UI state.\n", command.c_str());
-                        cr = CR_WRONG_USAGE;
-                    }
-                    else
-                    {
-                        cr = cmd.function(out, parameters);
-                    }
+            // running interactive things from some other source than the console would break it
+            if (!out.is_console() && cmdIt->interactive)
+                cr = CR_NEEDS_CONSOLE;
+            else if (cmdIt->guard) {
+                CoreSuspender suspend;
+                if (!cmdIt->guard(Core::getInstance().getTopViewscreen())) {
+                    out.printerr("Could not invoke %s: unsuitable UI state.\n", command.c_str());
+                    cr = CR_WRONG_USAGE;
                 }
-                else
-                {
-                    cr = cmd.function(out, parameters);
+                else {
+                    cr = cmdIt->function(out, parameters);
                 }
-                break;
+            }
+            else if (cmdIt->unlocked) {
+                cr = cmdIt->function(out, parameters);
+            }
+            else {
+                CoreSuspender suspend;
+                cr = cmdIt->function(out, parameters);
             }
         }
     }

diff --git a/library/include/Core.h b/library/include/Core.h
@@ -481,6 +481,4 @@ namespace DFHack
         CoreSuspendClaimMain();
         ~CoreSuspendClaimMain();
     };
-
-    using CoreSuspendClaimer = CoreSuspender;
 }
diff --git a/library/include/PluginManager.h b/library/include/PluginManager.h
@@ -99,11 +99,12 @@ namespace DFHack
                       const char * _description,
                       command_function function_,
                       bool interactive_ = false,
+                      bool unlocked_ = false,
                       const char * usage_ = ""
                      )
-            : name(_name), description(_description),
-              function(function_), interactive(interactive_),
-              guard(NULL), usage(usage_)
+            : name{_name}, description{_description}, function{function_},
+              interactive{interactive_}, unlocked{unlocked_}, guard{NULL},
+              usage{usage_}
         {
             fix_usage();
         }
@@ -113,9 +114,9 @@ namespace DFHack
                       command_function function_,
                       command_hotkey_guard guard_,
                       const char * usage_ = "")
-            : name(_name), description(_description),
-              function(function_), interactive(false),
-              guard(guard_), usage(usage_)
+            : name{_name}, description{_description}, function{function_},
+              interactive{false}, unlocked{false}, guard{guard_},
+              usage{usage_}
         {
             fix_usage();
         }
@@ -128,11 +129,12 @@ namespace DFHack
 
         bool isHotkeyCommand() const { return guard != NULL; }
 
-        std::string name;
-        std::string description;
-        command_function function;
-        bool interactive;
-        command_hotkey_guard guard;
+        const std::string name;
+        const std::string description;
+        const command_function function;
+        const bool interactive;
+        const bool unlocked;
+        const command_hotkey_guard guard;
         std::string usage;
     };
     class Plugin

diff --git a/library/include/VTableInterpose.h b/library/include/VTableInterpose.h
@@ -46,8 +46,7 @@ namespace DFHack
 
            DEFINE_VMETHOD_INTERPOSE(int, foo, (int arg)) {
                // If needed by the code, claim the suspend lock.
-               // DO NOT USE THE USUAL CoreSuspender, OR IT WILL DEADLOCK!
-               // CoreSuspendClaimer suspend;
+               // CoreSuspender suspend;
                ...
                ... this->field ... // access fields of the df::someclass object
                ...

diff --git a/library/modules/Screen.cpp b/library/modules/Screen.cpp
@@ -860,7 +860,7 @@ df::viewscreen *dfhack_lua_viewscreen::get_pointer(lua_State *L, int idx, bool m
 
 bool dfhack_lua_viewscreen::safe_call_lua(int (*pf)(lua_State *), int args, int rvs)
 {
-    CoreSuspendClaimer suspend;
+    CoreSuspender suspend;
     color_ostream_proxy out(Core::getInstance().getConsole());
 
     auto L = Lua::Core::State;

diff --git a/plugins/3dveins.cpp b/plugins/3dveins.cpp
@@ -1627,8 +1627,6 @@ command_result cmd_3dveins(color_ostream &con, std::vector<std::string> & parame
             return CR_WRONG_USAGE;
     }
 
-    CoreSuspender suspend;
-
     if (!Maps::IsValid())
     {
         con.printerr("Map is not available!\n");

diff --git a/plugins/aquifer.cpp b/plugins/aquifer.cpp
@@ -33,8 +33,6 @@ DFhackCExport command_result plugin_init(color_ostream &out, std::vector <Plugin
 }
 
 static command_result do_command(color_ostream &out, vector<string> &parameters) {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded()) {
         out.printerr("Cannot run %s without a loaded map.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/autobutcher.cpp b/plugins/autobutcher.cpp
@@ -395,8 +395,6 @@ static void autobutcher_target(color_ostream &out, const autobutcher_options &op
 static void autobutcher_modify_watchlist(color_ostream &out, const autobutcher_options &opts);
 
 static command_result df_autobutcher(color_ostream &out, vector<string> &parameters) {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded() || !World::isFortressMode()) {
         out.printerr("Cannot run %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/autochop.cpp b/plugins/autochop.cpp
@@ -199,8 +199,6 @@ DFhackCExport command_result plugin_onupdate(color_ostream &out) {
 }
 
 static command_result do_command(color_ostream &out, vector<string> &parameters) {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded() || !World::isFortressMode()) {
         out.printerr("Cannot run %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/autoclothing.cpp b/plugins/autoclothing.cpp
@@ -395,9 +395,7 @@ static bool validateMaterialCategory(ClothingRequirement *requirement) {
 
 // A command! It sits around and looks pretty. And it's nice and friendly.
 command_result autoclothing(color_ostream &out, vector<string> &parameters)
-{   // Be sure to suspend the core if any DF state is read or modified
-    CoreSuspender suspend;
-
+{
     if (!Core::getInstance().isMapLoaded() || !World::isFortressMode()) {
         out.printerr("Cannot run %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/autodump.cpp b/plugins/autodump.cpp
@@ -190,7 +190,6 @@ static command_result autodump_main(color_ostream &out, vector<string> &paramete
 }
 
 command_result df_autodump(color_ostream &out, vector<string> &parameters) {
-    CoreSuspender suspend;
     return autodump_main(out, parameters);
 }
 
@@ -201,7 +200,6 @@ command_result df_autodump_destroy_here(color_ostream &out, vector<string> &para
     vector<string> args;
     args.push_back("destroy-here");
 
-    CoreSuspender suspend;
     return autodump_main(out, args);
 }
 
@@ -213,8 +211,6 @@ command_result df_autodump_destroy_item(color_ostream &out, vector<string> &para
     if (!parameters.empty())
         return CR_WRONG_USAGE;
 
-    CoreSuspender suspend;
-
     df::item *item = Gui::getSelectedItem(out);
     if (!item)
         return CR_FAILURE;

diff --git a/plugins/autofarm.cpp b/plugins/autofarm.cpp
@@ -516,8 +516,6 @@ static command_result autofarm(color_ostream& out, std::vector<std::string>& par
         return CR_FAILURE;
     }
 
-    CoreSuspender suspend;
-
     if (parameters.size() == 1 && parameters[0] == "runonce")
     {
         if (autofarmInstance) autofarmInstance->process(out);

diff --git a/plugins/autolabor/autolabor.cpp b/plugins/autolabor/autolabor.cpp
@@ -1094,8 +1094,6 @@ DFhackCExport command_result plugin_enable ( color_ostream &out, bool enable )
 
 command_result autolabor (color_ostream &out, std::vector <std::string> & parameters)
 {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded() || !World::isFortressMode()) {
         out.printerr("Cannot run %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/autolabor/labormanager.cpp b/plugins/autolabor/labormanager.cpp
@@ -1977,8 +1977,6 @@ DFhackCExport command_result plugin_enable(color_ostream &out, bool enable)
 
 command_result labormanager(color_ostream &out, std::vector <std::string> & parameters)
 {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isWorldLoaded()) {
         out.printerr("World is not loaded: please load a game first.\n");
         return CR_FAILURE;

diff --git a/plugins/autonestbox.cpp b/plugins/autonestbox.cpp
@@ -137,8 +137,6 @@ static const struct_field_info autonestbox_options_fields[] = {
 struct_identity autonestbox_options::_identity(sizeof(autonestbox_options), &df::allocator_fn<autonestbox_options>, NULL, "autonestbox_options", NULL, autonestbox_options_fields);
 
 static command_result df_autonestbox(color_ostream &out, vector<string> &parameters) {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded() || !World::isFortressMode()) {
         out.printerr("Cannot run %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/blueprint.cpp b/plugins/blueprint.cpp
@@ -1591,8 +1591,6 @@ static bool do_transform(color_ostream &out,
 static command_result do_blueprint(color_ostream &out,
                                    const vector<string> &parameters,
                                    vector<string> &files) {
-    CoreSuspender suspend;
-
     if (parameters.size() >= 1 && parameters[0] == "gui") {
         ostringstream command;
         command << "gui/blueprint";

diff --git a/plugins/building-hacks.cpp b/plugins/building-hacks.cpp
@@ -252,7 +252,7 @@ struct work_hook : df::building_workshopst{
             {
                 if(world->frame_counter % def->skip_updates == 0)
                 {
-                    CoreSuspendClaimer suspend;
+                    CoreSuspender suspend;
                     color_ostream_proxy out(Core::getInstance().getConsole());
                     onUpdateAction(out,this);
                 }

diff --git a/plugins/buildingplan/buildingplan.cpp b/plugins/buildingplan/buildingplan.cpp
@@ -352,8 +352,6 @@ DFhackCExport command_result plugin_onupdate(color_ostream &out) {
 }
 
 static command_result do_command(color_ostream &out, vector<string> &parameters) {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded() || !World::isFortressMode()) {
         out.printerr("Cannot configure %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/burrow.cpp b/plugins/burrow.cpp
@@ -89,8 +89,6 @@ DFhackCExport command_result plugin_onstatechange(color_ostream &out, state_chan
 }
 
 static command_result do_command(color_ostream &out, vector<string> &parameters) {
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded() || !World::isFortressMode()) {
         out.printerr("Cannot run %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

diff --git a/plugins/changeitem.cpp b/plugins/changeitem.cpp
@@ -84,8 +84,6 @@ command_result changeitem_execute(
 
 command_result df_changeitem(color_ostream &out, vector <string> & parameters)
 {
-    CoreSuspender suspend;
-
     bool here = false;
     bool info = false;
     bool force = false;

diff --git a/plugins/changelayer.cpp b/plugins/changelayer.cpp
@@ -55,8 +55,6 @@ static bool warned = false;
 
 command_result changelayer (color_ostream &out, std::vector <std::string> & parameters)
 {
-    CoreSuspender suspend;
-
     string material;
     bool force = false;
     bool all_biomes = false;

diff --git a/plugins/changevein.cpp b/plugins/changevein.cpp
@@ -208,8 +208,6 @@ command_result df_changevein (color_ostream &out, vector <string> & parameters)
     if (parameters.size() != 1)
         return CR_WRONG_USAGE;
 
-    CoreSuspender suspend;
-
     if (!Maps::IsValid())
     {
         out.printerr("Map is not available!\n");

diff --git a/plugins/channel-safely/channel-safely-plugin.cpp b/plugins/channel-safely/channel-safely-plugin.cpp
@@ -215,7 +215,6 @@ namespace CSP {
     }
 
     void UnpauseEvent(bool full_scan = false){
-        CoreSuspender suspend; // we need exclusive access to df memory and this call stack doesn't already have a lock
         DEBUG(plugin).print("UnpauseEvent()\n");
         ChannelManager::Get().build_groups(full_scan);
         ChannelManager::Get().manage_groups();
@@ -361,7 +360,6 @@ namespace CSP {
     }
 
     void OnUpdate(color_ostream &out) {
-        CoreSuspender suspend;
         if (World::ReadPauseState())
             return;
 

diff --git a/plugins/cleanconst.cpp b/plugins/cleanconst.cpp
@@ -21,8 +21,6 @@ REQUIRE_GLOBAL(world);
 
 command_result df_cleanconst(color_ostream &out, vector <string> & parameters)
 {
-    CoreSuspender suspend;
-
     if (!Maps::IsValid())
     {
         out.printerr("Map is not available!\n");

diff --git a/plugins/cleaners.cpp b/plugins/cleaners.cpp
@@ -169,7 +169,6 @@ command_result cleanplants (color_ostream &out)
 
 command_result spotclean (color_ostream &out, vector <string> & parameters)
 {
-    // HOTKEY COMMAND: CORE ALREADY SUSPENDED
     if (cursor->x < 0)
     {
         out.printerr("The cursor is not active.\n");
@@ -236,8 +235,6 @@ command_result clean (color_ostream &out, vector <string> & parameters)
     if(!map && !units && !items && !plants)
         return CR_WRONG_USAGE;
 
-    CoreSuspender suspend;
-
     if(map)
         cleanmap(out,snow,mud,item_spatter);
     if(units)

diff --git a/plugins/cleanowned.cpp b/plugins/cleanowned.cpp
@@ -72,8 +72,6 @@ command_result df_cleanowned (color_ostream &out, vector <string> & parameters)
             return CR_WRONG_USAGE;
     }
 
-    CoreSuspender suspend;
-
     if (!Translation::IsValid())
     {
         out.printerr("Translation data unavailable!\n");

diff --git a/plugins/createitem.cpp b/plugins/createitem.cpp
@@ -219,7 +219,6 @@ command_result df_createitem (color_ostream &out, vector<string> &parameters) {
 
     if (parameters.size() == 1) {
         if (parameters[0] == "inspect") {
-            CoreSuspender suspend;
             auto item = Gui::getSelectedItem(out);
             if (!item)
                 return CR_FAILURE;
@@ -394,7 +393,6 @@ command_result df_createitem (color_ostream &out, vector<string> &parameters) {
         out.printerr("Map is not available.\n");
         return CR_FAILURE;
     }
-    CoreSuspender suspend;
 
     auto unit = Gui::getSelectedUnit(out, true);
     if (!unit) {