Object-cache generic locking

Question

I have a memory cached object-cache, said cache can hold multiple types, and i want to add a lock on said {T} whenever given {T} is accessed.

My implementation:

    readonly static IDictionary<Type, List<object>> _cache = new ConcurrentDictionary<Type, List<object>>();

    private static List<object> FindTypeInCache(Type type)
    {
        List<object> list;
        if (_cache.TryGetValue(type, out list))
        {
            return list;
        }
        else
        {
            _cache[type] = new List<object>();
        }

        return new List<object>();
    }

    public static T FindFirstBy<T>(Func<T, bool> predicate) where T : class
    {
        // Is this a valid lock locking only _cache[T] ? And not _cache as whole?
        lock (_cache[typeof(T)])
        {
            return FindTypeInCache(typeof(T)).Cast<T>().Where(predicate).FirstOrDefault();
        }
    }

    public static bool AddOrUpdate<T>(Func<T, bool> predicate, T entity) where T : class
    {
        lock (_cache[typeof(T)])
        {
            // Find Type cache.
            List<object> list = FindTypeInCache(typeof(T));

            // Look for old entity.
            var e = list.Cast<T>().Where(predicate).FirstOrDefault();

            // If no old record exists no problem we treat this as if its a new record.
            if (e != null)
            {
                // Old record found removing it.
                list.Remove(e);
            }

            // Regardless if object existed or not we add it to our Cache.
            list.Add(entity);
            _cache[typeof(T)] = list;
        }
    }

Is my implementation correct only locking down _cache[T] and not entire _cache as a whole when accessed?

Answer 1

There's a lot of things weird (or outright wrong) with your code.

First, you're using a ConcurrentDictionary, but you're not using it as a concurrent dictionary. For example, to initialize the list, you'd use the GetOrAddMethod:

private static List<object> FindTypeInCache(Type type)
{
    return _cache.GetOrAdd(type, () => new List<object>());
}

Simple and thread-safe :)

Second, you're locking on _cache[type] - but even when there's no such type in the cache. This means a KeyNotFoundException.

Third, the only code you're protecting with the lock is the reading. But that quite likely isn't enough - at the very least, you need to also protect the writing with the same lock (especially tricky given the point above), and depending on your actual usage of the return value, the return value's mutations (and if it is indeed mutable, any reads of that mutable value as well).

In other words, you've only managed to protect the code that doesn't actually need protecting (if you use the correct method to update the dictionary)! The extra lock around the Where etc. might help slightly, but it certainly doesn't make the List access safe.

All that said, maybe there's a better solution anyway. You're using the cache using generic methods. Why not make the cache itself generic? This way, you'll avoid using a dictionary in the first place, because each of the generic types you're storing in the dictionary will get their own type - it also means you can initialize the List<T> in a static constructor safely. Any locking can than also be safely applied to all access to a specific generic cache rather than the "aggregate" cache you have now.