Collection was modified; enumeration operation may not execute. Lock is being used everywhere how possible?

Question

This is a small program that only i am writing and using.

Now i am going to write code of all areas where i use the hashset that caused this problem

I don't understand how this is possible. This item is being used only at MainWindow

hsProxyList is a hashset

  HashSet<string> hsProxyList = new HashSet<string>();

the error happened at below iteration

 lock (hsProxyList)
            {
    int irRandomProxyNumber = GenerateRandomValue.GenerateRandomValueMin(hsProxyList.Count, 0);
    int irLocalCounter = 0;
    foreach (var vrProxy in hsProxyList)
    {
       if (irLocalCounter == irRandomProxyNumber)
       {
       srSelectedProxy = vrProxy;
       break;
       }
         irLocalCounter++;
       }
    }
}

The other places where i use hsProxyList

I don't lock the object when i am getting its count - i suppose this would not cause any error but may be not correct - not fatally important

 lblProxyCount.Content = "remaining proxy count: " + hsProxyList.Count;

new

lock (hsProxyList)
{
    hsProxyList.Remove(srSelectedProxy);
}

new

lock (hsProxyList)
{
    hsProxyList = new HashSet<string>();
    foreach (var vrLine in File.ReadLines(cmbBoxSelectProxy.SelectedItem.ToString()))
    {
        hsProxyList.Add(vrLine);
    }
}

As can be seen i am using lock everywhere. This is a multi threading software. All hsProxyList is being used in MainWindow.xaml.cs - it is a C# WPF application

Answer 1

The problem is where you have

lock (hsProxyList)
{
    hsProxyList = new HashSet<string>();
    // etc
}

All locks are on a particular object, however you're changing the object when you do hsProxyList = new HashSet<string>(); so the object that the variable hsProxyList refers to is no longer locked.

Answer 2

There are two issues here. The first, which has already been pointed out is that you're locking on the hash set whilst also changing the object hsProxyList points to:

lock (hsProxyList)
{
    hsProxyList = new HashSet<string>();
    // hsProxyList is no longer locked.
}

The second (and more subtle) problem, is that you're assuming that Count does not require a lock. This is not a safe assumption. Firstly, you don't know how HashSet has implemented it. The fact that Count is an O(1) operation indicates there is a member variable that keeps track of the count. This means that on Add or Remove this variable must be updated. An implementation of Add might look something like:

bool Add( T item ) {
    this.count++;
    // Point A.
    addItemToHashSet(item);
}

Note that the count variable is incremented and then the item is added. If the thread calling Add is interupted at point A and your other thread that calls Count is executed you will receive a count that is higher than the number of actual elements (count has been incremented, but addItemToHashSet has not).

This may not have any serious consequences, but if you're iterating over Count elements it could cause a crash. Similar behaviour is also likely when calling Remove.