Skip to content

Commit c37109c

Browse files
bitfasterbitfaster
authored
Scoped cache broken due to operator== ABA problem (#409)
* fix * cleanup * cleanup * rename * fix docs ---------
1 parent 5ffc05a commit c37109c

File tree

7 files changed

+71
-26
lines changed

7 files changed

+71
-26
lines changed

‎BitFaster.Caching.UnitTests/ReferenceCountTests.cs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
11
using FluentAssertions;
2-
using System;
3-
using System.Collections.Generic;
4-
using System.Text;
52
using Xunit;
63

74
namespace BitFaster.Caching.UnitTests
@@ -17,6 +14,15 @@ public void WhenOtherIsEqualEqualsReturnsTrue()
1714
a.Should().Be(b);
1815
}
1916

17+
[Fact]
18+
public void WhenOtherIsEqualReferenceEqualsReturnsFalse()
19+
{
20+
var a = new ReferenceCount<object>(new object());
21+
var b = a.IncrementCopy().DecrementCopy();
22+
23+
a.Should().NotBeSameAs(b);
24+
}
25+
2026
[Fact]
2127
public void WhenOtherIsNotEqualEqualsReturnsFalse()
2228
{

‎BitFaster.Caching.UnitTests/ScopedCacheTests.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,5 +54,22 @@ public void GetOrAddDisposedScopeThrows()
5454

5555
getOrAdd.Should().Throw<InvalidOperationException>();
5656
}
57+
58+
[Fact]
59+
public async Task WhenSoakScopedGetOrAddValueIsAlwaysAlive()
60+
{
61+
for (int i = 0; i < 10; i++)
62+
{
63+
await Threaded.Run(4, () => {
64+
for (int j = 0; j < 100000; j++)
65+
{
66+
using (var l = this.cache.ScopedGetOrAdd(j, k => new Scoped<Disposable>(new Disposable(k))))
67+
{
68+
l.Value.IsDisposed.Should().BeFalse($"ref count {l.ReferenceCount}");
69+
}
70+
}
71+
});
72+
}
73+
}
5774
}
5875
}

‎BitFaster.Caching.UnitTests/ScopedTests.cs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections.Generic;
55
using System.Text;
66
using Xunit;
7+
using System.Threading.Tasks;
78

89
namespace BitFaster.Caching.UnitTests
910
{
@@ -87,5 +88,29 @@ public void WhenScopedIsCreatedFromCacheItemHasExpectedLifetime()
8788

8889
valueFactory.Disposable.IsDisposed.Should().BeTrue();
8990
}
91+
92+
[Fact]
93+
public async Task WhenSoakCreateLifetimeScopeIsDisposedCorrectly()
94+
{
95+
for (int i = 0; i < 10; i++)
96+
{
97+
var scope = new Scoped<Disposable>(new Disposable(i));
98+
99+
await Threaded.Run(4, () => {
100+
for (int i = 0; i < 100000; i++)
101+
{
102+
using (var l = scope.CreateLifetime())
103+
{
104+
l.Value.IsDisposed.Should().BeFalse();
105+
}
106+
}
107+
});
108+
109+
scope.IsDisposed.Should().BeFalse();
110+
scope.Dispose();
111+
scope.TryCreateLifetime(out _).Should().BeFalse();
112+
scope.IsDisposed.Should().BeTrue();
113+
}
114+
}
90115
}
91116
}

‎BitFaster.Caching/ReferenceCount.cs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -93,21 +93,21 @@ public override int GetHashCode()
9393
}
9494

9595
/// <summary>
96-
/// Determines whether two ReferenceCount instances have the same value.
96+
/// Determines whether two ReferenceCount instances are the exact same value via a reference equality check.
9797
/// </summary>
98-
/// <param name="left">The left ReferernceCount to compare, or null.</param>
99-
/// <param name="right">The right ReferernceCount to compare, or null.</param>
98+
/// <param name="left">The left ReferenceCount to compare, or null.</param>
99+
/// <param name="right">The right ReferenceCount to compare, or null.</param>
100100
/// <returns>true if the value of left is the same as the value of right; otherwise, false.</returns>
101101
public static bool operator ==(ReferenceCount<TValue> left, ReferenceCount<TValue> right)
102102
{
103-
return EqualityComparer<ReferenceCount<TValue>>.Default.Equals(left, right);
103+
return object.ReferenceEquals(left, right);
104104
}
105105

106106
/// <summary>
107-
/// Determines whether two ReferenceCount instances have different values.
107+
/// Determines whether two ReferenceCount instances are different via a reference equality check.
108108
/// </summary>
109-
/// <param name="left">The left ReferernceCount to compare, or null.</param>
110-
/// <param name="right">The right ReferernceCount to compare, or null.</param>
109+
/// <param name="left">The left ReferenceCount to compare, or null.</param>
110+
/// <param name="right">The right ReferenceCount to compare, or null.</param>
111111
/// <returns>true if the value of left is different from the value of right; otherwise, false.</returns>
112112
public static bool operator !=(ReferenceCount<TValue> left, ReferenceCount<TValue> right)
113113
{

‎BitFaster.Caching/Scoped.cs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ namespace BitFaster.Caching
1616
public sealed class Scoped<T> : IScoped<T>, IDisposable where T : IDisposable
1717
{
1818
private ReferenceCount<T> refCount;
19-
private bool isDisposed;
19+
private int disposed = 0;
2020

2121
/// <summary>
2222
/// Initializes a new Scoped value.
@@ -30,7 +30,7 @@ public Scoped(T value)
3030
/// <summary>
3131
/// Gets a value indicating whether the scope is disposed.
3232
/// </summary>
33-
public bool IsDisposed => isDisposed;
33+
public bool IsDisposed => Volatile.Read(ref this.disposed) == 1;
3434

3535
/// <summary>
3636
/// 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)
4545
var oldRefCount = this.refCount;
4646

4747
// If old ref count is 0, the scoped object has been disposed and there was a race.
48-
if (this.isDisposed || oldRefCount.Count == 0)
48+
if (IsDisposed || oldRefCount.Count == 0)
4949
{
5050
lifetime = default;
5151
return false;
@@ -82,9 +82,11 @@ private void DecrementReferenceCount()
8282

8383
if (oldRefCount == Interlocked.CompareExchange(ref this.refCount, oldRefCount.DecrementCopy(), oldRefCount))
8484
{
85-
if (this.refCount.Count == 0)
85+
// Note this.refCount may be stale.
86+
// Old count == 1, thus new ref count is 0, dispose the value.
87+
if (oldRefCount.Count == 1)
8688
{
87-
this.refCount.Value?.Dispose();
89+
oldRefCount.Value?.Dispose();
8890
}
8991

9092
break;
@@ -98,10 +100,10 @@ private void DecrementReferenceCount()
98100
/// </summary>
99101
public void Dispose()
100102
{
101-
if (!this.isDisposed)
103+
// Dispose exactly once, decrement via dispose exactly once
104+
if (Interlocked.CompareExchange(ref this.disposed, 1, 0) == 0)
102105
{
103-
this.DecrementReferenceCount();
104-
this.isDisposed = true;
106+
DecrementReferenceCount();
105107
}
106108
}
107109

‎BitFaster.Caching/ScopedCacheDefaults.cs‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,9 @@
1-
using System;
2-
using System.Collections.Generic;
3-
using System.Linq;
4-
using System.Text;
5-
using System.Threading.Tasks;
6-
1+

72
namespace BitFaster.Caching
83
{
94
internal static class ScopedCacheDefaults
105
{
11-
internal const int MaxRetry = 5;
6+
internal const int MaxRetry = 64;
127
internal static readonly string RetryFailureMessage = $"Exceeded {MaxRetry} attempts to create a lifetime.";
138
}
149
}

‎BitFaster.Caching/Throw.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ internal static class Throw
5555
private static InvalidOperationException CreateScopedRetryFailure() => new InvalidOperationException(ScopedCacheDefaults.RetryFailureMessage);
5656

5757
[MethodImpl(MethodImplOptions.NoInlining)]
58-
private static ObjectDisposedException CreateObjectDisposedException<T>() => new ObjectDisposedException(nameof(T));
58+
private static ObjectDisposedException CreateObjectDisposedException<T>() => new ObjectDisposedException(typeof(T).Name);
5959

6060
[ExcludeFromCodeCoverage]
6161
private static string GetArgumentString(ExceptionArgument argument)

0 commit comments

Comments
 (0)