0

I've been trying to wrap my head around locks but I can't seem to figure it out. The code below uses a lock but still gives a 'collection was modified' error. What am I missing?


    class Program
    {
        static List<int> lEntries = new List<int>();
        static readonly object entriesLock = new object();

        public static List<int> Entries {
            get { lock (entriesLock) { return lEntries; } }
            set { lock (entriesLock) { lEntries = value; } }
        }

        static void Main(string[] args)
        {
            // Run 20 times to reproduce the issue more often
            for (int i = 0; i < 20; i++)
            {
                Task.Run(() =>
                {
                    for (int j = 0; j < 10000; j++)
                    {
                        Entries.Add(j);
                    }
                });

                Task.Run(() =>
                {
                    for (int i = 0; i < 1000; i++)
                    {
                        Entries.Average(); // System.InvalidOperationException: 'Collection was modified; enumeration operation may not execute.'
                    }
                });
            }

            Console.ReadLine();
        }
    }
Jens
  • 15
  • 5
  • I don't know the use case of your project. But I rather recommend using: ConcurrentBag. – M1sterPl0w May 22 '21 at 13:07
  • 1
    That's a good suggestion. This example is a little bit simplified though as I'm actually using a SortedList. Perhaps there is a concurrent collection for that as well. – Jens May 22 '21 at 13:17
  • @M1sterPl0w please don't recommend the `ConcurrentBag`. This is an [extremely specialized](https://stackoverflow.com/questions/15400133/when-to-use-blockingcollection-and-when-concurrentbag-instead-of-listt/64823123#64823123) collection. It's not a thread-safe `List`. The `ConcurrentQueue` is much preferable, without being a thread-safe `List` either. – Theodor Zoulias May 22 '21 at 13:39
  • 1
    Jens I have posted [here](https://stackoverflow.com/questions/2779703/guidelines-of-when-to-use-locking/65250343#65250343) some guidelines about how to use locks, that you might find useful. Be aware that even if you replace the `List` with a concurrent collection like a `ConcurrentQueue`, calling the `Average` LINQ operator on this collection will still be a thread-safety violation. Only the public members of these collections are thread-safe. Members accessed through any of the interfaces these collections implement, including extension methods, are not guaranteed to be thread safe. – Theodor Zoulias May 22 '21 at 13:57
  • @TheodorZoulias no ConcurrentBag is thread save... https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentbag-1?view=net-5.0. And I quote: "Represents a thread-safe, unordered collection of objects.". I don't know the use case and I tried to let OP find his solution, hence there are more concurrent objects. When you search for ConcurrentBag or ConcurrentQueue you find more datatypes like these... – M1sterPl0w May 23 '21 at 08:25
  • @M1sterPl0w yes, the `ConcurrentBag` is thread-safe, but it's not a suitable collection for almost any practical usage. Only if you are implementing a **mixed** producer-consumer scenario, you should consider using this class. Mixed producer-consumer scenarios are extremely rare in practice. – Theodor Zoulias May 23 '21 at 08:52

1 Answers1

4

Locks only last for their scope.

lock (entriesLock)
{
  //safe to access here.
}
// no longer safe

As such, your attempt of returning a locked list is unfortunately meaningless, as the lock expires right away when the getter/setter is left. Use the lock outside when you actually access the list.

for (int j = 0; j < 10000; j++)
{
  lock (entriesLock)
  {
    lEntries.Add(j);
  }
}

// or

lock (entriesLock)
{
  for (int j = 0; j < 10000; j++)
  {
    lEntries.Add(j);
  }
}
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
Jack T. Spades
  • 986
  • 6
  • 9
  • Oh I understand now, quite an oversight on my part. I will mark this as the answer as soon as I'm allowed to. – Jens May 22 '21 at 13:13