Class for mantain a thread safe cache

Question

I'm developing a thread safe class that I'll use as cache, it should work in .NET and Mono.

The items have a time to live, and every time that a object is retrieved, its time to live is refreshed. Each time that I add a item, the timestamp is added to another collection that holds the same keys. A timer raises the method that looks for out dated items and remove them.

When I try to get and item, I have to provide also a delegate indicating how to obtain it if it wouldn't exist in the cache.

I've testing, and although the items removal should happen every 30 seconds in the test, it's happening very often, almost every second, and I'don't know why.

This is the class:

    public class GenericCache<TId, TItem>:IDisposable where TItem : class
{
    SortedDictionary<TId, TItem> _cache;
    SortedDictionary<TId, DateTime> _timeouts;
    Timer _timer;
    Int32 _cacheTimeout;
    System.Threading.ReaderWriterLockSlim _locker;

    public GenericCache(Int32 minutesTTL)
    {
        _locker = new System.Threading.ReaderWriterLockSlim();
        _cacheTimeout = minutesTTL;
        _cache = new SortedDictionary<TId, TItem>();
        _timeouts = new SortedDictionary<TId, DateTime>();
        _timer = new Timer((minutesTTL * 60) / 2);
        _timer.Elapsed += new ElapsedEventHandler(_timer_Elapsed);
        _timer.AutoReset = true;
        _timer.Enabled = true;
        _timer.Start();
    }

    /// <summary>
    /// Get an item, if it doesn't exist, create it using the delegate
    /// </summary>
    /// <param name="id">Id for the item</param>
    /// <param name="create">A delegate that generates the item</param>
    /// <returns>The item</returns>
    public TItem Get(TId id, Func<TItem> create)
    {
        _locker.EnterUpgradeableReadLock();
        try
        {
            TItem item = _cache.Where(ci => ci.Key.Equals(id)).Select(ci => ci.Value).SingleOrDefault();
            if (item == null)
            {
                _locker.EnterWriteLock();
                // check again, maybe another thread is waiting in EnterWriteLock cos the same item is null
                item = _cache.Where(ci => ci.Key.Equals(id)).Select(ci => ci.Value).SingleOrDefault();
                if (item == null)
                {
                    Debug.Write("_");
                    item = create.Invoke();
                    if (item != null)
                    {
                        _cache.Add(id, item);
                        _timeouts.Add(id, DateTime.Now);
                    }
                }
            }
            else
                _timeouts[id] = DateTime.Now;

            return item;
        }
        finally
        {
            if(_locker.IsWriteLockHeld)
                _locker.ExitWriteLock();
            _locker.ExitUpgradeableReadLock();
        }
    }

    /// <summary>
    /// Execute a delegate in the items, for example clear nested collections.
    /// </summary>
    /// <param name="action">The delegate</param>
    public void ExecuteOnItems(Action<TItem> action)
    {
        _locker.EnterWriteLock();
        try
        {
            foreach (var i in _cache.Values)
                action.Invoke(i);
        }
        finally
        {
            _locker.ExitWriteLock();
        }
    }

    /// <summary>
    /// Clear this cache
    /// </summary>
    public void Clear()
    {
        _locker.EnterWriteLock();
        try
        {
            _cache.Clear();
            _timeouts.Clear();
        }
        finally
        {
            _locker.ExitWriteLock();
        }
    }

    /// <summary>
    /// Remove outdated items
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    private void _timer_Elapsed(object sender, ElapsedEventArgs e)
    {
        _locker.EnterUpgradeableReadLock();
        try
        {
            var delete = _timeouts.Where(to => DateTime.Now.Subtract(to.Value).TotalMinutes > _cacheTimeout).ToArray();

            if(delete.Any())
            {
                _locker.EnterWriteLock();
                foreach (var timeitem in delete)
                {
                    Debug.Write("-");
                    _cache.Remove(timeitem.Key);
                    _timeouts.Remove(timeitem.Key);
                }
            }
        }
        finally
        {
            if(_locker.IsWriteLockHeld)
                _locker.ExitWriteLock();
            _locker.ExitUpgradeableReadLock();
        }
    }

    #region IDisposable Members
    private volatile Boolean disposed = false;
    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            try
            {
                this.Clear();
            }
            finally
            {
                _locker.Dispose();
            }

            disposed = true;
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~GenericCache()
    {
        Dispose(false);
    }

    #endregion
}

As you can see, in debug mode, when a item is added a "_" symbol is printed, and when and item is removed a "-" symbol is printed. In the tests, after the second minute can see how items are removed and added in the same second, when the items should be removed only every 30 seconds, and I don't know why:

This is how I tests:

        static void Main(string[] args)
    {
        GenericCache<Int32, String> cache = new GenericCache<Int32, String>(1);

        Debug.Listeners.Add(new ConsoleTraceListener());

        Action a = delegate()
        {
            Random r = new Random(DateTime.Now.Millisecond);
            while (true)
            {
                Int32 number = r.Next(0, 9999);
                if (String.IsNullOrEmpty(cache.Get(number, () => number.ToString())))
                    Debug.Write("E");
                Thread.Sleep(number);
            }
        };

        for (int i = 0; i < 150; i++)
        {
            new Thread(new ThreadStart(a)).Start();
            Thread.Sleep(5);
        }

        Console.ReadKey();
    }

Do you see any problem in the GenericCache class?

Thanks in advance, kind regards.

Answer 1

First issue i see (assuming you are using System.Timers.Timer accepts milliseconds and you are passing seconds).

  _timer = new Timer((minutesTTL * 60000) / 2);