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());
}
});
}
}
}
-
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.
-
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.
Jon Skeet : The statement in the lock *is* executed in the lock. It's just that that statement returns a deferred-execution iterator. The *lambda expression* isn't executed in the lock, if that's what you meant to say. -
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 classDictionary
rather than the direct assignment.Then, I think you're thread-safe, but obviously your class is read-only.
0 comments:
Post a Comment