From ee0d67216837b8ee83b2a92a34be6f62da8ef1a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 19:30:44 +0000 Subject: [PATCH] Unsubscribe the ProcessExit handler on dispose [patch] EnsureDisposeOnExit subscribed a lambda closing over the instance to AppDomain.CurrentDomain.ProcessExit and kept no reference to it, so there was nothing to unsubscribe. AppDomain.CurrentDomain lives for the process, so every instance that ever queued a save stayed rooted for the rest of the run, and Dispose did not release it. Invisible for a single Get() singleton, but a process that churns instances through LoadOrCreate - one per save slot, profile or document - accumulated them without bound, which is what implementing IDisposable on this type is for. Store the delegate and remove it in Dispose(bool). IsDisposeRegistered stays true afterwards, as the existing multiple-dispose test requires, and that also stops a later EnsureDisposeOnExit re-rooting a disposed instance. Fixes #311 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Pup1E96GmvMiRTNMdABat --- AppDataStorage.Test/AppDataTests.cs | 30 +++++++++++++++++++++++++++++ AppDataStorage/AppData.cs | 19 +++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/AppDataStorage.Test/AppDataTests.cs b/AppDataStorage.Test/AppDataTests.cs index b147f03..567be0d 100644 --- a/AppDataStorage.Test/AppDataTests.cs +++ b/AppDataStorage.Test/AppDataTests.cs @@ -9,6 +9,7 @@ namespace ktsu.AppDataStorage.Test; using System.IO.Abstractions; using System.IO.Abstractions.TestingHelpers; using System.Net.Sockets; +using System.Runtime.CompilerServices; using System.Text.Json; using ktsu.CaseConverter; @@ -474,6 +475,35 @@ public void TestDisposeCanBeCalledMultipleTimes() Assert.IsTrue(appData.IsDisposeRegistered, "IsDisposeRegistered should remain true after multiple disposals."); } + // Kept out of the test body so the instance cannot survive in a local slot the JIT has not yet + // reported dead, which would make the weak reference stay alive for reasons unrelated to the + // event handler. + [MethodImpl(MethodImplOptions.NoInlining)] + private static WeakReference RegisterQueueSaveAndDispose() + { + TestAppData appData = new(); + appData.QueueSave(); + Assert.IsTrue(appData.IsDisposeRegistered, "QueueSave should have registered the process-exit handler."); + appData.Dispose(); + return new WeakReference(appData); + } + + [TestMethod] + public void TestDisposeReleasesTheProcessExitHandlerSoTheInstanceCanBeCollected() + { + // The handler closes over the instance and AppDomain.CurrentDomain lives for the whole + // process, so failing to unsubscribe roots every instance that ever queued a save. That is + // invisible for a single Get() singleton but unbounded for a process that churns instances + // through LoadOrCreate. + WeakReference reference = RegisterQueueSaveAndDispose(); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + Assert.IsFalse(reference.IsAlive, "A disposed instance should not still be rooted by the ProcessExit handler."); + } + [TestMethod] public void TestGetReturnsSameInstance() { diff --git a/AppDataStorage/AppData.cs b/AppDataStorage/AppData.cs index 9eeaa5f..85ed0dc 100644 --- a/AppDataStorage/AppData.cs +++ b/AppDataStorage/AppData.cs @@ -356,6 +356,11 @@ public static void ResetFileSystem() { private bool disposedValue; + // Held so the handler can be removed again. AppDomain.CurrentDomain lives for the process, so a + // handler closing over this instance roots it until the process ends; without a reference to the + // exact delegate there is no way to unsubscribe, and disposing the instance would not release it. + private EventHandler? processExitHandler; + /// /// Gets the file name for the app data file. /// @@ -454,7 +459,8 @@ internal void EnsureDisposeOnExit() if (!IsDisposeRegistered) { IsDisposeRegistered = true; - AppDomain.CurrentDomain.ProcessExit += (sender, e) => Dispose(); + processExitHandler = (sender, e) => Dispose(); + AppDomain.CurrentDomain.ProcessExit += processExitHandler; } } } @@ -474,6 +480,17 @@ protected virtual void Dispose(bool disposing) Save(); } + // Release the process-exit root. Unconditional rather than under `disposing`, because + // it touches only AppDomain.CurrentDomain, which outlives every instance and is safe + // to reach from a finalizer. IsDisposeRegistered deliberately stays true: it records + // that this instance has been registered, and leaving it set stops a later + // EnsureDisposeOnExit re-rooting an already-disposed instance. + if (processExitHandler is not null) + { + AppDomain.CurrentDomain.ProcessExit -= processExitHandler; + processExitHandler = null; + } + disposedValue = true; } }