Is this class threadsafe?

Is this class threadsafe?

德意的啸 发布于 2021-11-25 字数 1332 浏览 893 回复 3 原文

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());
        }
      });
    }
  }
}

如果你对这篇文章有疑问,欢迎到本站 社区 发帖提问或使用手Q扫描下方二维码加群参与讨论,获取更多帮助。

扫码加入群聊

发布评论

需要 登录 才能够评论, 你可以免费 注册 一个本站的账号。

评论(3

请帮我爱他 2022-06-07 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.

瞳孔里扚悲伤 2022-06-07 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.

久随 2022-06-07 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.