Unsubscribe the ProcessExit handler on dispose

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()
 	{

AppDataStorage/AppData.cs

@@ -356,6 +356,11 @@
 {
 	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;
+
 	/// <summary>
 	/// Gets the file name for the app data file.
 	/// </summary>
@@ -408,10 +413,10 @@
 	/// </summary>
 #if NET9_0_OR_GREATER
 	[JsonIgnore]
 	public static Lock Lock { get; } = new();
 #else
 	[JsonIgnore]
 	public static object Lock { get; } = new();
 #endif
 
 	internal bool IsSaveQueued()
@@ -454,7 +459,8 @@
 			if (!IsDisposeRegistered)
 			{
 				IsDisposeRegistered = true;
-				AppDomain.CurrentDomain.ProcessExit += (sender, e) => Dispose();
+				processExitHandler = (sender, e) => Dispose();
+				AppDomain.CurrentDomain.ProcessExit += processExitHandler;
 			}
 		}
 	}
@@ -474,6 +480,17 @@
 					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;
 			}
 		}