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; } }