Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm writing a custom dictionary which is to be used as a helper for caching. The reason I use delegates is because it must interface with generated code, so I can't change this. However, you can assume that the delegates are thread-safe. I did some preliminary tests that it's thread-safe, but I can always use an extra pair of eyes to spot possible concurrent issues

//StoreToCache will handle removal, replacement, and addition to the cache
public delegate object StoreToCache(string key, object value, CacheObject info);
public delegate object GetFromCache(string key);

/// <summary>
/// A super fast concurrent "dictionary" which passes through to the generated Cache class which constructed it. 
/// </summary>
public class CacheDictionary<K,V>
{
    ConcurrentDictionary<K, string> RealKeys=new ConcurrentDictionary<K, string>();
    readonly string BaseKey;
    readonly CacheObject Info;
    StoreToCache StoreTo;
    GetFromCache GetFrom;
    /// <summary>
    /// This returns the amount of keys we are tracking within this CacheDictionary.
    /// Note: This does not necessarily indicate how many items are actually still in the cache! 
    /// </summary>
    /// <returns>
    /// The count.
    /// </returns>
    public int TrackedCount
    {
        get
        {
            return RealKeys.Count;
        }
    }
    public CacheDictionary(string basekey, CacheObject info, StoreToCache store, GetFromCache get)
    {
        BaseKey=basekey;
        Info=info;
        StoreTo=store;
        GetFrom=get;
    }

    void Add (K key, V value)
    {
        string realkey=RealKeys.GetOrAdd(key, (s) => AddKey(key));
        StoreTo(realkey, value, Info);
    }
    public V Remove (K key)
    {
        var res=StoreTo(GetKey(key), null, Info);
        string trash=null;
        RealKeys.TryRemove(key, out trash);
        if(res!=null && res is V)
        {
            return (V)res;
        }
        else
        {
            return default(V);
        }
    }
    static long CurrentKey=0;
    string AddKey(K key)
    {
        long tmp=Interlocked.Increment(ref CurrentKey);
        string k=BaseKey+(tmp).ToString();
        if(!RealKeys.TryAdd(key, k))
        {
            return null;
        }
        return k;
    }
    string GetKey(K key)
    {
        string tmp=null;
        if(!RealKeys.TryGetValue(key, out tmp))
        {
            return null;
        }
        return tmp;
    }
    public V this [K key] {
        get {
            string realkey=GetKey(key);
            if(realkey==null)
            {
                return default(V);
            }
            object tmp=GetFrom(realkey);
            if(tmp!=null && tmp is V)
            {
                return (V)tmp;
            }
            else
            {
                string trash=null;
                RealKeys.TryRemove(key, out trash); //cleanup
                return default(V);
            }
        }
        set {
            if(value==null)
            {
                Remove(key);
            }
            else
            {
                Add (key, value);
            }
        }
    }
    public void Clear ()
    {
        lock(RealKeys)
        {
            foreach(var key in RealKeys.Keys)
            {
                //don't worry about concurrency here. Iterating over the collection is so non-thread-safe it's not even funny. 
                StoreTo(GetKey(key), null, Info);
            }
            RealKeys.Clear();
        }
    }
}

Is this code thread safe? Also assume that the delegates can return basically random values, because they pass through to cache. So, they may or may not have the value requested at anytime. If the delegate doesn't have the value requested, it will return null.

This dictionary is always initialized as static as well, and can be accessed from basically an unlimited number of threads concurrently. IT should never throw an exception of any kind. If a value doesn't exist, it should return null.

Is this thread-safe in all conditions? Also, is there anything that can make the code more clean?

An example use-case is like this:

static ConcurrentDictionary<int, string> d=new ConcurrencyDictionary....
....
d[10]="foo"; //add a value if it doesn't exist
d[10]=null; //remove value if it exists
string tmp=d[1]; //read value. returns the value if it exists, else null
share|improve this question

1 Answer 1

Your code is not safe:

  • simultaneous calls to getter of the indexer and setter for the same key may result in data missing in cache:
    • setter adds the key to RealKeys
    • getter reads it and looks for value in cache while it's not there yet
    • setter add the value to cache
    • getter cleans the cache
  • lock in Clear doesn't prevent other methods from updating RealKeys since other methods don't lock on the RealKeys.

Other issues with code worth noting:

  • naming conventions. Private fields are usually named in camelCase, and often have an underscore prefix
  • single-responsibility. CacheDictionary doesn't need to know about CacheObject. Instead it would be good if "real cache" implements an interface, and it's passed as cache implementation instead of 2 delegates.
  • This cache stores mapping keys->real keys in memory, so real cache most likely is also in-memory. It might be a good idea just to use ConcurrentDictionary<TKey, TValue> or System.Runtime.Caching.MemoryCache instead.
share|improve this answer
    
I though that locking in Clear with the standard C# locks would prevent other non-locked code from executing. I'll have to research that. Also, good point about that very very subtle race condition. Any ideas on how to prevent that? Also, for the naming conventions, I'm fine with that. For single-responsibility, that would make sense and is doable without complicating my code-generator. And finally, for the in-memory cache bit, I can't do this because it's not guaranteed to be an in-memory cache. It could pass through to memcached or some such. –  Earlz Dec 10 '12 at 2:29
    
I'm actually considering scrapping the whole RealKey idea in favor of just requiring that a key type has a meaningful(and unique) .ToString and/or .GetHashCode method –  Earlz Dec 10 '12 at 2:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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