How to implement a thread safe error-free event handler in C#?

Question

Problem background

An event can have multiple subscribers (i.e. multiple handlers may be called when an event is raised). Since any one of the handlers could throw an error, and that would prevent the rest of them from being called, I want to ignore any errors thrown from each individual handler. In other words, I do not want an error in one handler to disrupt the execution of other handlers in the invocation list, since neither those other handlers nor the event publisher has any control over what any particular event handler's code does.

This can be accomplished easily with code like this:

public event EventHandler MyEvent;
public void RaiseEventSafely( object sender, EventArgs e )
{
    foreach(EventHandlerType handler in MyEvent.GetInvocationList())
        try {handler( sender, e );}catch{}
}

A generic, thread-safe, error-free solution

Of course, I don't want to write all this generic code over and over every time I call an event, so I wanted to encapsulate it in a generic class. Furthermore, I'd actually need additional code to ensure thread-safety so that MyEvent's invocation list does not change while the list of methods is being executed.

I decided to implement this as a generic class where the generic type is constrained by the "where" clause to be a Delegate. I really wanted the constraint to be "delegate" or "event", but those are not valid, so using Delegate as a base class constraint is the best I can do. I then create a lock object and lock it in a public event's add and remove methods, which alter a private delegate variable called "event_handlers".

public class SafeEventHandler<EventType> where EventType:Delegate
{
    private object collection_lock = new object();
    private EventType event_handlers;

    public SafeEventHandler(){}

    public event EventType Handlers
    {
        add {lock(collection_lock){event_handlers += value;}}
        remove {lock(collection_lock){event_handlers -= value;}}
    }

    public void RaiseEventSafely( EventType event_delegate, object[] args )
    {
        lock (collection_lock)
            foreach (Delegate handler in event_delegate.GetInvocationList())
                try {handler.DynamicInvoke( args );}catch{}
    }
}

Compiler issue with += operator, but two easy workarounds

One problem ran into is that the line "event_handlers += value;" results in the compiler error "Operator '+=' cannot be applied to types 'EventType' and 'EventType'". Even though EventType is constrained to be a Delegate type, it will not allow the += operator on it.

As a workaround, I just added the event keyword to "event_handlers", so the definition looks like this "private event EventType event_handlers;", and that compiles fine. But I also figured that since the "event" keyword can generate code to handle this, that I should be able to as well, so I eventually changed it to this to avoid the compiler's inability to recognize that '+=' SHOULD apply to a generic type constrained to be a Delegate. The private variable "event_handlers" is now typed as Delegate instead of the generic EventType, and the add/remove methods follow this pattern event_handlers = MulticastDelegate.Combine( event_handlers, value );

The final code looks like this:

public class SafeEventHandler<EventType> where EventType:Delegate
{
    private object collection_lock = new object();
    private Delegate event_handlers;

    public SafeEventHandler(){}

    public event EventType Handlers
    {
        add {lock(collection_lock){event_handlers = Delegate.Combine( event_handlers, value );}}
        remove {lock(collection_lock){event_handlers = Delegate.Remove( event_handlers, value );}}
    }

    public void RaiseEventSafely( EventType event_delegate, object[] args )
    {
        lock (collection_lock)
            foreach (Delegate handler in event_delegate.GetInvocationList())
                try {handler.DynamicInvoke( args );}catch{}
    }
}

The Question

My question is... does this appear to do the job well? Is there a better way or is this basically the way it must be done? I think I've exhausted all the options. Using a lock in the add/remove methods of a public event (backed by a private delegate) and also using the same lock while executing the invocation list is the only way I can see to make the invocation list thread-safe, while also ensuring errors thrown by handlers don't interfere with the invocation of other handlers.

Answer 1

Since any one of the handlers could throw an error, and that would prevent the rest of them from being called,

You say that like it is a bad thing. That is a very good thing. When an unhandled, unexpected exception is thrown that means that the entire process is now in an unknown, unpredictable, possibly dangerously unstable state.

Running more code at this point is likely to make things worse, not better. The safest thing to do when this happens is to detect the situation and cause a failfast that takes down the entire process without running any more code. You don't know what awful thing running more code is going to do at this point.

I want to ignore any errors thrown from each individual handler.

This is a super dangerous idea. Those exceptions are telling you that something awful is happening, and you're ignoring them.

In other words, I do not want an error in one handler to disrupt the execution of other handlers in the invocation list, since neither those other handlers nor the event publisher has any control over what any particular event handler's code does.

Whose in charge here? Someone is adding those event handlers to this event. That is the code that is responsible for ensuring that the event handlers do the right thing should there be an exceptional situation.

I then create a lock object and lock it in a public event's add and remove methods, which alter a private delegate variable called "event_handlers".

Sure, that's fine. I question the necessity of the feature -- I very rarely have a situation where multiple threads are adding event handlers to an event -- but I'll take your word for it that you are in this situation.

But in that scenario this code is very, very, very dangerous:

    lock (collection_lock)
        foreach (Delegate handler in event_delegate.GetInvocationList())
            try {handler.DynamicInvoke( args );}catch{}

Let's think about what goes wrong there.

Thread Alpha enters the collection lock.

Suppose there is another resource, foo, which is also controlled by a different lock. Thread Beta enters the foo lock in order to obtain some data that it needs.

Thread Beta then takes that data and attempts to enter the collection lock, because it wants to use the contents of foo in an event handler.

Thread Beta is now waiting on thread Alpha. Thread Alpha now calls a delegate, which decides that it wants to access foo. So it waits on thread Beta, and now we have a deadlock.

But can't we avoid this by ordering the locks? No, because the very premise of your scenario is that you don't know what the event handlers are doing! If you already know that the event handlers are well-behaved with respect to their lock ordering then you can presumably also know that they are well-behaved with respect to not throwing exceptions, and the whole problem vanishes.

OK, so let's suppose that you do this instead:

    Delegate copy;
    lock (collection_lock)
        copy = event_delegate;
    foreach (Delegate handler in copy.GetInvocationList())
        try {handler.DynamicInvoke( args );}catch{}

Delegates are immutable and copied atomically by reference, so you now know that you're going to be invoking the contents of event_delegate but not holding the lock during the invocation. Does that help?

Not really. You've traded one problem for another one:

Thread Alpha takes the lock and makes a copy of the delegate list, and leaves the lock.

Thread Beta takes the lock, removes event handler X from the list, and destroys state necessary to prevent X from deadlocking.

Thread Alpha takes over again and starts up X from the copy. Because Beta just destroyed state necessary for the correct execution of X, X deadlocks. And once more, you are deadlocked.

Event handlers are required to not do that; they are required to be robust in the face of their suddenly becoming "stale". It sounds like you are in a scenario where you cannot trust your event handlers to be well-written. That's a horrid situation to be in; you then cannot trust any code to be reliable in the process. You seem to think that there is some level of isolation you can impose on an event handler by catching all its errors and muddling through, but there is not. Event handlers are just code, and they can affect arbitrary global state in the program like any other code.

---

In short, your solution is generic, but it is not threadsafe and it is not error-free. Rather, it exacerbates threading problems like deadlocks and it turns off safety systems.

You simply cannot abdicate responsibility for ensuring that event handlers are correct, so don't try. Write your event handlers so that they are correct -- so that they order locks correctly and never throw unhandled exceptions.

If they are not correct and end up throwing exceptions then take down the process immediately. Don't keep muddling through trying to run code that is now living in an unstable process.

Based on your comments on other answers it looks like you think that you should be able to take candy from strangers with no ill effects. You cannot, not without a whole lot more isolation. You can't just sign up random code willy-nilly to events in your process and hope for the best. If you have stuff that is unreliable because you're running third party code in your application, you need a managed add-in framework of some sort to provide isolation. Try looking up MEF or MAF.

Answer 2

The lock inside RaiseEventSafely is both unnecessary and dangerous.

It is unnecessary because delegates are immutable. Once you read it, the invokation list you obtained will not change. It doesn't matter if the changes happen while event code runs, or if the changes need to wait until after.

It is dangerous because you're calling external code while holding a lock. This can easily lead to lock order violations and thus deadlocks. Consider an eventhandler that spawns a new thread that tries to modify the event. Boom, deadlock.

The you have an empty catch for exception. That's rarely a good idea, since it silently swallows the exception. At minimum you should log the exception.

Your generic parameter doesn't start with a T. That's a bit confusing IMO.

where EventType:Delegate I don't think this compiles. Delegate is not a valid generic constraint. For some reason the C# specification forbids certain types as a generic constraint, and one of them is Delegate. (no idea why)

Answer 3

Have you looked into the PRISM EventAggregator or MVVMLight Messenger classes? Both of these classes fulfill all your requirements. MVVMLight's Messenger class uses WeakReferences to prevent memory leaks.