Make OnDemandHandler safer against pickle errors. Resolve #3513

CHANGELOG.md

@@ -32,6 +32,8 @@
 - [Fix][issue3858]: Fix parsing issues in dice contrib (Griatch)
 - Fix: `Typeclass.objects.get_by_tag()` will now always convert tag keys/categories to integers, to
   avoid inconsistencies with PostgreSQL databases (Griatch)
+- [Fix][issue3513]: Fixed issue where OnDemandHandler could traceback on an
+  un-pickle-able object and cause an error at server shutdown (Griatch)
 - [Doc][pull3801]: Move Evennia doc build system to latest Sphinx/myST
   (PowershellNinja, also honorary mention to electroglyph)
 - [Doc][pull3800]: Describe support for Telnet SSH in HAProxy documentation (holl0wstar)
@@ -60,6 +62,7 @@
 [pull3854]: https://github.com/evennia/evennia/pull/3853
 [pull3733]: https://github.com/evennia/evennia/pull/3853
 [issue3858]: https://github.com/evennia/evennia/issues/3858
+[issue3813]: https://github.com/evennia/evennia/issues/3513
 
 
 ## Evennia 5.0.1

evennia/scripts/ondemandhandler.py

@@ -62,6 +62,8 @@ state = ON_DEMAND_HANDLER.get_stage("flowering", last_checked=plant.planted_time
 
 """
 
+import pickle
+
 from evennia.server.models import ServerConfig
 from evennia.utils import logger
 from evennia.utils.utils import is_iter
@@ -398,10 +400,25 @@ class OnDemandHandler:
         Save the on-demand timers to ServerConfig storage. Should be called when Evennia shuts down.
 
         """
+        cleaned_tasks = {}
         for key, category in list(self.tasks.keys()):
             # in case an object was used for categories, and were since deleted, drop the task
             if hasattr(category, "id") and category.id is None:
-                self.tasks.pop((key, category))
+                self.tasks.pop((key, category), None)
+                continue
+
+            task = self.tasks.get((key, category))
+            try:
+                pickle.dumps(task)
+            except Exception as err:
+                logger.log_trace(
+                    f"Error saving on-demand task {key}[{category}] (purging task): {err}"
+                )
+                self.tasks.pop((key, category), None)
+                continue
+            cleaned_tasks[(key, category)] = task
+
+        self.tasks = cleaned_tasks
         ServerConfig.objects.conf(ONDEMAND_HANDLER_SAVE_NAME, self.tasks)
 
     def _build_key(self, key, category):

evennia/scripts/tests.py

@@ -722,6 +722,60 @@ class TestOnDemandHandler(EvenniaTest):
         self.handler.clear()
         self.handler.save()
 
+    def test_handler_save_purges_unpicklable_task(self):
+        class _UnpicklableValue:
+            def __getstate__(self):
+                raise TypeError("cannot pickle this")
+
+        self.handler.clear()
+        self.handler.save()
+        self.handler.add("good", category="decay", stages={0: "new"})
+        self.handler.add("bad", category=_UnpicklableValue(), stages={0: "new"})
+
+        self.handler.save()
+
+        self.assertEqual(
+            set(self.handler.tasks.keys()),
+            {
+                ("good", "decay"),
+            },
+        )
+        reloaded_handler = OnDemandHandler()
+        reloaded_handler.load()
+        self.assertEqual(
+            set(reloaded_handler.tasks.keys()),
+            {
+                ("good", "decay"),
+            },
+        )
+
+    def test_handler_save_purges_recursive_task(self):
+        class _RecursiveValue:
+            def __getstate__(self):
+                return self.__getstate__()
+
+        self.handler.clear()
+        self.handler.save()
+        self.handler.add("good", category="decay", stages={0: "new"})
+        self.handler.add(_RecursiveValue(), category="loop", stages={0: "new"})
+
+        self.handler.save()
+
+        self.assertEqual(
+            set(self.handler.tasks.keys()),
+            {
+                ("good", "decay"),
+            },
+        )
+        reloaded_handler = OnDemandHandler()
+        reloaded_handler.load()
+        self.assertEqual(
+            set(reloaded_handler.tasks.keys()),
+            {
+                ("good", "decay"),
+            },
+        )
+
     @mock.patch("evennia.scripts.ondemandhandler.OnDemandTask.runtime")
     def test_call_staging_function_with_kwargs(self, mock_runtime):
         """ """

evennia/server/service.py

@@ -522,7 +522,10 @@ class EvenniaServerService(MultiService):
             # only save monitor state on reload, not on shutdown/reset
             from evennia.scripts.monitorhandler import MONITOR_HANDLER
 
-            MONITOR_HANDLER.save()
+            try:
+                MONITOR_HANDLER.save()
+            except Exception as err:
+                logger.log_trace(f"Error saving MonitorHandler state: {err}")
         else:
             if mode == "reset":
                 # like shutdown but don't unset the is_connected flag and don't disconnect sessions
@@ -552,12 +555,18 @@ class EvenniaServerService(MultiService):
         # tickerhandler state should always be saved.
         from evennia.scripts.tickerhandler import TICKER_HANDLER
 
-        TICKER_HANDLER.save()
+        try:
+            TICKER_HANDLER.save()
+        except Exception as err:
+            logger.log_trace(f"Error saving TickerHandler state: {err}")
 
         # on-demand handler state should always be saved.
         from evennia.scripts.ondemandhandler import ON_DEMAND_HANDLER
 
-        ON_DEMAND_HANDLER.save()
+        try:
+            ON_DEMAND_HANDLER.save()
+        except Exception as err:
+            logger.log_trace(f"Error saving OnDemandHandler state: {err}")
 
         # always called, also for a reload
         self.at_server_stop()