Making a "modify-while-enumerating" collection thread-safe

Question

I want to create a thread-safe collection that can be modified while being enumerated.

The sample ActionSet class stores Action handlers. It has the Add method that adds a new handler to the list and the Invoke method that enumerates and invokes all of the collected action handlers. The intended working scenarios include very frequent enumerations with occasional modifications while enumerating.

Normal collections throw exception if you modify them using the Add method while the enumeration is not over.

There is an easy, but slow solution to the problem: Just clone the collection before enumeration:

class ThreadSafeSlowActionSet {
    List<Action> _actions = new List<Action>();

    public void Add(Action action) {
        lock(_actions) {
            _actions.Add(action);
        }
    }

    public void Invoke() {
        lock(_actions) {
            List<Action> actionsClone = _actions.ToList();
        }
        foreach (var action in actionsClone ) {
            action();
        }
    }
}

The problem with this solution is the enumeration overhead and I want enumeration to be very fast.

I've created a rather fast "recursion-safe" collection that allows adding new values even while enumerating. If you add new values while the main _actions collection is being enumerated, the values are added to the temporary _delta collection instead of the main one. After all enumerations are finished, the _delta values are added to the _actions collection. If you add some new values while the main _actions collection is being enumerated (creating the _delta collection) and then re-enter the Invoke method again we have to create a new merged collection (_actions + _delta) and replace _actions with it.

So, this collection looks "recursion-safe", but I want to make it thread-safe. I think that I need to use the Interlocked.* constructs, classes from System.Threading and other synchronization primitives to make this collection thread-safe, but I don't have a good idea on how to do that.

How to make this collection thread-safe?

class RecursionSafeFastActionSet {
    List<Action> _actions = new List<Action>(); //The main store
    List<Action> _delta; //Temporary buffer for storing added values while the main store is being enumerated
    int _lock = 0; //The number of concurrent Invoke enumerations

    public void Add(Action action) {
        if (_lock == 0) { //_actions list is not being enumerated and can be modified
            _actions.Add(action);
        } else { //_actions list is being enumerated and cannot be modified
            if (_delta == null) {
                _delta = new List<Action>();
            }
            _delta.Add(action); //Storing the new values in the _delta buffer
        }
    }

    public void Invoke() {
        if (_delta != null) { //Re-entering Invoke after calling Add:  Invoke->Add,Invoke
            Debug.Assert(_lock > 0);
            var newActions = new List<Action>(_actions); //Creating a new list for merging delta
            newActions.AddRange(_delta); //Merging the delta
            _delta = null;
            _actions = newActions; //Replacing the original list (which is still being iterated)
        }
        _lock++;
        foreach (var action in _actions) {
            action();
        }
        _lock--;
        if (_lock == 0 && _delta != null) {
            _actions.AddRange(_delta); //Merging the delta
            _delta = null;
        }
    }
}

Update: Added the ThreadSafeSlowActionSet variant.

Answer 1

A simpler approach (used, for example, by ConcurrentBag) is to have GetEnumerator() return an enumerator over a snapshot of the collection's contents. In your case this might look like:

public IEnumerator<Action> GetEnumerator()
{
    lock(sync)
    {
        return _actions.ToList().GetEnumerator();
    }
}

If you do this, you don't need a _delta field and the complexity it adds.

Answer 2

Here is your class modified for thread safety:

class SafeActionSet
{
    Object _sync = new Object();
    List<Action> _actions = new List<Action>(); //The main store
    List<Action> _delta = new List<Action>();   //Temporary buffer for storing added values while the main store is being enumerated
    int _lock = 0; //The number of concurrent Invoke enumerations

    public void Add(Action action)
    {
        lock(sync)
        {
            if (0 == _lock)
            { //_actions list is not being enumerated and can be modified
                _actions.Add(action);
            }
            else
            { //_actions list is being enumerated and cannot be modified
                _delta.Add(action); //Storing the new values in the _delta buffer
            }
        }
    }

    public void Invoke()
    {
        lock(sync)
        {
            if (0 < _delta.Count)
            { //Re-entering Invoke after calling Add:  Invoke->Add,Invoke
                Debug.Assert(0 < _lock);
                var newActions = new List<Action>(_actions); //Creating a new list for merging delta
                newActions.AddRange(_delta); //Merging the delta
                _delta.Clear();
                _actions = newActions; //Replacing the original list (which is still being iterated)
            }
            ++_lock;
        }
        foreach (var action in _actions)
        {
            action();
        }
        lock(sync)
        {
            --_lock;
            if ((0 == _lock) && (0 < _delta.Count))
            {
                _actions.AddRange(_delta); //Merging the delta
                _delta.Clear();
            }
        }
    }
}

I made a few other tweaks, for the following reason:

• reversed IF expressions to have constant value first, so if I do a typo and put "=" instead of "==" or "!=" etc., the compiler will instantly tell me of the typo. (: a habit I got into because my brain and fingers are often out of sync :)
• preallocated _delta, and called .Clear() instead of setting it to null, because I find it is easier to read.
• the various lock(_sync) {...} give you your thread safety on all instance variable access. :( with the exception of your access to _action in the enumeration itself. ):

Answer 3

Since I actually also needed to delete items from the collection, the implementation that I ultimately used was based on a rewritten LinkedList that locks adjacent nodes on deletion/insertion and doesn't complain if the collection was changed during enumeration. I also added a Dictionary to make the element search fast.