Scoped cache broken due to operator== ABA problem

BitFaster.Caching.UnitTests/ReferenceCountTests.cs

@@ -1,7 +1,4 @@
 using FluentAssertions;
-using System;
-using System.Collections.Generic;
-using System.Text;
 using Xunit;
 
 namespace BitFaster.Caching.UnitTests
@@ -17,6 +14,15 @@ public void WhenOtherIsEqualEqualsReturnsTrue()
             a.Should().Be(b);
         }
 
+        [Fact]
+        public void WhenOtherIsEqualReferenceEqualsReturnsFalse()
+        {
+            var a = new ReferenceCount<object>(new object());
+            var b = a.IncrementCopy().DecrementCopy();
+
+            a.Should().NotBeSameAs(b);
+        }
+
         [Fact]
         public void WhenOtherIsNotEqualEqualsReturnsFalse()
         {

BitFaster.Caching.UnitTests/ScopedCacheTests.cs

@@ -54,5 +54,22 @@ public void GetOrAddDisposedScopeThrows()
 
             getOrAdd.Should().Throw<InvalidOperationException>();
         }
+
+        [Fact]
+        public async Task WhenSoakScopedGetOrAddValueIsAlwaysAlive()
+        {
+            for (int i = 0; i < 10; i++)
+            {
+                await Threaded.Run(4, () => {
+                    for (int j = 0; j < 100000; j++)
+                    {
+                        using (var l = this.cache.ScopedGetOrAdd(j, k => new Scoped<Disposable>(new Disposable(k))))
+                        {
+                            l.Value.IsDisposed.Should().BeFalse($"ref count {l.ReferenceCount}");
+                        }
+                    }
+                });
+            }
+        }
     }
 }

BitFaster.Caching.UnitTests/ScopedTests.cs

@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.Text;
 using Xunit;
+using System.Threading.Tasks;
 
 namespace BitFaster.Caching.UnitTests
 {
@@ -87,5 +88,29 @@ public void WhenScopedIsCreatedFromCacheItemHasExpectedLifetime()
 
             valueFactory.Disposable.IsDisposed.Should().BeTrue();
         }
+
+        [Fact]
+        public async Task WhenSoakCreateLifetimeScopeIsDisposedCorrectly()
+        {
+            for (int i = 0; i < 10; i++)
+            {
+                var scope = new Scoped<Disposable>(new Disposable(i));
+
+                await Threaded.Run(4, () => {
+                    for (int i = 0; i < 100000; i++)
+                    {
+                        using (var l = scope.CreateLifetime()) 
+                        {
+                            l.Value.IsDisposed.Should().BeFalse();
+                        }
+                    }
+                });
+
+                scope.IsDisposed.Should().BeFalse();
+                scope.Dispose();
+                scope.TryCreateLifetime(out _).Should().BeFalse();
+                scope.IsDisposed.Should().BeTrue();
+            }
+        }
     }
 }

BitFaster.Caching/ReferenceCount.cs

@@ -93,21 +93,21 @@ public override int GetHashCode()
         }
 
         /// <summary>
-        /// Determines whether two ReferenceCount instances have the same value.
+        /// Determines whether two ReferenceCount instances are the exact same value via a reference equality check.
         /// </summary>
-        /// <param name="left">The left ReferernceCount to compare, or null.</param>
-        /// <param name="right">The right ReferernceCount to compare, or null.</param>
+        /// <param name="left">The left ReferenceCount to compare, or null.</param>
+        /// <param name="right">The right ReferenceCount to compare, or null.</param>
         /// <returns>true if the value of left is the same as the value of right; otherwise, false.</returns>
         public static bool operator ==(ReferenceCount<TValue> left, ReferenceCount<TValue> right)
         {
-            return EqualityComparer<ReferenceCount<TValue>>.Default.Equals(left, right);
+            return object.ReferenceEquals(left, right);
         }
 
         /// <summary>
-        /// Determines whether two ReferenceCount instances have different values.
+        /// Determines whether two ReferenceCount instances are different via a reference equality check.
         /// </summary>
-        /// <param name="left">The left ReferernceCount to compare, or null.</param>
-        /// <param name="right">The right ReferernceCount to compare, or null.</param>
+        /// <param name="left">The left ReferenceCount to compare, or null.</param>
+        /// <param name="right">The right ReferenceCount to compare, or null.</param>
         /// <returns>true if the value of left is different from the value of right; otherwise, false.</returns>
         public static bool operator !=(ReferenceCount<TValue> left, ReferenceCount<TValue> right)
         {

BitFaster.Caching/Scoped.cs

@@ -16,7 +16,7 @@ namespace BitFaster.Caching
     public sealed class Scoped<T> : IScoped<T>, IDisposable where T : IDisposable
     {
         private ReferenceCount<T> refCount;
-        private bool isDisposed;
+        private int disposed = 0;
 
         /// <summary>
         /// Initializes a new Scoped value.
@@ -30,7 +30,7 @@ public Scoped(T value)
         /// <summary>
         /// Gets a value indicating whether the scope is disposed.
         /// </summary>
-        public bool IsDisposed => isDisposed;
+        public bool IsDisposed => Volatile.Read(ref this.disposed) == 1;
 
         /// <summary>
         /// Attempts to create a lifetime for the scoped value. The lifetime guarantees the value is alive until 
@@ -45,7 +45,7 @@ public bool TryCreateLifetime(out Lifetime<T> lifetime)
                 var oldRefCount = this.refCount;
 
                 // If old ref count is 0, the scoped object has been disposed and there was a race.
-                if (this.isDisposed || oldRefCount.Count == 0)
+                if (IsDisposed || oldRefCount.Count == 0)
                 {
                     lifetime = default;
                     return false;
@@ -82,9 +82,11 @@ private void DecrementReferenceCount()
 
                 if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, oldRefCount.DecrementCopy(), oldRefCount))
                 {
-                    if (this.refCount.Count == 0)
+                    // Note this.refCount may be stale.
+                    // Old count == 1, thus new ref count is 0, dispose the value.
+                    if (oldRefCount.Count == 1)
                     {
-                        this.refCount.Value?.Dispose();
+                        oldRefCount.Value?.Dispose();
                     }
 
                     break;
@@ -98,10 +100,10 @@ private void DecrementReferenceCount()
         /// </summary>
         public void Dispose()
         {
-            if (!this.isDisposed)
+            // Dispose exactly once, decrement via dispose exactly once
+            if (Interlocked.CompareExchange(ref this.disposed, 1, 0) == 0)
             {
-                this.DecrementReferenceCount();
-                this.isDisposed = true;
+                DecrementReferenceCount();
             }
         }
 

BitFaster.Caching/ScopedCacheDefaults.cs

@@ -1,14 +1,9 @@
-using System;
-using System.Collections.Generic;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;
-
+
 namespace BitFaster.Caching
 {
     internal static class ScopedCacheDefaults
     {
-        internal const int MaxRetry = 5;
+        internal const int MaxRetry = 64;
         internal static readonly string RetryFailureMessage = $"Exceeded {MaxRetry} attempts to create a lifetime.";
     }
 }

BitFaster.Caching/Throw.cs

@@ -55,7 +55,7 @@ internal static class Throw
         private static InvalidOperationException CreateScopedRetryFailure() => new InvalidOperationException(ScopedCacheDefaults.RetryFailureMessage);
 
         [MethodImpl(MethodImplOptions.NoInlining)]
-        private static ObjectDisposedException CreateObjectDisposedException<T>() => new ObjectDisposedException(nameof(T));
+        private static ObjectDisposedException CreateObjectDisposedException<T>() => new ObjectDisposedException(typeof(T).Name);
 
         [ExcludeFromCodeCoverage]
         private static string GetArgumentString(ExceptionArgument argument)