From 889f203ac13f97ea846b4221c8f63892a6b86690 Mon Sep 17 00:00:00 2001
From: Alex Peck <alexpeck@microsoft.com>
Date: Tue, 9 Nov 2021 17:32:06 -0800
Subject: [PATCH 1/2] fix race

---
 BitFaster.Caching.UnitTests/ScopedTests.cs | 10 +++++
 BitFaster.Caching/Scoped.cs                | 44 +++++++++++++++-------
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/BitFaster.Caching.UnitTests/ScopedTests.cs b/BitFaster.Caching.UnitTests/ScopedTests.cs
index db018177..1cd1706c 100644
--- a/BitFaster.Caching.UnitTests/ScopedTests.cs
+++ b/BitFaster.Caching.UnitTests/ScopedTests.cs
@@ -50,6 +50,16 @@ public void WhenScopeIsDisposedCreateScopeThrows()
             scope.Invoking(s => s.CreateLifetime()).Should().Throw<ObjectDisposedException>();
         }
 
+        [Fact]
+        public void WhenScopeIsDisposedTryCreateScopeReturnsFalse()
+        {
+            var disposable = new Disposable();
+            var scope = new Scoped<Disposable>(disposable);
+            scope.Dispose();
+
+            scope.TryCreateLifetime(out var l).Should().BeFalse();
+        }
+
         [Fact]
         public void WhenScopedIsCreatedFromCacheItemHasExpectedLifetime()
         {
diff --git a/BitFaster.Caching/Scoped.cs b/BitFaster.Caching/Scoped.cs
index c66f1ca6..58a88c41 100644
--- a/BitFaster.Caching/Scoped.cs
+++ b/BitFaster.Caching/Scoped.cs
@@ -26,33 +26,51 @@ public Scoped(T value)
         }
 
         /// <summary>
-        /// Creates a lifetime for the scoped value. The lifetime guarantees the value is alive until 
+        /// Attempts to create a lifetime for the scoped value. The lifetime guarantees the value is alive until 
         /// the lifetime is disposed.
         /// </summary>
-        /// <returns>A value lifetime.</returns>
-        /// <exception cref="ObjectDisposedException">The scope is disposed.</exception>
-        public Lifetime<T> CreateLifetime()
+        /// <param name="lifetime">When this method returns, contains the Lifetime that was created, or the default value of the type if the operation failed.</param>
+        /// <returns>true if the Lifetime was created; otherwise false.</returns>
+        public bool TryCreateLifetime(out Lifetime<T> lifetime)
         {
-            if (this.isDisposed)
-            {
-                throw new ObjectDisposedException($"{nameof(T)} is disposed.");
-            }
-
             while (true)
             {
-                // IncrementCopy will throw ObjectDisposedException if the referenced object has no references.
-                // This mitigates the race where the value is disposed after the above check is run.
                 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)
+                {
+                    lifetime = default;
+                    return false;
+                }
+
                 var newRefCount = oldRefCount.IncrementCopy();
 
                 if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, newRefCount, oldRefCount))
                 {
-                    // When Lease is disposed, it calls DecrementReferenceCount
-                    return new Lifetime<T>(oldRefCount, this.DecrementReferenceCount);
+                    // When Lifetime is disposed, it calls DecrementReferenceCount
+                    lifetime = new Lifetime<T>(oldRefCount, this.DecrementReferenceCount);
+                    return true;
                 }
             }
         }
 
+        /// <summary>
+        /// Creates a lifetime for the scoped value. The lifetime guarantees the value is alive until 
+        /// the lifetime is disposed.
+        /// </summary>
+        /// <returns>A value lifetime.</returns>
+        /// <exception cref="ObjectDisposedException">The scope is disposed.</exception>
+        public Lifetime<T> CreateLifetime()
+        {
+            if (!TryCreateLifetime(out var lifetime))
+            {
+                throw new ObjectDisposedException($"{nameof(T)} is disposed.");
+            }
+
+            return lifetime;
+        }
+
         private void DecrementReferenceCount()
         {
             while (true)

From 1d84b1d3debaecef248a0abd996f7e1f05e0c25a Mon Sep 17 00:00:00 2001
From: Alex Peck <alexpeck@microsoft.com>
Date: Tue, 9 Nov 2021 17:39:12 -0800
Subject: [PATCH 2/2] simplify code

---
 BitFaster.Caching/Scoped.cs | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/BitFaster.Caching/Scoped.cs b/BitFaster.Caching/Scoped.cs
index 58a88c41..6379e442 100644
--- a/BitFaster.Caching/Scoped.cs
+++ b/BitFaster.Caching/Scoped.cs
@@ -44,9 +44,7 @@ public bool TryCreateLifetime(out Lifetime<T> lifetime)
                     return false;
                 }
 
-                var newRefCount = oldRefCount.IncrementCopy();
-
-                if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, newRefCount, oldRefCount))
+                if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, oldRefCount.IncrementCopy(), oldRefCount))
                 {
                     // When Lifetime is disposed, it calls DecrementReferenceCount
                     lifetime = new Lifetime<T>(oldRefCount, this.DecrementReferenceCount);
@@ -76,13 +74,12 @@ private void DecrementReferenceCount()
             while (true)
             {
                 var oldRefCount = this.refCount;
-                var newRefCount = oldRefCount.DecrementCopy();
 
-                if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, newRefCount, oldRefCount))
+                if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, oldRefCount.DecrementCopy(), oldRefCount))
                 {
-                    if (newRefCount.Count == 0)
+                    if (this.refCount.Count == 0)
                     {
-                        newRefCount.Value.Dispose();
+                        this.refCount.Value.Dispose();
                     }
 
                     break;