Are `System.MulticastDelegate`'s thread-safe?

Question

I'm looking for someone that might know more about this, my gut tells me that the answer is "no, it is not thread-safe" but I want to be sure.

In order to illustrate my question, I have provided some context with this class

public class MyContext
{
    private readonly object _lock = new object();
    public delegate bool MyDelegate(MyContext context);
    private MyDelegate _multicastDelegate;

    public MyContext()
    {
        _multicastDelegate = null;
    }

    public void AddDelegate(MyDelegate del)
    {
        lock(_lock)
        {
            _multicastDelegate += del;
        }
    }

    public void RemoveDelegate(MyDelegate del)
    {
        lock(_lock)
        {
            _multicastDelegate += del;
        }
    }

    public void Go()
    {
        _multicastDelegate.Invoke(this);
    }
}

Edit: I added locks to the example above, but it's really not the point of my question.

---

I am trying to understand better if the array that holds the invocation list is thread-safe. Frankly, I am not clearly seeing how this all gets put together and some help would be appreciated.

According to documentation I have found, the only quote that provides no real insight is the following:

A MulticastDelegate has a linked list of delegates, called an invocation list, consisting of one or more elements. When a multicast delegate is invoked, the delegates in the invocation list are called synchronously in the order in which they appear. If an error occurs during execution of the list then an exception is thrown.

https://msdn.microsoft.com/en-us/library/system.multicastdelegate.aspx

Thanks in advance.

Answer 1

Delegates are immutable. You never change a delegate. Any method that appears to mutate a delegate is in fact creating a new instance.

Delegates are immutable; once created, the invocation list of a delegate does not change.

There is thus no cause for concern that the invocation list may be updated whilst a delegate is being invoked.

What you do have to guard against, however, and have failed to do in your method is when the delegate may in fact be null.

(new MyContext()).Go();

will cause an exception. You used to have to guard against this by reading the value into a local variable, testing it for null and then invoking using it. It can now more easily be resolved as:

public void Go()
{
    _multicastDelegate?.Invoke(this);
}

Answer 2

The definition of thread-safe as used by MSDN documentation means code that is properly synchronized. It usually doesn't state on what it synchronizes, but it can be the class object for static members, the instance object for instance members, or it can be some inner object, such as SyncRoot in many collection types.

Although delegates are immutable, you must still synchronize properly. .NET and C#, unlike Java, don't guarantee safe publication, so if you don't guarantee synchronization, you can observe a partially initialized object in other threads¹.

To turn your code thread-safe, you just need to use _lock when reading from the delegate field as well, but you can call Invoke outside the lock, landing onto the delegate the responsibility to keep its own thread safety.

public class MyContext
{
    private readonly object _lock = new object();
    public delegate bool MyDelegate(MyContext context);
    private MyDelegate _delegate;

    public MyContext()
    {
    }

    public void AddDelegate(MyDelegate del)
    {
        lock (_lock)
        {
            _delegate += del;
        }
    }

    public void RemoveDelegate(MyDelegate del)
    {
        lock (_lock)
        {
            // You had a bug here, +=
            _delegate -= del;
        }
    }

    public void Go()
    {
        MyDelegate currentDelegate;
        lock (_lock)
        {
            currentDelegate = _delegate;
        }
        currentDelegate?.Invoke(this);
    }
}

---

1. Microsoft's .NET Framework implementation always makes volatile writes (or so they say), which kind of gives you safe publication implicitly, but I personally don't rely on this.