Is this class threadsafe?

Question

Is this ValueStore class threadsafe? Does the lock scope in GetInt(string key) need to be extended around the yield return?

public class ValueStore
{
  private readonly object _locker = new object();
  private readonly Dictionary<string, int> _data = 
    new Dictionary<string, int>();

  public ValueStore(Dictionary<string, int> data)
  {
    _data = data;
  }

  public IEnumerable<int> GetInt(string key)
  {
    IEnumerable<KeyValuePair<string, int>> selected;
    lock(_locker)
    {
      selected = _data.Where(x => x.Key.Equals(key));
    }

    foreach (KeyValuePair<string, int> pair in selected)
    {
      yield return pair.Value;
    }
  }
}

The unit test seems to be fine:

[TestFixture]
public class ValueStoreTest
{
  [Test]
  public void test1()
  {
    Dictionary<string, int> data = new Dictionary<string, int>();
    for (int i = 0; i < 100000; i++)
    {
      data.Add(i.ToString(),i);
    }

    ValueStore vs = new ValueStore(data);

    for (int i = 0; i < 900000; i++)
    {
      ThreadPool.QueueUserWorkItem(delegate
      {
        for (int j = 0; j < 100000; j++)
        {
          IEnumerable<int> d = vs.GetInt(j.ToString());
        }
      });
    }
  }
}

Answer 1

No, it's definitely not thread-safe.

The fact that it uses a dictionary passed in by the client means you have no control over when the client changes it. You're also only locking it when you apply the Where clause - but that doesn't actually perform any iteration at all. You'd need to hold the lock while iterating over the results - but as I said before, it doesn't stop the client from changing the dictionary at any time anyway.

If you created the dictionary in the class and only ever exposed the data within it (i.e. protected it from the outside world) you could make it fully thread-safe. If you insist that the client code doesn't mutate the dictionary, you don't need the lock at all, as dictionaries are safe to read from multiple threads when there are no writers.

Answer 2

I can tell you that the statement in the lock isn't executed until after the lock is released. If you must lock the collection during iteration then move the yield into the lock statement.

Answer 3

No, it is not. If you begin reading and writing to the Dictionary<string, int> object you passed into the constructor, you have a problem. Your class declaration of _data is instantly overwritten by the assignment in the constructor.

To correct this, copy each key/value pair from the passed in Dictionary in the constructor into your class Dictionary rather than the direct assignment.

Then, I think you're thread-safe, but obviously your class is read-only.