bug: keep repeating timers running after a throw, and gone after an unload

apps/swiftly/src/FiveStack.Utilities/TimerUtility.cs

@@ -1,4 +1,5 @@
 using System.Threading;
+using Microsoft.Extensions.Logging;
 using SwiftlyS2.Shared;
 
 namespace FiveStack.Utilities
@@ -24,7 +25,29 @@ public static CancellationTokenSource AddTimer(float interval, Action callback)
 
         public static CancellationTokenSource Repeat(float interval, Action callback)
         {
-            CancellationTokenSource timer = _core.Scheduler.RepeatBySeconds(interval, callback);
+            CancellationTokenSource? timer = null;
+
+            // SwiftlyS2 never reschedules a repeating timer whose callback throws,
+            // and still runs a timer's first pass if it was cancelled before then
+            timer = _core.Scheduler.RepeatBySeconds(
+                interval,
+                () =>
+                {
+                    if (timer?.IsCancellationRequested == true)
+                    {
+                        return;
+                    }
+
+                    try
+                    {
+                        callback();
+                    }
+                    catch (Exception ex)
+                    {
+                        _core.Logger.LogError(ex, "repeating timer callback failed");
+                    }
+                }
+            );
             Timers.Add(timer);
             return timer;
         }

apps/swiftly/src/FiveStack.cs

@@ -224,14 +224,27 @@ public override void Unload()
         }
 
         UninstallConnectClientHook();
-        ClanTagUtility.Unhook();
 
         TimerUtility.Kill(_pingTimer);
 
-        _matchService.GetCurrentMatch()?.Reset();
+        // SwiftlyS2 only tears a plugin down once Unload returns, and a hot
+        // reload loads the new build either way: an exception here would leave
+        // this instance's timers and handlers running beside the new one.
+        try
+        {
+            ClanTagUtility.Unhook();
 
-        _ = _matchEvents.Disconnect();
+            _matchService.GetCurrentMatch()?.Reset();
 
-        TimerUtility.ClearAll();
+            _ = _matchEvents.Disconnect();
+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, "Failed to reset match state during unload");
+        }
+        finally
+        {
+            TimerUtility.ClearAll();
+        }
     }
 }

apps/utility-sw/src/UtilityPracticePlugin.cs

@@ -267,8 +267,8 @@ public override void Load(bool hotReload)
         // One repeating job for the whole plugin, not one per player. These
         // deliberately do not get StopOnMapChange: the plugin is not reloaded
         // on a map change, so a timer that stopped there would never come back.
-        _secondTimer = Core.Scheduler.RepeatBySeconds(1, OnSecond);
-        _refillTimer = Core.Scheduler.RepeatBySeconds(0.1f, OnFastTick);
+        _secondTimer = Core.Scheduler.RepeatBySeconds(1, Surviving(OnSecond));
+        _refillTimer = Core.Scheduler.RepeatBySeconds(0.1f, Surviving(OnFastTick));
 
         // Only on a hot reload. A cold boot has no engine globals yet -- asking
         // for the map here is what stopped the plugin loading at all -- and the
@@ -300,6 +300,30 @@ public override void Load(bool hotReload)
         );
     }
 
+    private bool _tornDown;
+
+    // SwiftlyS2 never reschedules a repeating timer whose callback throws, and
+    // still runs a timer's first pass if it was cancelled before then.
+    private Action Surviving(Action callback)
+    {
+        return () =>
+        {
+            if (_tornDown)
+            {
+                return;
+            }
+
+            try
+            {
+                callback();
+            }
+            catch (Exception error)
+            {
+                _logger.LogError(error, "repeating timer callback failed");
+            }
+        };
+    }
+
     // SwiftlyS2 hot reloads a plugin from a thread pool thread -- its file
     // watcher schedules the reload through Task.Run -- and the game thread is
     // never one of those. Every native call throws off the game thread, so the
@@ -309,7 +333,17 @@ public override void Unload()
     {
         if (!Thread.CurrentThread.IsThreadPoolThread)
         {
-            TearDown();
+            // An exception out of Unload stops SwiftlyS2 tearing this instance
+            // down, which leaves its timers and handlers running.
+            try
+            {
+                TearDown();
+            }
+            catch (Exception exception)
+            {
+                _logger.LogWarning(exception, "teardown failed");
+            }
+
             return;
         }
 
@@ -338,6 +372,8 @@ public override void Unload()
 
     private void TearDown()
     {
+        _tornDown = true;
+
         // Drawn entities are not the plugin's to leave behind: without this a
         // hot reload orphans every beam, label and model in the world, with no
         // instance left holding a reference to any of them.