views:

194

answers:

6

I'm implementing a simple cache in C#, and trying to make it accessible from multiple threads. In the basic read case, it's easy:

var cacheA = new Dictionary<int, MyObj>(); // Populated in constructor

public MyObj GetCachedObjA(int key)
{
    return cacheA[key];
}

This is, I believe, completely thread-safe. But I'd also like to make the it self-loading. That is, if the cache isn't populated on access, it populates it before satisfying the access request.

Dictionary<int, MyObj> cacheB = null;

public MyObj GetCachedObjB(int key)
{
    if (cacheB == null)
    {
        PopulateCacheB();
    }
    return cacheB[key];
}

private void PopulateCacheB()
{
    cacheB = new Dictionary<int, MyObj>();
    foreach (MyObj item in databaseAccessor)
    {
        cacheB.Add(item.Key, item);
    }
}

This isn't thread-safe, since a thread could access GetCachedObjB after cacheB is instantiated but before it's fully populated, if another thread is in the process of populating it. So what's the best way to perform locking on cacheB so that the cache is thread-safe?

+2  A: 

Like this: http://devplanet.com/blogs/brianr/archive/2008/09/26/thread-safe-dictionary-in-net.aspx

Your first example isn't thread safe at all. What if someone inserts just when another one fetches something from the map ? Inserting will likely alter the dictionary, meaning reading it could access corrupted state

nos
+1  A: 

You could use a ReaderWriterLock.

marcc
A: 

You could use Enterprise Library Cache, which is thread safe.

Shiraz Bhaiji
+1  A: 

Wrap the populate operation in a Monitor. This way if another thread tries to read the cache while it is being populated then the thread will be forced to wait till the populate operation completes/errors before it can attempt the read.

You may also wish to do this for the reads to stop the cache being altered while you are reading from it, in this case you'll need to read into a temporary variable, release the monitor and then return the variable or you'll run into locking issues.

public MyObj GetCachedObjB(int key)
{
    try {
        Monitor.Enter(cacheB);

        if (cacheB == null)
        {
            PopulateCacheB();
        }
    } finally {
        Monitor.Exit(cacheB);
    }
    return cacheB[key];
}

As a general note it you may wish to add some key validation when doing reads on your dictionary, though this depends on whether you want errors on non-existent keys or some default value.

RobV
Any reason for not using the "lock" statement rather than all that "try...Enter...finally...Exit" stuff? http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx
LukeH
Mainly because it illustrates explicitly the point of using a monitor which is a key concept in concurrency programming. Plus this pattern applies in any .Net language. The usage via the c# lock keyword or the vb synclock keyword is implicit.
RobV
Had another thought, personally I use this form because often in my code I take some actions that may fail and throw an exception so I have a Catch block to do appropriate logging/cleanup and the finally block ensures that I never deadlock some resource because of an exception
RobV
+1  A: 

Assuming your dictionary is thread-safe or read-only after population, you could populate it in a thread-safe way thus:

private void PopulateCacheB()
{    
    Dictionary<int, MyObj>() dictionary = new Dictionary<int, MyObj>();    
    foreach (MyObj item in databaseAccessor)    
    {        
        dictionary.Add(item.Key, item);    
    }
    cacheB = dictionary;
}

At worst the data will be retrieved from "databaseAccessor" more than once if there is a race condition, but this shouldn't hurt.

Joe
+1  A: 

You can use the lock statement for simple thread-safety:

private Dictionary<int, MyObj> cacheB = null;
private readonly object cacheLockB = new object();

public MyObj GetCachedObjB(int key)
{
    lock (cacheLockB)
    {
        if (cacheB == null)
        {
            Dictionary<int, MyObj> temp = new Dictionary<int, MyObj>();
            foreach (MyObj item in databaseAccessor)
            {
                temp.Add(item.Key, item);
            }
            cacheB = temp;
        }
        return cacheB[key];
    }
}

If you need to squeeze out more performance than lock allows then you could use a ReaderWriterLockSlim instead, which would allow multiple threads to read from the dictionary simultaneously. When you need to populate or update the dictionary, you could just upgrade the lock to write mode.

LukeH