How to safely write to the same List

Question

I've got a public static List<MyDoggie> DoggieList;

DoggieList is appended to and written to by multiple processes throughout my application.

We run into this exception pretty frequently:

Collection was modified; enumeration operation may not execute

Assuming there are multiple classes writing to DoggieList how do we get around this exception?

Please note that this design is not great, but at this point we need to quickly fix it in production.

How can we perform mutations to this list safely from multiple threads?

I understand we can do something like:

lock(lockObject)
{
   DoggieList.AddRange(...)
}

But can we do this from multiple classes against the same DoggieList?

Yes. Don't expose the List methods directly. Write a wrapper around it that does the locking. This could even implement `IList` and (in most cases) you wouldn't even need to update the client code. — spender, Mar 15 '18 at 19:22
...however, basing a significant amount of application around a raw list has a rather bad smell to it. — spender, Mar 15 '18 at 19:23
@Eser locking on a publicly accessible object (the architecture described requires it to be so) is going to be a recipe for disaster. If you're considering locking, you need to make sure that **all** locking remains under your control and is tightly moderated. Public lock objects (here, the list itself) are a big no-no, and an invitation for deadlocks down the road. — spender, Mar 15 '18 at 19:30
suggest you make use of ConcurrentBag and that will managet locking thing internal and easy to use ... — Pranay Rana, Mar 15 '18 at 19:35
@PranayRana So a ConcurrentBag is good for multithreaded inserts but can you predict its behaviour as you iterate and modify at the same time? Iteration is over a moment-in-time snapshot... Might be good for OP, might not... — spender, Mar 15 '18 at 19:40
@spender - yes ,,,you can go though the documentation..http://www.albahari.com/threading/part5.aspx#_Concurrent_Collections — Pranay Rana, Mar 15 '18 at 19:42
Just realized... by "multiple processes" you actually mean "multiple threads", right? — vc 74, Mar 15 '18 at 19:42
I would add one note to this question. If you know this list can be modified you never want to use that list in a ForEach loop. This will also lock the list during the loop. You cant even modify the list within the loop itself — Kevbo, Mar 15 '18 at 19:47
@vc74 yes, that would be multiple classes, and multiple threads in each class — Alex Gordon, Mar 15 '18 at 19:53
@spender ConcurrentBag does not even guarantee order of elements when you enumerate it. It can return elements in completely arbitrary order (not how they were inserted). It doesn't have indexer. That's not a replacement of List at all. From docs of concurrent bag: "Bags are useful for storing objects when ordering doesn't matter". Heck, you cannot even remove item from ConcurrentBag. — Evk, Mar 15 '18 at 20:02
Thread-Safe Collections: https://learn.microsoft.com/en-us/dotnet/standard/collections/thread-safe/ — M.Hassan, Mar 15 '18 at 20:10
@spender thanks for explaining the things I already know. It is just a comment to make the things roll. Not an answer and doesn't deserve a 4 lines response :) — Eser, Mar 15 '18 at 20:22

vc 74 · Answer 1 · 2018-03-16T07:15:12.213

1

Using lock a the disadvantage of preventing concurrent readings.

An efficient solution which does not require changing the collection type is to use a ReaderWriterLockSlim

private static readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();

With the following extension methods:

public static class ReaderWriterLockSlimExtensions
{
    public static void ExecuteWrite(this ReaderWriterLockSlim aLock, Action action)
    {
        aLock.EnterWriteLock();
        try
        {
            action();
        }
        finally
        {
            aLock.ExitWriteLock();
        }
    }

    public static void ExecuteRead(this ReaderWriterLockSlim aLock, Action action)
    {
        aLock.EnterReadLock();
        try
        {
            action();
        }
        finally
        {
            aLock.ExitReadLock();
        }
    }
}

which can be used the following way:

_lock.ExecuteWrite(() => DoggieList.Add(new Doggie()));

_lock.ExecuteRead(() =>
{
    // safe iteration
    foreach (MyDoggie item in DoggieList)
    {
        ....
    }
})

And finally if you want to build your own collection based on this:

public class SafeList<T>
{
    private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
    private readonly List<T> _list = new List<T>();

    public T this[int index]
    {
        get
        {
            T result = default(T);

            _lock.ExecuteRead(() => result = _list[index]);

            return result;
        }
    }

    public List<T> GetAll()
    {
        List<T> result = null;

        _lock.ExecuteRead(() => result = _list.ToList());

        return result;
    }

    public void ForEach(Action<T> action) => 
      _lock.ExecuteRead(() => _list.ForEach(action));

    public void Add(T item) => _lock.ExecuteWrite(() => _list.Add(item));

    public void AddRange(IEnumerable<T> items) => 
      _lock.ExecuteWrite(() => _list.AddRange(items));
}

This list is totally safe, multiple threads can add or get items in parallel without any concurrency issue. Additionally, multiple threads can get items in parallel without locking each other, it's only when writing than 1 single thread can work on the collection.

Note that this collection does not implement IEnumerable<T> because you could get an enumerator and forget to dispose it which would leave the list locked in read mode.

edited Mar 16 '18 at 07:15

answered Mar 15 '18 at 19:25

vc 74

37,131
7
73
89

can i make _lock a global object? since i need to access it from multiple classes? – Alex Gordon Mar 15 '18 at 19:30
1

_Using lock a the disadvantage of preventing concurrent readings._ - do you mean that using ```lock``` instead of your suggested implementation, will lead to dirty reads? – Alex Gordon Mar 15 '18 at 19:30
No, but multuiple threads can read the list using this mechanism, while only 1 thread is allowed to modify the list at a time – vc 74 Mar 15 '18 at 19:31
1

@l--''''''---------'''''''''''' Don't use a global, public lock. You'll lose control of it's usage and it will become a deadlocking pit of potential failure. – spender Mar 15 '18 at 19:35
@l--''''''---------'''''''''''' Rather, you should use this mechanism in your own `IList` implementation that wraps all access to the contained list. The lock object will be part of its ***internal*** implementation that cannot be abused from unknown parts of your code. – spender Mar 15 '18 at 19:37
@spender Aggreed, but "but at this point we need to quickly fix it in production." – vc 74 Mar 15 '18 at 19:38
If we are talking about quick fixes, creating custom IList is much faster than going through whole codebase and changing how that list is used everywhere. – Evk Mar 15 '18 at 19:49
@Evk thanks so much guys, can you please show me an example of the implementatoin you suggest, where you extend IList? – Alex Gordon Mar 15 '18 at 19:51
@l--''''''---------'''''''''''' well you create new class and implement `IList` interface. All methods you just proxy to `List _list` field, implementing locks where necessary (like here: https://stackoverflow.com/a/33086040/5311735, note that implementation is not good, just first I found to illustrate). Problem is - that will not solve your problems anyway, just put them under the carpet. To fix your problem - you _need_ to go through codebase and implement proper locking at each place, like this answer suggests. ConcurrentBag will not help either. – Evk Mar 15 '18 at 19:59

Pranay Rana · Answer 2 · 2018-03-16T05:15:56.510

1

you can also create you own class and encapsulate locking thing in that only, you can try like as below ,

you can add method you want like addRange, Remove etc.

class MyList { 

  private object objLock = new object(); 
  private List<int> list = new List<int>();


  public void Add(int value) {
    lock (objLock) {
      list.Add(value);
    }
  }

  public int Get(int index) {
   int val = -1;
   lock (objLock) {
      val = list[0];
    }
    return val;
  }

  public void GetAll() {
   List<int> retList = new List<int>();
   lock (objLock) {
      retList = new List<T>(list);
    }
    return retList;
  }
}

Good stuff : Concurrent Collections very much in detail :http://www.albahari.com/threading/part5.aspx#_Concurrent_Collections

making use of concurrent collection ConcurrentBag Class can also resolve issue related to multiple thread update

Example

using System.Collections.Concurrent;
using System.Threading.Tasks;

public static class Program
{
    public static void Main()
    {
        var items = new[] { "item1", "item2", "item3" };
        var bag = new ConcurrentBag<string>();
        Parallel.ForEach(items, bag.Add);
    }
}

edited Mar 16 '18 at 05:15

answered Mar 15 '18 at 19:28

Pranay Rana

175,020
35
237
263

are they really plug and play? – Alex Gordon Mar 15 '18 at 19:49
@l--''''''---------'''''''''''' -- yes – Pranay Rana Mar 16 '18 at 05:09
unfortunately i cant use concurrentbag because i need to be able to AddRange and do IEnumerable stuff like DistinctBy – Alex Gordon Mar 16 '18 at 16:39
@l--''''''---------'''''''''''' -- than you can write you own and encapsulate thing like i did in update – Pranay Rana Mar 16 '18 at 17:22
what would be the advantage of concurrentbag over blockedcollection? – Alex Gordon Mar 16 '18 at 17:27
@l--''''''---------'''''''''''' - i tried concurrentbag but not the other one ....you need to go through doucumentation thats my suggestion for that – Pranay Rana Mar 16 '18 at 17:29

score 0 · Answer 3 · edited Mar 15 '18 at 19:32

0

make DoggieList of type ConcurrentStack and then use pushRange method. It is thread safe.

using System.Collections.Concurrent;

var doggieList = new ConcurrentStack<MyDoggie>();
doggieList.PushRange(YourCode)

edited Mar 15 '18 at 19:32

Pranay Rana

175,020
35
237
263

answered Mar 15 '18 at 19:28

Fancy5g

99
4

How to safely write to the same List

3 Answers3