Lock vs. ToArray for thread safe foreach access of List collection
I've got a List collection and I want to iterate over it in a multi threaded app. I need to protect it every time I iterate it since it could be changed and I don't want "collection was modified" exceptions when I do a foreach.
What is the correct way to do this?
Use lock every time I access or loop. I'm rather terrified of deadlocks. Maybe I'm just paranoid of using lock and shouldn't be. What do I need to know if I go this route to avoid deadlocks? Is lock fairly efficient?
Use List<>.ToArray() to copy to an array each time I do a foreach. This causes a performance hit but is easy to do. I'm worried about memory thrashing as well as the time to copy it. Just seems excessive. Is it thread safe to use ToArray?
Don't use foreach and use for loops instead. Wouldn't I need to do a length check every time I did this to make sure the list didn't shrink? That seems annoying.
Answer
There's little reason to be afraid of deadlocks, they are easy to detect. Your program stops running, dead giveaway. What you really should be terrified of is threading races, the kind of bug you'll get when you don't lock when you should. Very hard to diagnose.
Using lock is fine, just make sure you use the exact same locking object in any code that touches that list. Like the code that adds or removes items from that list. If that code runs on the same thread that iterates the list then you don't need a lock. Generally, the only chance for deadlock here is if you have code that relies on the thread state, like Thread.Join(), while it is also holding that locking object. Which ought to be rare.
Yes, iterating a copy of the list is always thread-safe, as long as you use a lock around the ToArray() method. Note that you still need the lock, no structural improvement. The advantage is that you'll hold the lock for a short amount of time, improving concurrency in your program. The disadvantages are its O(n) storage requirements, only having a safe list but not protecting the elements in the list and the tricky problem of always having a stale view of the list content. Especially the last problem is subtle and hard to analyze. If you cannot reason out the side-effects then you probably shouldn't consider this.
Do make sure to treat the ability of foreach to detect a race as a gift, not a problem. Yes, an explicit for(;;) loop is not going to throw the exception, it is just going to malfunction. Like iterating the same item twice or skipping an item completely. You could avoid having to re-check the number of items by iterating it backwards. As long as other thread(s) are only calling Add() and not Remove() that would behave similarly to ToArray(), you'll get the stale view. Not that this will work in practice, indexing the list is not thread-safe either. List<> will reallocate its internal array if necessary. This just won't work and malfunction in unpredictable ways.
There are two points of view here. You can be terrified and follow common wisdom, you'll get a program that works but might not be optimal. That's wise and keeps the boss happy. Or you can experiment and find out for yourself how skewing the rules gets you in trouble. Which will make you happy, you'll be a much better programmer. But your productivity is going to suffer. I don't know what your schedule looks like.