Introduction

Following on from my previous post about a “Thread Safe” Dictionary, and the subsequent comment from Rajeesh, made me think that perhaps a general post on “thread safety” was in order.

Stop With the Quotes Already!

The astute amongst you may notice I always try and put the phrase “thread safe” in quotes; and there is a good reason for that! “Thread safe” is a pretty horrible term (although I can’t think of a better one) that doesn’t really give enough detail for what exactly we mean when we express that a class is “thread safe”. As far as I’m concerned, my “library” classes are thread safe if:

“All public static and instance methods and properties on the class are safe to be called concurrently from multiple threads.”

What this certainly does *not* mean is:

“You do not have to consider thread safety or concurrency issues in your own code that uses this class.”

So in the instance of our dictionary if you get/set a value and then in subsequent lines of code you *assume that the value you got or set hasn’t changed* then you have a concurrency issue in *your* code and you will *need to address it, likely using the same synchronisation primitives we used internally in our SafeDictionary class.*

Some Examples

As an example we can take similar code to the snippet that Rajeesh posted:

// Check the item exists and process
// if it does
if (mySafeDictionary["Testing"] != null)
{
    // Process element
    ProcessElement(mySafeDictionary["Testing"]);
}

Although each of those calls to the SafeDictionary are themselves threadsafe, we are making an assumption in *our code* that those value won’t change between the “if” statement and our call to ProcessElement. This particular scenario is one reason why we should always try to use TryGetValue, rather than the “if it exists do this with it” approach above. To this end we should probably alter our SafeDictionary to explicitly deny gets:

public class SafeDictionary<TKey, TValue>
{
    private readonly object _Padlock = new object();
    private readonly Dictionary<TKey, TValue> _Dictionary = new Dictionary<TKey, TValue>();

    public TValue this[TKey key]
    {
        set
        {
            lock (_Padlock)
            {
                _Dictionary[key] = value;
            }
        }
    }

    public bool TryGetValue(TKey key, out TValue value)
    {
        lock (_Padlock)
        {
            return _Dictionary.TryGetValue(key, out value);
        }
    }
}

If you now try and access a member using the array syntax, rather than through TryGetValue you will get a compile time error like:

The property or indexer ‘Variabler.this[string]’ cannot be used in this context because it lacks the get accessor.

From that simple example you may think that TryGetValue solves all our problems; but unfortunately it still isn’t able to save your from yourself! To reiterate what I said earlier, you need to consider concurrency yourself if you *assume that the value you got or set hasn’t changed*. Assuming we expand our SafeDictionary to include all of the options that the normal Dictionary supports, take the following horribly contrived example:

public List<String> GetKeysToProcess()
{
    object myObject;
    List<String> keysToProcess = new List<String>();

    // Check to see if the type is valid for
    // processing
    foreach (var currentKey in myDictionary.Keys)
    {
        if (myDictionary.TryGetValue(currentKey, out myObject))
        {
            if (myObject.GetType() == typeof(ProcessorClass))
                keysToProcess.Add(currentKey);
        }
    }

    return keysToProcess;
}

Now there’s a chance that a key might have been removed between the start of the foreach and us actually using it; but as we use TryGetValue that doesn’t matter – the value just won’t resolve. The key here (pardon the pun) is that we are building and returning a list that *makes assumptions about the contents of the dictionary*. Even if in our consuming code we stick to TryGetValue to handle items being deleted, there’s nothing in this example preventing another thread changing any of the items to be a different type – effectively rendering our List of “Keys to process” completely invalid.

Conclusion

The examples above are very contrived, and I’d hope nobody would ever write those exact pieces of code; but hopefully it illustrates the point that *just because you are working with a “Thread safe” object doesn’t mean you can ignore thread safety concerns in your own code*.

Update : Based on a comment on this post I’ve added a follow up post that covers the question: What is This “Thread Safe” Thing Anyway?

Introduction

A pet project I’m currently working on requires the use of an internal Dictionary to store “registered” data, which is a pretty common requirement. For this particular project I’d like to at least *attempt* to make it “thread safe” on .NET 3.5, with an eye to moving it to the ConcurrentDictionary in .NET 4 which promises not only thread safety, but more granular locking to increase multi-threaded performance.

A simple enough task, but one that I’ve seen many people make mistakes implementing.

Just Locking Writes?

It’s obvious we need to do some kind of syncronisation primitive around our writes, but first impressions might make you think that read should be ok – especially if we stick to the preferred TryGetValue pattern instead of “if it exists then get the value”:

object myValue;

// This is obviously not thread safe.
// Something else can alter the collection
// between ContainsKey and reading the
// value.
if (dictionary.ContainsKey("Testing"))
{     myValue = dictionary["Testing"];
}

// Using TryGetValue looks safe though?
// Doesn't it?!
if (!dictionary.TryGetValue("Testing", out myValue))     throw new KeyNotFoundException();

Unfortunately if we fire up Reflector, and take a look at how TryGetValue is implemented, it’s pretty obvious it has exactly the same concurrency problem as the first method above:

public bool TryGetValue(TKey key, out TValue value)
{
    int index = this.FindEntry(key);
    if (index >= 0)
    {
        value = this.entries[index].value;
        return true;
    }
    value = default(TValue);
    return false;
}

Ok, So I Will Lock Reads and Writes?

The next obvious approach would be to find everywhere in our code where we access the Dictionary, for either reading or writing, and use a syncronisation primitive, such as lock, to ensure we’re only accessing it from a single thread at any one time:

private readonly object padlock = new object();
private readonly Dictionary<string, object> dictionary = new Dictionary<string, object>();

private void Test()
{
    object myValue;

    // Now we lock before we do anything
    lock (padlock)
    {
        if (dictionary.ContainsKey("Testing"))
        {
            myValue = dictionary["Testing"];
        }
    }

    lock (padlock)
    {
        if (!dictionary.TryGetValue("Testing", out myValue))
            throw new KeyNotFoundException();
    }
}

Simple enough, but you’re relying on locking around every access, which is not only ugly, but also potentially prone to errors if you miss one. Also, once we move our code to .NET 4, and the new ConcurrentDictionary, we will have to go through the code and remove each lock in turn – pretty laborious!

Composition, Composition, Composition!

In this approach we wrap our nasty non-thread safe dictionary in our own class, expose the methods we want to use, and take any locks accordingly. This class only implements “array” access and TryGetValue, but is sufficient to show the approach:

public class SafeDictionary<TKey, TValue>
{
    private readonly object _Padlock = new object();
    private readonly Dictionary<TKey, TValue> _Dictionary = new Dictionary<TKey, TValue>();

    public TValue this[TKey key]
    {
        get
        {
            lock (_Padlock)
            {
                return _Dictionary[key];
            }
        }
        set
        {
            lock (_Padlock)
            {
                _Dictionary[key] = value;
            }
        }
    }

    public bool TryGetValue(TKey key, out TValue value)
    {
        lock (_Padlock)
        {
            return _Dictionary.TryGetValue(key, out value);
        }
    }
}

As we prevent any direct access to the Dictionary, and use our lock internally whenever we need to access it, we can now use our SafeDictionary in code without worrying about concurrency issues – both for reading and for writing!

To .Net 4 ?

As I mentioned earlier .Net 4 will be shipping with several “thread safe” collections, including a dictionary, in the new System.Collections.Concurrent namespace. As we have our own SafeDictionary implementation we have a few options here:

• We could go through our code and replace all references to SafeDictionary with ConcurrentDictionary. We don’t have any locks in our main code so we could do this with a direct replacement.
• We could alter our SafeDictionary to use a ConcurrentDictionary internally, and remove all of our internal locks.
• If we don’t mind exposing extra methods we can remove all of the implementation from SafeDictionary and just derive it from ConcurrentDictionary:

public class SafeDictionary<Tkey, TValue> : ConcurrentDictionary<Tkey, TValue>
{
}

Conclusion

Quite a long blog post for quite a simple issue, but even simple concurrency can be the source of mistakes and headaches. Once .NET 4 arrives with its concurrent collections, parallel extensions and parallel debugging options hopefully at least *some* of this headache will go away.