lock used and there is no looping still, Exceptions System.InvalidOperationException: Collection was modified; enumeration operation may not execute

Question

I have gone through similar looking questions, where answers are suggesting to use lock and not to modify List inside of foreach loop on the same collection, I have already taken care of these two.

I have globally available List<Object> and there are two different threads which are reading and writing in the list respectively.

To avoid race condition, I have used Lock at both (reading[using linq] and writing). and also I'm creating local copy, modify it and restore that copy to main copy (to reduce time of lock)

I guess while creating a local copy, it is having reference of main copy and thus main copy is being updated out side of lock too. Correct me here!

Including similar code for reference,

    private void UpdateList(List<Object> newList)
    {
       List<Object> localCopyOfList;
       lock(m_lockObj) //again a global object for locking
       {
          //I suspect it is reference type's problem here
          localCopyOfList = m_globalCopyOfList; 
       }
       foreach(Object obj in newList)
       {
          localCopyOfList.Add(obj);
       }
       lock(m_lockObj)
       {
          m_globalCopyOfList = localCopyOfList; 
       }
   }

and for reading, I have method like,

private int GetTotalCount()
{
   List<Object> localCopyOfList;
   lock(m_lockObj) //again a global object for locking
   {
      localCopyOfList = m_globalCopyOfList; 
   }
   //On following line I'm getting this exception
   //Exceptions System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   return localCopyOfList.where(x => x.status == true).Count;
}

Your code example is clearly made up and not real code; there is no declaration of `lockCopyOfList` shown, and you probably meant `localCopyOfList`. That said, your guess about reference types is not far off course; you're not making a copy of the list, you're only making a copy of the reference. It's the same list. If you really want a copy, try `m_globalCopyOfList.ToList()`. Note that, likewise, the `lock` statements protect only the _variable_ `m_globalCopyOfList`, and not the contents of the list itself. — Peter Duniho, Jun 14 '17 at 03:16
The code, `localCopyOfList = m_globalCopyOfList` does *not* create a local copy of the list. It creates a local pointer to the *same* list. This might work better: `localCopyOfList = m_globalCopyOfList.ToList()` (but note that the objects inside the list are also references and still point to the same as those in the original list) — Rufus L, Jun 14 '17 at 03:40

score 4 · Accepted Answer · answered Jun 14 '17 at 03:18

I suspect it is reference type's problem here.

Yes. You are not creating a new list, you just copy the reference to the same list.

In order to create a new list based on existing values, you can use List copy constructor:

List<Object> localCopyOfList;
lock(m_lockObj) //again a global object for locking
{
    //I suspect it is reference type's problem here
    localCopyOfList = new List<Object>(m_globalCopyOfList); 
}
foreach(Object obj in newList)
{
    lockCopyOfList.Add(obj);
}
lock(m_lockObj)
{
    m_globalCopyOfList = localCopyOfList; 
}

Keep in mind that localCopyOfList does not reference to the same collection as m_globalCopyOfList anymore, but its elements do.

score 0 · Answer 2 · answered Jun 14 '17 at 03:20

0

You are merely copying the reference to the same list in your lock. They are therefore the same instance. You need to create a new list for local and copy the contents from the global list into it, inside your lock.

answered Jun 14 '17 at 03:20

cineam mispelt

393
1
8

TJK · Answer 3 · 2017-06-14T04:43:47.797

I don't think any locking is required.

private void UpdateList(List<Object> newList)
{
  //only read operation on global list, adding  items only to local list  
  newList.AddRange(m_globalCopyOfList);  
  //reference assignment is atomic
  m_globalCopyOfList = newList;
}

private int GetTotalCount()
{
   //reference assignment is atomic
   var localCopyOfList = m_globalCopyOfList; 
   //if UpdateList method is called anytime during below operation, 
   //m_globalCopyOfList ref will point to new list, localCopyOfList will not 
   //be touched.  
return localCopyOfList.where(x => x.status == true).Count;
}

@Amit I made demo program with 250 tasks calling updatelist and getlist each. I did not get any exception or data loss. Not sure if I am missing anything but check code below

class Program
{
    static int Main(string[] args)
    {

        var obj = new Demo();
        var t = Enumerable.Range(1, 500)
        .Select((x, index) => Task.Run<int>(() => index %2 == 0 
                                            ? obj.UpdateList(new List<object>() { new { status = true } }) 
                                            : obj.GetTotalCount())).ToArray();
        Task.WaitAll(t);
        Console.Write(obj.GetTotalCount());
        Console.ReadLine();
        return 1;
    }

    public class Demo
    {
        private List<Object> m_globalCopyOfList { get; set; }
        public Demo() {
            m_globalCopyOfList = new List<object>();
        }
        public int UpdateList(List<Object> newList)
        {
            //only read operation on global list, adding  items only to local list  
            newList.AddRange(m_globalCopyOfList);
            //reference assignment is atomic
            m_globalCopyOfList = newList;
            return 1;
        }

        public int GetTotalCount()
        {
            //reference assignment is atomic
            var localCopyOfList = m_globalCopyOfList;
            //if UpdateList method is called anytime during below operation, 
            //m_globalCopyOfList ref will point to new list, localCopyOfList will not 
            //be touched.
            Console.WriteLine("Current count " + localCopyOfList.Count); 
            return localCopyOfList.Count;
        }
    }
}

They both functionality (reading and writing) are in different threads which are quite a frequent thus to be in safe side `Lock` is needed, However I have tried your suggested answer at first place before asking here. It was causing the same exception. — Amit, Jun 14 '17 at 03:58
Amit I made demo program with 250 tasks calling UpdateList and GetTotalCount each. I did not get any exception. Not sure if I am missing anything but check code above — TJK, Jun 14 '17 at 04:48

lock used and there is no looping still, Exceptions System.InvalidOperationException: Collection was modified; enumeration operation may not execute

3 Answers3