Run OnClose once even when Dispose arrives while it is running [patch]

ScopedAction.Tests/ScopedActionTests.cs

@@ -7,6 +7,8 @@
 [TestClass]
 public class ScopedActionTests
 {
+	public TestContext TestContext { get; set; } = null!;
+
 	/// <summary>
 	/// Test implementation of ScopedAction for testing purposes.
 	/// </summary>
@@ -60,7 +62,7 @@
 	}
 
 	[TestMethod]
 	public void Dispose_WithNullOnClose_DoesNotThrow()
 	{
 		// Arrange
 		TestScopedAction scopedAction = new(onOpen: null, onClose: null);
@@ -87,6 +89,75 @@
 		Assert.AreEqual(1, onCloseCallCount, "OnClose should only be called once");
 	}
 
+	[TestMethod]
+	public void Dispose_CalledFromInsideOnClose_OnlyExecutesOnCloseOnce()
+	{
+		// Arrange
+		int onCloseCount = 0;
+		TestScopedAction? scopedAction = null;
+		scopedAction = new TestScopedAction(
+			onOpen: null,
+			onClose: () =>
+			{
+				// Capped so that a regression fails the assertion instead of overflowing the stack
+				if (Interlocked.Increment(ref onCloseCount) < 5)
+				{
+					scopedAction!.Dispose();
+				}
+			});
+
+		// Act
+		scopedAction.Dispose();
+
+		// Assert
+		Assert.AreEqual(1, onCloseCount, "OnClose should run once even when it disposes its own scope");
+	}
+
+	[TestMethod]
+	public void Dispose_CalledConcurrently_OnlyExecutesOnCloseOnce()
+	{
+		const int threadCount = 4;
+
+		for (int trial = 0; trial < 5; trial++)
+		{
+			// Arrange
+			int onCloseCount = 0;
+			int disposeCallsReturned = 0;
+			using TestScopedAction scopedAction = new(
+				onOpen: null,
+				onClose: () =>
+				{
+					Interlocked.Increment(ref onCloseCount);
+
+					// Hold OnClose open until every other thread's Dispose has returned, so each of them
+					// calls Dispose while OnClose is still running. A regression that lets them run OnClose
+					// too blocks them here, so the wait times out and the count assertion fails.
+					SpinWait.SpinUntil(() => Volatile.Read(ref disposeCallsReturned) >= threadCount - 1, TimeSpan.FromSeconds(5));
+				});
+			using Barrier barrier = new(threadCount);
+
+			// Act
+			Thread[] threads = [.. Enumerable.Range(0, threadCount).Select(_ => new Thread(() =>
+			{
+				barrier.SignalAndWait(TestContext.CancellationToken);
+				scopedAction.Dispose();
+				Interlocked.Increment(ref disposeCallsReturned);
+			}))];
+			foreach (Thread thread in threads)
+			{
+				thread.Start();
+			}
+
+			foreach (Thread thread in threads)
+			{
+				thread.Join();
+			}
+
+			// Assert
+			Assert.AreEqual(1, onCloseCount, $"OnClose should run once when {threadCount} threads dispose together (trial {trial})");
+		}
+	}
+
 	[TestMethod]
 	public void ExceptionInOnOpen_DoesNotPreventConstruction()
 	{

ScopedAction/ScopedAction.cs

@@ -7,7 +7,7 @@ namespace ktsu.ScopedAction;
 /// </summary>
 public abstract class ScopedAction : IDisposable
 {
-	private bool disposedValue;
+	private int disposed;
 
 	/// <summary>
 	/// The action to execute when the scoped action is disposed.
@@ -36,23 +36,16 @@ protected ScopedAction() { }
 	/// <param name="disposing"></param>
 	protected virtual void Dispose(bool disposing)
 	{
-		if (!disposedValue)
+		// Claim disposal before running OnClose, so a Dispose that arrives while OnClose is still running,
+		// from inside OnClose or from another thread, finds the instance already disposed
+		if (Interlocked.Exchange(ref disposed, 1) != 0)
 		{
-			if (disposing)
-			{
-				try
-				{
-					OnClose?.Invoke();
-				}
-				finally
-				{
-					disposedValue = true;
-				}
-			}
-			else
-			{
-				disposedValue = true;
-			}
+			return;
+		}
+
+		if (disposing)
+		{
+			OnClose?.Invoke();
 		}
 	}