Skip to content

Commit

Permalink
Fix (minor) 'Random' non-thread-safe access in 'Constants' for TFMs <…
Browse files Browse the repository at this point in the history
…NET6

This was a really minor and likely unobservable bug where on TFMs below NET6 the implementation of Random instance wasn't thread-safe which could have been resulting in FullEvictionInterval and CacheStoreEvictionDelay being calculated as their minimum random value.

Also add more coverage for CacheManager and remove dead code in tests
  • Loading branch information
neon-sunset committed Jul 3, 2022
1 parent 837d578 commit 481245b
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 46 deletions.
20 changes: 15 additions & 5 deletions src/FastCache.Cached/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ internal static class Constants
private static readonly TimeSpan DefaultQuickListInterval = TimeSpan.FromSeconds(15);
private static readonly TimeSpan MaxQuickListInterval = TimeSpan.FromSeconds(60);

#if NET6_0_OR_GREATER
private static readonly Random Random = Random.Shared;
#else
#if !NET6_0_OR_GREATER
private static readonly Random Random = new();
#endif

Expand Down Expand Up @@ -73,7 +71,7 @@ public static TimeSpan FullEvictionInterval
// This is necessary to avoid application stalling induced by all caches getting collected at the same time.
var quickListTicks = (int)QuickListEvictionInterval.TotalMilliseconds;
var delay = quickListTicks * EvictionIntervalMultiplyFactor;
var jitter = Random.Next(-quickListTicks, (quickListTicks * 2) + 1);
var jitter = GetRandomInt(-quickListTicks, (quickListTicks * 2) + 1);
return TimeSpan.FromMilliseconds(delay + jitter);
}
}
Expand All @@ -83,7 +81,7 @@ public static TimeSpan CacheStoreEvictionDelay
get
{
var delay = (int)QuickListEvictionInterval.TotalMilliseconds;
var jitter = Random.Next(0, delay * 2);
var jitter = GetRandomInt(0, delay * 2);
return TimeSpan.FromMilliseconds(delay + jitter);
}
}
Expand All @@ -94,4 +92,16 @@ public static TimeSpan CacheStoreEvictionDelay
public static readonly TimeSpan CooldownDelayAfterFullGC = QuickListEvictionInterval.MultiplyBy(4);

private static string? GetVar(string key) => Environment.GetEnvironmentVariable(key);

private static int GetRandomInt(int minValue, int maxValue)
{
#if NET6_0_OR_GREATER
return Random.Shared.Next(minValue, maxValue);
#else
lock (Random)
{
return Random.Next(minValue, maxValue);
}
#endif
}
}
33 changes: 31 additions & 2 deletions tests/FastCache.CachedTests/Internals/CacheManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ public sealed class CacheManagerTests
{
private record ExpiredEntry(string Value);
private record OptionallyExpiredEntry(string Value, bool IsExpired);
private record RemovableEntry(string Value);

private static readonly Random Random = new();
private static readonly TimeSpan DelayTolerance = TimeSpan.FromMilliseconds(100);

[Theory]
Expand All @@ -28,7 +30,7 @@ public void Trim_Throws_OnInvalidPercentage(double percentage)
}

[Fact]
public async Task ImmediateFullEviction_CorrectlyEvictsEntries_AllExpired()
public async Task QueueFullEviction_CorrectlyEvictsEntries_AllExpired()
{
CacheManager.SuspendEviction<int, ExpiredEntry>();

Expand All @@ -55,7 +57,7 @@ public async Task ImmediateFullEviction_CorrectlyEvictsEntries_AllExpired()
}

[Fact]
public async Task ImmediateFullEviction_CorrectlyEvictsEntries_SomeExpired()
public async Task QueueFullEviction_CorrectlyEvictsEntries_SomeExpired()
{
CacheManager.SuspendEviction<int, OptionallyExpiredEntry>();

Expand Down Expand Up @@ -89,4 +91,31 @@ public async Task ImmediateFullEviction_CorrectlyEvictsEntries_SomeExpired()
Assert.False(inner.Value.IsExpired);
}
}

[Fact]
public async Task QueueFullClear_Clears()
{
CacheManager.SuspendEviction<string, RemovableEntry>();

var quickList = CacheStaticHolder<string, RemovableEntry>.QuickList;
var store = CacheStaticHolder<string, RemovableEntry>.Store;

var length = Constants.QuickListMinLength * 2;
for (var i = 0; i < length; i++)
{
var expiration = TimeSpan.FromMilliseconds(Random.Next(1, int.MaxValue));

new RemovableEntry(GetRandomString()).Cache(i.ToString(), expiration);
}

Assert.Equal((uint)Constants.QuickListMinLength, quickList.AtomicCount);
Assert.Equal(length, store.Count);

CacheManager.QueueFullClear<string, RemovableEntry>();

await Task.Delay(DelayTolerance);

Assert.Equal(0u, quickList.AtomicCount);
Assert.Empty(store);
}
}
43 changes: 4 additions & 39 deletions tests/FastCache.CachedTests/TestHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,58 +16,23 @@ public static string GetTestKey<T>(T arg1, [CallerMemberName] string testName =
return $"{testName}:{arg1}";
}

public static string GetTestKey<T1, T2>(T1 arg1, T2 arg2, [CallerMemberName] string testName = "")
{
return testName + string.Join(':', arg1, arg2);
}

public static string GetTestKey<T1, T2, T3>(T1 arg1, T2 arg2, T3 arg3, [CallerMemberName] string testName = "")
{
return testName + string.Join(':', arg1, arg2, arg3);
}

public static string GetTestKey<T1, T2, T3, T4>(T1 arg1, T2 arg2, T3 arg3, T4 arg4, [CallerMemberName] string testName = "")
{
return testName + string.Join(':', arg1, arg2, arg3, arg4);
}

public static string GetRandomString()
{
#if !NETSTANDARD2_0
var bytes = (stackalloc byte[64]);
#else
var bytes = new byte[64];
#endif

_random.NextBytes(bytes);
lock (_random)
{
_random.NextBytes(bytes);
}

return Convert.ToBase64String(bytes);
}

public static long GetRandomLong()
{
#if !NETSTANDARD2_0
var bytes = (stackalloc byte[8]);
#else
var bytes = new byte[8];
#endif

_random.NextBytes(bytes);

#if !NETSTANDARD2_0
return BitConverter.ToInt64(bytes);
#else
return BitConverter.ToInt64(bytes, 0);
#endif
}

public static void Unreachable()
{
throw new InvalidOperationException("This part of code should never be reached");
}

public static T Unreachable<T>()
{
throw new InvalidOperationException("This part of code should never be reached");
}
}

0 comments on commit 481245b

Please sign in to comment.