Collection was modified; enumeration operation may not execute even though the collection was modified exclusively in lock statements

Question

I have the following base code. The ActionMonitor can be used by anyone, in whatever setting, regardless of single-thread or multi-thread.

using System;
public class ActionMonitor
{
    public ActionMonitor()
    {

    }


    private object _lockObj = new object();

    public void OnActionEnded()
    {
        lock (_lockObj)
        {
            IsInAction = false;
            foreach (var trigger in _triggers)
                trigger();
            _triggers.Clear();
        }
    }

    public void OnActionStarted()
    {
        IsInAction = true;
    }

    private ISet<Action> _triggers = new HashSet<Action>();
    public void ExecuteAfterAction(Action action)
    {
        lock (_lockObj)
        {
            if (IsInAction)
                _triggers.Add(action);
            else
                action();
        }
    }

    public bool IsInAction
    {
       get;private set;

    }
}

On exactly one occasion, when I examined a crash on client's machine, an exception was thrown at:

System.Core: System.InvalidOperationException Collection was modified;enumeration operation may not execute. at

System.Collections.Generic.HashSet`1.Enumerator.MoveNext() at

WPFApplication.ActionMonitor.OnActionEnded()

My reaction when seeing this stack trace: this is unbelievable! This must be a .Net bug!.

Because although ActionMonitor can be used in multithreading setting, but the crash above shouldn't occur-- all the _triggers ( the collection) modification happens inside a lock statement. This guarantees that one cannot iterate over the collection and modifying it at the same time.

And, if _triggers happened to contain an Action that involves ActionMonitor, then the we might get a deadlock, but it would never crash.

I have seen this crash exactly once, so I can't reproduce the problem at all. But base on my understanding of multithreading and lock statement, this exception can never have occurred.

Do I miss something here? Or is it known that .Net can behave it a very quirky way, when it involves System.Action?

What calls ExecuteAfterAction and can it be reached by the trigger() call? Because the one thing locks definitely won't help you with is if the thread doing the enumeration is also the thread doing the modification. locks are reentrant. — Damien_The_Unbeliever
The same thread can take the lock multiple times - that's what my previous comment was trying to tell you. — Damien_The_Unbeliever
The simple explanation is that ExecuteAfterAction() ran on the same thread as OnActionEnded(). The lock statement is re-entrant on the same thread. So "regardless of single-thread or multi-thread" is a bug, not a feature. WPF is a hint as well, the lock statement does not actually block the UI thread. Blocking that thread is illegal, the CLR has a dispatcher loop to ensure no deadlock can occur. So, say, an Invoke() call will complete even though the lock is held. — Hans Passant
To visualize, lock (_lockObj) { lock (_lockObj) { stuff(); } } would not deadlock but execute stuff() unhindered. — Cobra_Fast

Rand Random Rand Random · Accepted Answer · 2018-09-21T13:26:23

You didn't shield your code against the following call:

private static ActionMonitor _actionMonitor;

static void Main(string[] args)
{
    _actionMonitor = new ActionMonitor();
    _actionMonitor.OnActionStarted();
    _actionMonitor.ExecuteAfterAction(Foo1);
    _actionMonitor.ExecuteAfterAction(Foo2);
    _actionMonitor.OnActionEnded();

    Console.ReadLine();
}
private static void Foo1()
{
    _actionMonitor.OnActionStarted();
    //Notice that if you would call _actionMonitor.OnActionEnded(); here instead of _actionMonitor.OnActionStarted(); - you would get a StackOverflow Exception
    _actionMonitor.ExecuteAfterAction(Foo3);
}
private static void Foo2()
{

}
private static void Foo3()
{

}

FYI - that's the scenario Damien_The_Unbeliever is talking about in the comments.

To fix that issue the only 2 things that come in mind are

Don't call it like this, it's your class and your code is calling it so make sure you stick to your own rules
Get a copy of the _trigger list and enumarate this

About point 1, you could track if OnActionEnded is running and throw an exception if OnActionStarted is called while running:

private bool _isRunning = false;
public void OnActionEnded()
{
    lock (_lockObj)
    {
        try
        {
            _isRunning = true;

            IsInAction = false;
            foreach (var trigger in _triggers)
                trigger();
            _triggers.Clear();
        }
        finally
        {
            _isRunning = false;
        }
    }
}

public void OnActionStarted()
{
    lock (_lockObj)
    {        
        if (_isRunning)
            throw new NotSupportedException();

        IsInAction = true;
    }
}

About point 2, how about this

public class ActionMonitor
{
    public ActionMonitor()
    {

    }

    private object _lockObj = new object();

    public void OnActionEnded()
    {
        lock (_lockObj)
        {
            IsInAction = false;

            var tmpTriggers = _triggers;
            _triggers = new HashSet<Action>();
            foreach (var trigger in tmpTriggers)
                trigger();

            //have to decide what to do if _triggers isn't empty here, we could use a while loop till its empty
            //so for example

            while (true)
            {    
                var tmpTriggers = _triggers;
                _triggers = new HashSet<Action>();
                if (tmpTriggers.Count == 0)
                    break;

                foreach (var trigger in tmpTriggers)
                    trigger();
            }
        }
    }

    public void OnActionStarted()
    {
        lock (_lockObj) //fix the error @EricLippert talked about in comments
            IsInAction = true;
    }

    private ISet<Action> _triggers = new HashSet<Action>();
    public void ExecuteAfterAction(Action action)
    {
        lock (_lockObj)
        {
            if (IsInAction)
                _triggers.Add(action);
            else
                action();
        }
    }

    public bool IsInAction
    {
       get;private set;
    }
}

Collection was modified; enumeration operation may not execute even though the collection was modified exclusively in lock statements

2 Answers