6
\$\begingroup\$

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

\$\endgroup\$
1
  • \$\begingroup\$ Hmmm i think in CacheItem property - ItemLifeTimeExpired can get incorrect result in specific situation ?! \$\endgroup\$ – ParanoidPanda Jun 5 '18 at 11:12
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.