C# Is this method thread safe?

Question

Consider the following code:

Dictionary<string, string> list = new Dictionary<string, string>();
object lockObj = new object();

public void MyMethod(string a) {

    if (list.Contains(a))
        return;

    lock (lockObj) {
        list.Add(a,"someothervalue");
    }
}

Assuming I'm calling MyMethod("mystring") from different threads concurrently.

Would it be possible for more than one thread (we'll just take it as two) enter the if (!list.Contains(a)) statement in the same time (with a few CPU cycles differences), both threads get evaluated as false and one thread enters the critical region while another gets locked outside, so the second thread enters and add "mystring" to the list again after the first thread exits, resulting in the dictionary trying to add a duplicate key?

Answer 1

No, it's not thread-safe. You need the lock around the list.Contains too as it is possible for a thread to be switched out and back in again between the the if test and adding the data. Another thread may have added data in the meantime.

Answer 2

You need to lock the entire operation (check and add) or multiple threads may attempt to add the same value.

I would recommend using the ConcurrentDictionary(TKey, TValue) since it is designed to be thread safe.

private readonly ConcurrentDictionary<string, string> _items
    = new ConcurrentDictionary<string, string>();

public void MyMethod(string item, string value)
{
    _items.AddOrUpdate(item, value, (i, v) => value);
}

Answer 3

You need to lock around the whole statement. It's possible for you to run into issues on the .Contains portion (the way your code is now)

Answer 4

You should check the list after locking. e.g.

if (list.Contains(a))
return;

    lock (lockObj) {
       if (list.Contains(a))
         return;
       list.Add(a);
    }
}

Answer 5

private Dictionary<string, string> list = new Dictionary<string, string>();

public void MyMethod(string a) {
   lock (list) {
      if (list.Contains(a))
        return;
      list.Add(a,"someothervalue");
    }
}

Check out this guide to locking, it's good

A few guidelines to bear in mind

1. Generally lock around a private static object when locking on multiple writeable values
2. Do not lock on things with scope outside the class or local method such as lock(this), which could lead to deadlocks!
3. You may lock on the object being changed if it is the only concurrently accessed object
4. Ensure the object you lock is not null!
5. You can only lock on reference types

Answer 6

I am going to assume that you meant write ContainsKey instead of Contains. Contains on a Dictionary is explicitly implemented so it is not accessible via the type you declared.¹

Your code is not safe. The reason is because there is nothing preventing ContainsKey and Add from executing at the same time. There are actually some quite remarkable failure scenarios that this would introduce. Because I looked at how the Dictionary is implemented I can see that your code could cause a situation where data structure contains duplicates. And I mean it literally contains duplicates. The exception will not necessarily be thrown. The other failure scenarios just keep getting stranger and stranger, but I will not go into those here.

One trivial modification to your code might involve a variation of the double-checked locking pattern.

public void MyMethod(string a) 
{
  if (!dictionary.ContainsKey(a))
  {
    lock (dictionary)
    {
      if (!dictionary.ContainsKey(a))
      {
        dictionary.Add(a, "someothervalue");
      }
    }
  }
}

This, of course, is not any safer for the reason I already stated. Actually, the double-checked locking pattern is notoriously difficult to get right in all but the simplest cases (like the canonical implementation of a singleton). There are many variations on this theme. You can try it with TryGetValue or the default indexer, but ultimately all of these variations are just dead wrong.

So how could this be done correctly without taking a lock? You could try ConcurrentDictionary. It has the method GetOrAdd which is really useful in these scenarios. Your code would look like this.

public void MyMethod(string a) 
{
  // The variable 'dictionary' is a ConcurrentDictionary.
  dictionary.GetOrAdd(a, "someothervalue");
}

That is all there is to it. The GetOrAdd function will check to see if the item exists. If it does not then it will be added. Otherwise, it will leave the data structure alone. This is all done in a thread-safe manner. In most cases the ConcurrentDictionary does this without waiting on a lock.²

---

¹By the way, your variable name is obnoxious too. If it were not for Servy's comment I may have missed the fact that we were talking about a Dictionary as opposed to a List. In fact, based on the Contains call I first thought we were talking about a List.

²On the ConcurrentDictionary readers are completely lock free. However, writers always take a lock (adds and updates that is; the remove operation is still lock free). This includes the GetOrAdd function. The difference is that the data structure maintains several possible lock options so in most cases there is little or no lock contention. That is why this data structure is said to be "low lock" or "concurrent" as opposed to "lock free".

Answer 7

You can first do a non-locking check, but if you want to be thread-safe you need to repeat the check again within the lock. This way you don't lock unless you have to and ensure thread safety.

Dictionary<string, string> list = new Dictionary<string, string>();
object lockObj = new object();

public void MyMethod(string a) {

    if (list.Contains(a))
        return;

    lock (lockObj) {
       if (!list.Contains(a)){
        list.Add(a,"someothervalue");
       }
    }
}