Problem
In my project i should cache some items (1 – 1000) in one time.
Item should be removed from cache after 5 minutes by last access to cached item.
Better use community libs, or standart mechanisms (MemoryCache), but i want implement my self stupid cache. I want to hear different criticisms and tips. Thanks!
Code for review :
public interface ICacheService<T>
{
bool TryGetItem(string key, out T item);
void StoreItem(string key, T item);
}
public sealed class CacheService<T> : IDisposable, ICacheService<T>
{
private static readonly ILogger _logger = LogManager.GetLogger(nameof(ICacheService<T>));
private readonly ReaderWriterLockSlim _cacheSync = new ReaderWriterLockSlim();
private readonly ConcurrentDictionary<string, CacheItem> _cache = new ConcurrentDictionary<string, CacheItem>();
private readonly System.Timers.Timer _expirationTimer = new System.Timers.Timer();
public CacheService()
{
_expirationTimer.Interval = TimeSpan.FromMinutes(1).Milliseconds;
_expirationTimer.Elapsed += ExpireationTimerTick;
_expirationTimer.Start();
}
public void StoreItem(string key, T item)
{
if(string.IsNullOrWhiteSpace(key)) { throw new ArgumentNullException(nameof(key)); }
if(item == null) { throw new ArgumentNullException(nameof(item)); }
var cacheItem = new CacheItem(key, item);
try
{
_cacheSync.EnterWriteLock();
if (_cache.TryAdd(key, cacheItem))
{
_logger.Info($"Item with key : {key} has been append to cache");
}
}
finally
{
_cacheSync.ExitWriteLock();
}
}
public bool TryGetItem(string key, out T item)
{
_cacheSync.EnterReadLock();
try
{
CacheItem cachedItem = null;
_cache.TryGetValue(key, out cachedItem);
item = (T)cachedItem?.Item;
return item != null;
}
finally
{
_cacheSync.ExitReadLock();
}
}
private void ExpireationTimerTick(object sender, EventArgs args)
{
try
{
_expirationTimer.Stop();
_cacheSync.EnterReadLock();
if (_cache.Count == 0)
{
return;
}
var removeList = new List<string>();
foreach (var item in _cache.Values)
{
if (item.ItemLifeTimeExpired)
{
removeList.Add(item.Key);
}
}
if (!removeList.Any())
{
return;
}
_cacheSync.EnterWriteLock();
foreach (var key in removeList)
{
CacheItem temp;
if (_cache.TryRemove(key, out temp))
{
_logger.Info($"Item with key : {key} has been deleted from cache by expiration time over");
}
}
}
finally
{
_expirationTimer.Start();
_cacheSync.ExitReadLock();
if (_cacheSync.IsWriteLockHeld)
{
_cacheSync.ExitWriteLock();
}
}
}
public void Dispose()
{
_expirationTimer.Dispose();
}
}
internal class CacheItem
{
private readonly object _item;
private readonly string _key;
private ulong _creationTimeTicks;
private ulong? _lastAccessTimerTicks;
public object Item
{
get
{
_lastAccessTimerTicks = (ulong)DateTime.UtcNow.Ticks;
return _item;
}
}
public string Key
{
get
{
return _key;
}
}
public bool ItemLifeTimeExpired
{
get
{
var ticks = (ulong)DateTime.UtcNow.Ticks;
if (_lastAccessTimerTicks.HasValue)
{
return ticks >= _lastAccessTimerTicks + (ulong)TimeSpan.FromMinutes(5).Ticks;
}
return ticks >= _creationTimeTicks + (ulong)TimeSpan.FromMinutes(5).Ticks;
}
}
public CacheItem(object item)
{
_key = item.GetHashCode().ToString();
_item = item;
_creationTimeTicks = (ulong)DateTime.UtcNow.Ticks;
}
public CacheItem(string key, object item)
{
_key = key;
_item = item;
_creationTimeTicks = (ulong)DateTime.UtcNow.Ticks;
}
}
UPD append ICachedService<T>
interface
Solution
If you are using ConcurrentDictionary I don’t see why you need to take locks.
Avoid hard coded numbers. Declare intervals once.
private int timerInterval = TimeSpan.FromMinutes(1).Milliseconds;
private ulong cacheDuration = (ulong)TimeSpan.FromMinutes(5).Ticks;
Ticks returns long
. Why cast to ulong
?
StoreItem
should return a bool.
In CacheItem replace
private ulong _creationTimeTicks;
private ulong? _lastAccessTimerTicks;
with
private ulong _expirationTimeTicks;
Then
_expirationTimeTicks = (ulong)DateTime.UtcNow.Ticks + cacheDuration;
It would simplify code and reduce calculations.
public bool ItemLifeTimeExpired
{
get
{
return _expirationTimeTicks >= (ulong)DateTime.UtcNow.Ticks;
}
}
You are taking locks on ConcurrentDictionary where I don’t think you need to. In CacheItem you should take locks on _expirationTimeTicks.
An acync Task might be better for Cache maintenance than a Timer.
If the cache has a small number of items then I would not clear the cache. To me keep the X most recent items makes more sense.