How to implement a thread safe event handler without errors in C #?

Question

Problem background

An event can have several subscribers (i.e., when an event is called, several handlers can be called). Since any of the handlers may throw an error, and this will prevent the rest of them from being called, I want to ignore any errors that occur from each individual handler. In other words, I don’t want an error in one handler to disrupt the execution of other handlers in the call list, because neither of these handlers or the event publisher has any control over what any specific event handler code does.

This can be easily done with the code:

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

General, thread-safe, error-free solution

Of course, I don’t want to write all this common code over and over every time the event is called, so I wanted to encapsulate it in a common class. In addition, I really need additional code to ensure thread safety so that the MyEvent call list does not change while the method list is running.

I decided to implement this as a generic class, where the generic type is limited to the where clause, which is the delegate. I really wanted the constraint to be “delegate” or “event”, but they are not valid, so using a delegate as a base class constraint is the best I can do. Then I create a lock object and lock it in an open event, adding and removing methods that modify the 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 problem with + = operator, but two simple workarounds

One problem was that the string "event_handlers + = value;" leads to a compiler error “Operator” + = 'cannot be applied to the types “EventType” and “EventType". Despite the fact that EventType is limited by the type of delegation, it will not allow it to use the + = operator.

As a workaround, I just added the event keyword to event_handlers, so the definition looks like " private event EventType event_handlers; " and it compiles fine. But I also thought that since the keyword “event” could generate code to handle this, I should also be able to, so I ended up changing it to this to avoid the compiler being unable to recognize that “+ =” SHOULD refer to general type, limited to be a delegate. The private variable "event_handlers" is now entered as "Delegate" instead of the general type EventType, and the add / remove methods follow this pattern event_handlers = MulticastDelegate.Combine( event_handlers, value );

The final code is as follows:

 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{} } } 

Question

My question is ... does this seem to work well? Is there a better way or is it basically how it should be done? I think I have exhausted all the options. Using a lock in the methods of adding / removing a public event (supported by a private delegate), as well as using the same lock when executing a call list, is the only way that you can make the call list thread safe, and also ensure errors caused by the handlers do not interfere call other handlers.

Answer 1

Since any of the handlers may cause an error, and this will prevent the rest of them from being called,

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

Running more code at the moment is likely to make things worse, not better. The safest thing to do when this happens is to detect the situation and call failfast, which removes the entire process without running more code. You do not know what a terrible thing will be launched at this stage.

I want to ignore any errors coming from each handler.

This is a very dangerous idea. These exceptions tell you that something terrible is happening and you are ignoring them.

In other words, I don’t want an error in one handler to disrupt the execution of other handlers in the call list, because neither handlers or event publisher have any control over what any specific event handler code does.

Whose responsibility is here? Someone adds these event handlers to this event. This is the code that is responsible for ensuring that event handlers act correctly if there is an exception.

Then I create a lock object and lock it in the add and remove methods of an open event that change the private delegate variable called "event_handlers".

Of course, that’s good. I doubt the necessity of this function - I rarely have a situation where several threads add event handlers to an event - but I will take your word for it that you are in this situation.

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

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

Think about what's wrong there.

Thread Alpha enters collection lock.

Suppose there is another foo resource that is also controlled by another lock. Thread Beta enters foo lock to get some data that it needs.

Beta then takes this data and tries to lock the collection because it wants to use the contents of foo in the event handler.

Beta Thread is now waiting on the Alpha thread. Thread Alpha now calls a delegate who decides that he wants to access foo. So he waits in the stream of Beta, and now we have a dead end.

But can't we avoid this by ordering locks? No, because the premise of your script is that you do not know what event handlers do. If you already know that event handlers behave well in relation to blocking them, then you can probably also know that they behave well in order to not throw exceptions, and the whole problem disappears.

Ok, let's assume that you do this:

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

Delegates are immutable and atomically copied by reference, so now you know that you are going to reference the contents of event_delegate, but do not hold the lock during the call. Does it help?

Not really. You sold one problem for another:

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

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

Thread Alpha grabs and launches X from the copy again. Because Beta simply destroyed the state necessary for X, X deadlocks to execute correctly. And again, you are at a standstill.

Event handlers should not do this; they must be strong in the face of their sudden becoming "stagnation." It looks like you are in a scenario in which you cannot trust event handlers to be well written. This is a terrible situation; then you cannot trust any code to be reliable in this process. It seems that you think that there is a certain level of isolation that you can impose on the event handler, catching all its errors and getting confused, but this does not happen. Event handlers are just code, and they can affect an arbitrary global state in a program, like any other code.

---

In short, your solution is generic, but it is not thread safe, and it is not error free. Rather, it exacerbates threading problems, such as interlocks, and disables security systems.

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

If they are wrong and ultimately throw exceptions, then clear this process immediately . Don't get confused trying to run code that is currently living in an unstable process.

Based on your comments on other answers, it seems you think you can get candy from strangers without any side effects. You cannot, not without unnecessary isolation. You cannot simply register a random code perforce for events in your process and hope for the best. If you have things that are unreliable because your application uses third-party code, you need a managed add-in structure to provide isolation. Try to find MEF or MAF.

Answer 2

Locking inside RaiseEventSafely is unnecessary and dangerous.

This is optional because delegates are immutable. After you read this, the activation list that you received will not change. It doesn’t matter if changes occur when the code is triggered, or if the changes should wait until the next.

This is dangerous because you are calling external code while holding the lock. This can easily lead to violations of the locking order and thus to deadlocks. Consider an event handler that spawns a new thread that is trying to change an event. Boom, dead end.

You have an empty catch for exception . This is rarely a good idea, because it silently swallows an exception. At a minimum, you should file an exception.

Your general parameter does not start with T This is a bit confusing IMO.

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

Answer 3

Have you studied the PRISM EventAggregator or the MVVMLight Messenger class ? Both of these classes meet all your requirements. The MVVMLight Messenger class uses WeakReferences to prevent memory leaks.

Answer 4

Besides the bad idea of ​​catching exceptions, I suggest you not block while invoking the delegate list.

You will need to add a note to your class documentation that delegates can be called after being removed from the event.

The reason I will do this is because otherwise you risk performance consequences and possibly deadlocks. You have a lock when calling another user's code. Let me call your inner Lock Lock 'A'. If one of the handlers tries to get a private lock "B", and someone tries to register a handler in a separate thread by holding the lock "B", then one thread holds the lock "A" while trying to get the "B" and the other thread holds the lock "B" "while trying to get lock" A ". Dead end.

Third-party libraries, like yours, are often written without thread safety to avoid such problems, and clients must protect methods that access internal variables. I think it is reasonable for the class of events to ensure thread safety, but I think the risk of a late callback is better than a poorly defined locking hierarchy prone to deadlocks.

The last nit-pick, do you think SafeEventHandler really describes what this class does? It looks like an event logger and dispatcher for me.

Answer 5

Bad practice is to completely eliminate exceptions. If you have a use case when you want the publisher to gracefully recover the error caused by the subscriber, then this requires the use of an event aggregator.

Also, I'm not sure if I am following the code in SafeEventHandler.RaiseEventSafely. Why is an event delegate a parameter? This does not seem to be related to the event_handlers field. As for thread safety, after calling GetInvocationList it doesn't matter if the original delegate collection is changed, because the returned array will not change.

If you need, I would suggest doing the following:

 class MyClass { event EventHandler myEvent; public event EventHandler MyEvent { add { this.myEvent += value.SwallowException(); } remove { this.myEvent -= value.SwallowException(); } } protected void OnMyEvent(EventArgs args) { var e = this.myEvent; if (e != null) e(this, args); } } public static class EventHandlerHelper { public static EventHandler SwallowException(this EventHandler handler) { return (s, args) => { try { handler(s, args); } catch { } }; } } 

Answer 6

Juval Löwy provides an implementation of this in his book "Programming .NET Components".

http://books.google.com/books?id=m7E4la3JAVcC&lpg=PA129&pg=PA143#v=onepage&q&f=false

Answer 7

I reviewed everything everyone said and came to the following code:

 public class SafeEvent<EventDelegateType> where EventDelegateType:class { private object collection_lock = new object(); private Delegate event_handlers; public SafeEvent() { if(!typeof(Delegate).IsAssignableFrom( typeof(EventDelegateType) )) throw new ArgumentException( "Generic parameter must be a delegate type." ); } public Delegate Handlers { get { lock (collection_lock) return (Delegate)event_handlers.Clone(); } } public void AddEventHandler( EventDelegateType handler ) { lock(collection_lock) event_handlers = Delegate.Combine( event_handlers, handler as Delegate ); } public void RemoveEventHandler( EventDelegateType handler ) { lock(collection_lock) event_handlers = Delegate.Remove( event_handlers, handler as Delegate ); } public void Raise( object[] args, out List<Exception> errors ) { lock (collection_lock) { errors = null; foreach (Delegate handler in event_handlers.GetInvocationList()) { try {handler.DynamicInvoke( args );} catch (Exception err) { if (errors == null) errors = new List<Exception>(); errors.Add( err ); } } } } } 

This circumvents the compiler’s special approach to the delegate as an invalid base class. In addition, events cannot be entered as a delegate.

Here's how SafeEvent will be used to create an event in the class:

 private SafeEvent<SomeEventHandlerType> a_safe_event = new SafeEvent<SomeEventHandlerType>(); public event SomeEventHandlerType MyEvent { add {a_safe_event.AddEventHandler( value );} remove {a_safe_event.RemoveEventHandler( value );} } 

And here is how the event and errors will occur:

 List<Exception> event_handler_errors; a_safe_event.Raise( new object[] {event_type, disk}, out event_handler_errors ); //Report errors however you want; they should not have occurred; examine logged errors and fix your broken handlers! 

To summarize, this component task is to publish events to the list of subscribers in an atomic way (i.e. the event will not be re-raised and the call list will not be changed during the execution of the call list). A deadlock is possible, but it can be easily avoided by controlling access to SafeEvent, because the handler would have to spawn a thread that calls one of the SafeEvent public methods, and then wait for that thread. In any other scenario, other threads are simply blocked until the lock-thread releases the lock. In addition, despite the fact that I do not believe in ignoring errors at all, I also do not believe that this component is not able to deal with subscriber errors reasonably and not judge the severity of such errors, so instead of throwing them and risking the application reports “Raise” errors to the caller, as the caller is likely in a better position to handle such errors. With that said, these components provide some stability for events that are missing in the C # event system.

I think people are worried that letting other subscribers run after an error has occurred means that they are working in an unstable context. While this may be true, it does mean that the application is actually spelled incorrectly in any way that you look at it. Failure is not a better solution than permission to run code, because permission to run code allows you to report errors and allows you to show the full consequences of the error, and this, in turn, will help engineers more quickly and fully understand the nature of the error and fix its code.