trjhtr

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

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.