From 41c3d0e7e6cd8462fe19e3752e0507a3368e5730 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 13 Oct 2023 17:17:52 -0700 Subject: [PATCH 1/5] fix --- BitFaster.Caching.UnitTests/ScopedTests.cs | 42 ++++++++++++++++++++++ BitFaster.Caching/ReferenceCount.cs | 2 +- BitFaster.Caching/Throw.cs | 2 +- 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/BitFaster.Caching.UnitTests/ScopedTests.cs b/BitFaster.Caching.UnitTests/ScopedTests.cs index d2f1618b..a78a3750 100644 --- a/BitFaster.Caching.UnitTests/ScopedTests.cs +++ b/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,46 @@ public void WhenScopedIsCreatedFromCacheItemHasExpectedLifetime() valueFactory.Disposable.IsDisposed.Should().BeTrue(); } + + [Fact] + public async Task WhenSoak1() + { + for (int i = 0; i < 10; i++) + { + var scope = new Scoped(new Disposable(i)); + + var l = scope.CreateLifetime(); + + await Threaded.Run(4, () => { + for (int i = 0; i < 100000; i++) + { + using (var l = scope.CreateLifetime()) + { + l.Value.IsDisposed.Should().BeFalse(); + } + } + }); + } + } + + + [Fact] + public async Task WhenSoak2() + { + var lru = new ConcurrentLruBuilder().AsScopedCache().Build(); + + for (int i = 0; i < 10; i++) + { + await Threaded.Run(4, () => { + for (int i = 0; i < 100000; i++) + { + using (var l = lru.ScopedGetOrAdd(i, k => new Scoped(new Disposable(k)))) + { + l.Value.IsDisposed.Should().BeFalse(); + } + } + }); + } + } } } diff --git a/BitFaster.Caching/ReferenceCount.cs b/BitFaster.Caching/ReferenceCount.cs index d67a7314..27328d8f 100644 --- a/BitFaster.Caching/ReferenceCount.cs +++ b/BitFaster.Caching/ReferenceCount.cs @@ -100,7 +100,7 @@ public override int GetHashCode() /// true if the value of left is the same as the value of right; otherwise, false. public static bool operator ==(ReferenceCount left, ReferenceCount right) { - return EqualityComparer>.Default.Equals(left, right); + return object.ReferenceEquals(left, right); } /// diff --git a/BitFaster.Caching/Throw.cs b/BitFaster.Caching/Throw.cs index d56ca5ec..10fe2767 100644 --- a/BitFaster.Caching/Throw.cs +++ b/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() => new ObjectDisposedException(nameof(T)); + private static ObjectDisposedException CreateObjectDisposedException() => new ObjectDisposedException(typeof(T).Name); [ExcludeFromCodeCoverage] private static string GetArgumentString(ExceptionArgument argument) From 51f27bf711a82231490747c28b29820cc2e5821d Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 13 Oct 2023 18:47:29 -0700 Subject: [PATCH 2/5] cleanup --- .../ReferenceCountTests.cs | 12 +++-- .../ScopedCacheTests.cs | 18 ++++++++ BitFaster.Caching.UnitTests/ScopedTests.cs | 45 ++++++++++--------- BitFaster.Caching/Scoped.cs | 12 ++--- BitFaster.Caching/ScopedCacheDefaults.cs | 9 +--- 5 files changed, 58 insertions(+), 38 deletions(-) diff --git a/BitFaster.Caching.UnitTests/ReferenceCountTests.cs b/BitFaster.Caching.UnitTests/ReferenceCountTests.cs index 84208c02..7824b657 100644 --- a/BitFaster.Caching.UnitTests/ReferenceCountTests.cs +++ b/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(new object()); + var b = a.IncrementCopy().DecrementCopy(); + + a.Should().NotBeSameAs(b); + } + [Fact] public void WhenOtherIsNotEqualEqualsReturnsFalse() { diff --git a/BitFaster.Caching.UnitTests/ScopedCacheTests.cs b/BitFaster.Caching.UnitTests/ScopedCacheTests.cs index 2345b48f..131e8815 100644 --- a/BitFaster.Caching.UnitTests/ScopedCacheTests.cs +++ b/BitFaster.Caching.UnitTests/ScopedCacheTests.cs @@ -54,5 +54,23 @@ public void GetOrAddDisposedScopeThrows() getOrAdd.Should().Throw(); } + + [Fact] + public async Task WhenSoak2() + { + 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(new Disposable(k)))) + { + // somehow, this is returning disposed!?! + l.Value.IsDisposed.Should().BeFalse(); + } + } + }); + } + } } } diff --git a/BitFaster.Caching.UnitTests/ScopedTests.cs b/BitFaster.Caching.UnitTests/ScopedTests.cs index a78a3750..5ab9e174 100644 --- a/BitFaster.Caching.UnitTests/ScopedTests.cs +++ b/BitFaster.Caching.UnitTests/ScopedTests.cs @@ -90,14 +90,12 @@ public void WhenScopedIsCreatedFromCacheItemHasExpectedLifetime() } [Fact] - public async Task WhenSoak1() + public async Task WhenSoakCreateLifetimeScopeIsDisposedCorrectly() { for (int i = 0; i < 10; i++) { var scope = new Scoped(new Disposable(i)); - var l = scope.CreateLifetime(); - await Threaded.Run(4, () => { for (int i = 0; i < 100000; i++) { @@ -107,27 +105,30 @@ await Threaded.Run(4, () => { } } }); - } - } - - [Fact] - public async Task WhenSoak2() - { - var lru = new ConcurrentLruBuilder().AsScopedCache().Build(); - - for (int i = 0; i < 10; i++) - { - await Threaded.Run(4, () => { - for (int i = 0; i < 100000; i++) - { - using (var l = lru.ScopedGetOrAdd(i, k => new Scoped(new Disposable(k)))) - { - l.Value.IsDisposed.Should().BeFalse(); - } - } - }); + scope.IsDisposed.Should().BeFalse(); + scope.Dispose(); + scope.IsDisposed.Should().BeTrue(); } } + + //[Fact] + //public async Task WhenSoak2() + //{ + // var lru = new ConcurrentLruBuilder().AsScopedCache().Build(); + + // for (int i = 0; i < 10; i++) + // { + // await Threaded.Run(4, () => { + // for (int i = 0; i < 100000; i++) + // { + // using (var l = lru.ScopedGetOrAdd(i, k => new Scoped(new Disposable(k)))) + // { + // l.Value.IsDisposed.Should().BeFalse(); + // } + // } + // }); + // } + //} } } diff --git a/BitFaster.Caching/Scoped.cs b/BitFaster.Caching/Scoped.cs index f3b9cee7..57e76cf5 100644 --- a/BitFaster.Caching/Scoped.cs +++ b/BitFaster.Caching/Scoped.cs @@ -16,7 +16,7 @@ namespace BitFaster.Caching public sealed class Scoped : IScoped, IDisposable where T : IDisposable { private ReferenceCount refCount; - private bool isDisposed; + private int disposed = 0; /// /// Initializes a new Scoped value. @@ -30,7 +30,7 @@ public Scoped(T value) /// /// Gets a value indicating whether the scope is disposed. /// - public bool IsDisposed => isDisposed; + public bool IsDisposed => Volatile.Read(ref this.disposed) == 1; /// /// 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 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; @@ -98,10 +98,10 @@ private void DecrementReferenceCount() /// 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(); } } diff --git a/BitFaster.Caching/ScopedCacheDefaults.cs b/BitFaster.Caching/ScopedCacheDefaults.cs index 0e826c62..906bd2a8 100644 --- a/BitFaster.Caching/ScopedCacheDefaults.cs +++ b/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."; } } From 1028136f9825e2e30493bb16705a374e893417bc Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 13 Oct 2023 19:22:22 -0700 Subject: [PATCH 3/5] cleanup --- .../ScopedCacheTests.cs | 3 +-- BitFaster.Caching.UnitTests/ScopedTests.cs | 20 +------------------ BitFaster.Caching/Scoped.cs | 4 ++-- 3 files changed, 4 insertions(+), 23 deletions(-) diff --git a/BitFaster.Caching.UnitTests/ScopedCacheTests.cs b/BitFaster.Caching.UnitTests/ScopedCacheTests.cs index 131e8815..4fe9603b 100644 --- a/BitFaster.Caching.UnitTests/ScopedCacheTests.cs +++ b/BitFaster.Caching.UnitTests/ScopedCacheTests.cs @@ -65,8 +65,7 @@ await Threaded.Run(4, () => { { using (var l = this.cache.ScopedGetOrAdd(j, k => new Scoped(new Disposable(k)))) { - // somehow, this is returning disposed!?! - l.Value.IsDisposed.Should().BeFalse(); + l.Value.IsDisposed.Should().BeFalse($"ref count {l.ReferenceCount}"); } } }); diff --git a/BitFaster.Caching.UnitTests/ScopedTests.cs b/BitFaster.Caching.UnitTests/ScopedTests.cs index 5ab9e174..cc47f404 100644 --- a/BitFaster.Caching.UnitTests/ScopedTests.cs +++ b/BitFaster.Caching.UnitTests/ScopedTests.cs @@ -108,27 +108,9 @@ await Threaded.Run(4, () => { scope.IsDisposed.Should().BeFalse(); scope.Dispose(); + scope.TryCreateLifetime(out _).Should().BeFalse(); scope.IsDisposed.Should().BeTrue(); } } - - //[Fact] - //public async Task WhenSoak2() - //{ - // var lru = new ConcurrentLruBuilder().AsScopedCache().Build(); - - // for (int i = 0; i < 10; i++) - // { - // await Threaded.Run(4, () => { - // for (int i = 0; i < 100000; i++) - // { - // using (var l = lru.ScopedGetOrAdd(i, k => new Scoped(new Disposable(k)))) - // { - // l.Value.IsDisposed.Should().BeFalse(); - // } - // } - // }); - // } - //} } } diff --git a/BitFaster.Caching/Scoped.cs b/BitFaster.Caching/Scoped.cs index 57e76cf5..a27274a1 100644 --- a/BitFaster.Caching/Scoped.cs +++ b/BitFaster.Caching/Scoped.cs @@ -82,9 +82,9 @@ private void DecrementReferenceCount() if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, oldRefCount.DecrementCopy(), oldRefCount)) { - if (this.refCount.Count == 0) + if (oldRefCount.Count == 1) { - this.refCount.Value?.Dispose(); + oldRefCount.Value?.Dispose(); } break; From 9511d5234becf657c2320e71b65fd768b490c2a8 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 13 Oct 2023 19:33:53 -0700 Subject: [PATCH 4/5] rename --- BitFaster.Caching.UnitTests/ScopedCacheTests.cs | 2 +- BitFaster.Caching/Scoped.cs | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/BitFaster.Caching.UnitTests/ScopedCacheTests.cs b/BitFaster.Caching.UnitTests/ScopedCacheTests.cs index 4fe9603b..34fa30a3 100644 --- a/BitFaster.Caching.UnitTests/ScopedCacheTests.cs +++ b/BitFaster.Caching.UnitTests/ScopedCacheTests.cs @@ -56,7 +56,7 @@ public void GetOrAddDisposedScopeThrows() } [Fact] - public async Task WhenSoak2() + public async Task WhenSoakScopedGetOrAddValueIsAlwaysAlive() { for (int i = 0; i < 10; i++) { diff --git a/BitFaster.Caching/Scoped.cs b/BitFaster.Caching/Scoped.cs index a27274a1..6a656080 100644 --- a/BitFaster.Caching/Scoped.cs +++ b/BitFaster.Caching/Scoped.cs @@ -82,6 +82,8 @@ private void DecrementReferenceCount() if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, oldRefCount.DecrementCopy(), oldRefCount)) { + // Note this.refCount may be stale. + // Old count == 1, thus new ref count is 0, dispose the value. if (oldRefCount.Count == 1) { oldRefCount.Value?.Dispose(); From 218443e612897d8b30d53764a81f419e0c00c5cd Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 13 Oct 2023 19:57:50 -0700 Subject: [PATCH 5/5] fix docs --- BitFaster.Caching/ReferenceCount.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/BitFaster.Caching/ReferenceCount.cs b/BitFaster.Caching/ReferenceCount.cs index 27328d8f..7c1dd54f 100644 --- a/BitFaster.Caching/ReferenceCount.cs +++ b/BitFaster.Caching/ReferenceCount.cs @@ -93,10 +93,10 @@ public override int GetHashCode() } /// - /// Determines whether two ReferenceCount instances have the same value. + /// Determines whether two ReferenceCount instances are the exact same value via a reference equality check. /// - /// The left ReferernceCount to compare, or null. - /// The right ReferernceCount to compare, or null. + /// The left ReferenceCount to compare, or null. + /// The right ReferenceCount to compare, or null. /// true if the value of left is the same as the value of right; otherwise, false. public static bool operator ==(ReferenceCount left, ReferenceCount right) { @@ -104,10 +104,10 @@ public override int GetHashCode() } /// - /// Determines whether two ReferenceCount instances have different values. + /// Determines whether two ReferenceCount instances are different via a reference equality check. /// - /// The left ReferernceCount to compare, or null. - /// The right ReferernceCount to compare, or null. + /// The left ReferenceCount to compare, or null. + /// The right ReferenceCount to compare, or null. /// true if the value of left is different from the value of right; otherwise, false. public static bool operator !=(ReferenceCount left, ReferenceCount right) {