Getting Collection was modified; enumeration operation may not execute. exception

Question

Getting Collection was modified; enumeration operation may not execute. exception

Code:

public static string GetValue(List<StateBag> stateBagList, string name)
{
    string retValue = string.Empty;

    if (stateBagList != null)
    {
        foreach (StateBag stateBag in stateBagList)
        {
            if (stateBag.Name.Equals(name, StringComparison.InvariantCultureIgnoreCase))
            {
                retValue = stateBag.Value;
            }
        }
    }

    return retValue;
}

getting this exception some time times not every time at this place.

stacktrace:

at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)

at System.Collections.Generic.List`1.Enumerator.MoveNextRare()

at System.Collections.Generic.List`1.Enumerator.MoveNext()

at Tavisca.TravelNxt.Shared.Entities.StateBag.GetValue(List`1 stateBagList, String name)

---

@no one i have tried for following code but still getting exception

code:

 class StateBag
{
    public string Name;
    public string Value;
}

class Program
{
    static List<StateBag> _concurrent = new List<StateBag>();

    static void Main()
    {
        var sw = new Stopwatch();
        try
        {
            sw.Start();
            Thread thread1 = new Thread(new ThreadStart(A));
            Thread thread2 = new Thread(new ThreadStart(B));
            thread1.Start();
            thread2.Start();
            thread1.Join();
            thread2.Join();
            sw.Stop();
        }
        catch (Exception ex)
        {
        }


        Console.WriteLine("Average: {0}", sw.ElapsedTicks);
        Console.ReadKey();
    }

    private static Object thisLock = new Object();

    public static string GetValue(List<StateBag> stateBagList, string name)
    {
        string retValue = string.Empty;


        if (stateBagList != null)
        {
            lock (thisLock)
            {
                foreach (StateBag stateBag in stateBagList)
                {
                    if (stateBag.Name.Equals(name, StringComparison.InvariantCultureIgnoreCase))
                    {
                        retValue = stateBag.Value;
                    }
                }
            }

        }



        return retValue;
    }

    static void A()
    {
        for (int i = 0; i < 5000; i++)
        {
            _concurrent.Add(new StateBag() { Name = "name" + i, Value = i.ToString() });
        }
    }

    static void B()
    {
        for (int i = 0; i < 5000; i++)
        {
            var t = GetValue(_concurrent, "name" + i);
        }
    }
}

Answer 1

Getting Collection was modified; enumeration operation may not execute. exception

Reason: This exception occurs when the enumeration that you are looping through is modified in same thread or some other thread.

Now, in the code that you have provided there isnn't any such scenario. Which means that you might be calling this in a multi-threaded environment and collection is modified in some other thread.

Solution: Implement locking on your enumeration so that only one thread gets access at a time. Something like this should do it.

private static Object thisLock = new Object();
public static string GetValue(List<StateBag> stateBagList, string name)
{
    string retValue = string.Empty;

    if (stateBagList != null)
    {
        lock(thisLock)
        {
            foreach (StateBag stateBag in stateBagList)
            {
               if (stateBag.Name.Equals(name, StringComparison.InvariantCultureIgnoreCase))
               {
                retValue = stateBag.Value;
                }
             }
        }
    }

    return retValue;
}

Answer 2

Although locking is the right way to go for fixing the original implementation, there might be a better approach altogether which will involve a lot less code and potential bugs.

The following demo console app uses ConcurrentDictionary instead of List, and is fully threadsafe without the need for your own locking logic.

It also offers better performance, as a dictionary lookup is much faster than serially searching a list:

class StateBag
{
    public string Name;
    public string Value;
}

class Program
{
    public static string GetValue(ConcurrentDictionary<string, StateBag> stateBagDict, string name)
    {
        StateBag match;
        return stateBagDict.TryGetValue(name.ToUpperInvariant(), out match) ? 
            match.Value : string.Empty;
    }

    static void Main(string[] args)
    {
        var stateBagDict = new ConcurrentDictionary<string, StateBag>();

        var stateBag1 = new StateBag { Name = "Test1", Value = "Value1" };
        var stateBag2 = new StateBag { Name = "Test2", Value = "Value2" };

        stateBagDict[stateBag1.Name.ToUpperInvariant()] = stateBag1;
        stateBagDict[stateBag2.Name.ToUpperInvariant()] = stateBag2;

        var result = GetValue(stateBagDict, "test1");

        Console.WriteLine(result);
    }
}

Answer 3

This is happening because some other thread in your application is modifying the stateBagList. There are 2 thing you can do... either use locking around your code block where you refer the stateBagList or you can make a deep copy of stateBagList in GetValues method and then use the new list in your for loop.

Answer 4

As already suggested you need to place a lock around the enumeration.

However that action is only effective if you also lock around the statements that are modifying the collection.

static void A()
{
    for (int i = 0; i < 5000; i++)
    {
        lock(thisLock) 
        {
            _concurrent.Add(new StateBag() { Name = "name" + i, Value = i.ToString() });
        }
    }
}

Otherwise all you are doing is ensuring that only one thread can enumerate the collection at a time. A single thread or multiple other threads could still be modifying the collection while this single enumeration takes place.

I'd also recommend the following link: http://www.albahari.com/threading/part2.aspx#_Thread_Safety_and_NET_Framework_Types

Other tips: It is possible to lock on the collection itself like so:

lock(_concurrent) { //statements}

And the GetValue method can be simplified like so:

public static string GetValue(List<StateBag> stateBagList, string name)
{
    if (stateBagList != null)
    {
        lock (thisLock)
        {
            return stateBagList.FirstOrDefault
                 (x => x.Name.Equals(name, StringComparison.InvariantCultureIgnoreCase));
            }
        }
    }    

    return string.Empty;
}

Answer 5

Replace List with SynchronizedCollection. It is thread-safe collection class.

It does this via locking so that you essentially have a List where every access is wrapped in a lock statement.