Simple time-bound cache

Posted on

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.

Leave a Reply

Your email address will not be published. Required fields are marked *