0

I have the following code:

lock (fullEventList)
{
    ftl = fullEventList.Where(a => a != null)
                       .OrderBy(a => a.Start)
                       .ThenBy(a => a.TaskID)
                       .ThenBy(a => a.Status).ToList();
}

An exception (collection was modified) is being thrown which highlights the .OrderBy part of the line. Given there is a lock around the List fullEventList, I think there may be another thread somewhere modifying the list. There are some usages of .Sort() which modifies the list rather than returning a new list, but I've not found where that is occurring yet.

Is there a way to protect this line from the list being modified elsewhere?

Gilad Green
  • 36,708
  • 7
  • 61
  • 95
Stephen Price
  • 1,629
  • 1
  • 24
  • 42
  • 2
    No there isn't... You have to `lock` everywhere. – xanatos Apr 05 '17 at 07:55
  • I was under the impression you only had to lock things that modify the list, and reading the list is OK without a lock. – Stephen Price Apr 05 '17 at 08:28
  • then what happens if one thread locks and remove the last element, and at the same time another thread, while ignoring the lock, tries to read the last element? **every** time you access a method, property or indexer you should `lock` before using it... What is true is that if there are **no** writers (for example all the data has been written at the beginning of the program and the collection won't ever be modified), then any number of readers can read without locks – xanatos Apr 05 '17 at 08:32
  • Thanks, makes sense. I guess that leads me to the thought that how come I've never used locks with Lists before now and had no issues? This is an Android/Xamarin app, and usually I've been in .Net / C# and lists and using locks were rare. Never seen this type of exception until now. – Stephen Price Apr 05 '17 at 08:43
  • Because you weren't using multithread, or you where using `for` cycles instead of `foreach` cycle (indirectly the `foreach` checks if the collection has been modified and then throws an exception, the `for` doesn't check). Linq methods internally are big `foreach`. – xanatos Apr 05 '17 at 08:46

1 Answers1

0

As pointed out by @xanatos in comments, modifying the list while iterating the list (from any thread) will throw an exception.

Linq internally iterate through the list.

using a lock does not prevent another thread from modifying the list from another block of code, so that code must also lock on the list.

Another way to deal with lists is to treat them as immutable objects and not modify them. If you want to order the items in the list, use the OrderBy into a new list (thus not modifying the List) and then when done processing it, assign this new list to the old list variable. 

    var closureList = fullEventList.Where(a => a != null).ToList();
    ftl = closureList.Where(a => a != null)
                     .OrderBy(a => a.Start)
                     .ThenBy(a => a.TaskID)
                     .ThenBy(a => a.Status).ToList();

A bit more memory required but less of an issue with threads. No need for the lock as each thread would be operating on it's own copy of the list.

Stephen Price
  • 1,629
  • 1
  • 24
  • 42