11

One of my pet peeves with raising events in C# is the fact that an exception in an event handler will break my code, and possibly prevent other handlers from being called, if the broken one happened to get called first; In most cases my code couldn't care less if somebody else's code that's listening to its events is broken.

I created an extension method that catches exceptions:

public static void Raise(this EventHandler eh, object sender, EventArgs e)
{
  if (eh == null)
    return;
  try
  {
    eh(sender, e);
  }
  catch { }
}

Although this does mean my code carries on regardless, this method doesn't stop a first event handler throwing an exception and preventing second and subsequent handlers being notified of the event. I'm looking into a way of iterating through GetInvocationList to wrap each individual event handler in it's own try/catch, but this seems inefficient, and I'm not certain of the best way to do it, or even if I should be.

Also, I'm really not comfortable simply ignoring the exception here (and neither's FxCop/Resharper for that matter); realistically what should happen to the exception in this case?

Flynn1179
  • 11,925
  • 6
  • 38
  • 74
  • 1
    I've often wondered what the best way to handle this is. – ChaosPandion Jun 24 '10 at 21:00
  • This actually raises another question I'll ask separately; in this question I'm coding from the point of view of the class raising the event, but I'm wondering if event handlers should be allowed to throw exceptions in the first place. http://stackoverflow.com/questions/3114543/should-event-handlers-in-c-ever-raise-exceptions – Flynn1179 Jun 24 '10 at 23:07

4 Answers4

4

What if you did something like this?

public static void Raise(this EventHandler eh, object sender, EventArgs e)
{
    if (eh == null)
        return;

    foreach(var handler in eh.GetInvocationList().Cast<EventHandler>())
    {
        try
        {
            handler(sender, e);
        }
        catch { }
    }
}
ChaosPandion
  • 77,506
  • 18
  • 119
  • 157
  • Actually, I did `foreach (Delegate handler in WeaponChanged.GetInvocationList()) handler(sender, e);`, but unfortunately, the `handler(sender, e)` gives an error in VS, that 'Method, delegate or event is expected' for 'handler'; I'm scratching my head over that one, it's clearly a Delegate. – Flynn1179 Jun 24 '10 at 21:13
  • @Flynn1179 - Delegate by it self cannot be invoked in that matter. You either need to cast as you see in my example or invoke the delegate like this: `handler.DynamicInvoke(sender, e);` – ChaosPandion Jun 24 '10 at 21:16
  • aah.. I'm getting confused between 'delegate' and 'Delegate' :) – Flynn1179 Jun 24 '10 at 21:26
  • Ok, which is faster- casting and then doing `handler(sender, e)`, or not casting and doing `handler.DynamicInvoke(sender, e)`? I know I could do this with a quick test, but I'm curious as to WHY one is faster, or perhaps better – Flynn1179 Jun 24 '10 at 22:31
  • @Flynn1179 - As the name implies calling `DynamicInvoke` can take any arguments but at run-time the arguments will be validated. Casting the delegates to their proper types will remove the check and will in fact increase performance. Actual numbers are hard to gauge as it depends on how many subscribers you have and how often this event is raised. – ChaosPandion Jun 24 '10 at 23:11
  • One small problem I just discovered: `EventHandler` can't be cast to `EventHandler`; unfortunately both the generic and non-generic `EventHandler` classes inherit directly from MulticastDelegate, so it looks like I've got no choice but to go with the `DynamicInvoke` option. – Flynn1179 Jun 28 '10 at 13:31
3

You should only trap those exceptions you can handle. For example you might be accessing a file and not have sufficient privileges. If you know this can happen occasionally and all that needs to happen is that the case is logged then trap that exception alone.

If your code can't handle the exception then don't trap it.

It's an exceptional circumstance so there's a good chance the other event handlers won't be able to "do their stuff" either, so let it propagate to a level where it can be handled safely.

ChrisF
  • 134,786
  • 31
  • 255
  • 325
3

If the event handler didn't catch the exception, I don't see any good way you can handle the exception in the class throwing the event. You don't have any context to know what the handler is doing. It's safest to let the app crash hard in this case, as you have no idea what side effects the other event handler may have caused that could destabilize your application. The lesson here is that event handlers must be robust (handle their own exceptions), as classes that fire events should never catch exceptions thrown by the handlers.

Dan Bryant
  • 27,329
  • 4
  • 56
  • 102
  • Failing fast is great when you're debugging, but not so much when you're a user. Why should a buggy plug-in crash every app it touches? – Gabe Jun 24 '10 at 21:09
  • If you have a buggy plug-in and you want a true robust architecture, then you need to isolate the plugin so that it can't harm your application. You can do this with an AppDomain or a separate process. If it's not isolated, then you can't guarantee your state hasn't been corrupted and continuing to run despite an unhandled exception is a recipe for much worse failures down the line. Basically, running incorrectly is worse than not running at all and if you can't be sure that you can still run correctly, it's better to fail. – Dan Bryant Jun 24 '10 at 21:12
  • 1
    @Gabe - A buggy plug-in should crash every app it touches, because it is a buggy plug-in. People should be made aware of the fact that it's buggy, so it can be fixed or avoided. Protecting lousy plug-ins at the possible expense of user's data is a bad policy. – Jeffrey L Whitledge Jun 24 '10 at 21:17
  • @Dan - Nothing is set in stone, it may be that they **have** to do this so the service or job can continue processing. – ChaosPandion Jun 24 '10 at 21:17
  • 1
    @Chaos, I've been in that boat before, where the perceived cost of a customer seeing a 'crash' was such that there was pressure to keep the app running, at all costs. I've become increasingly resistant to that line of thought as I accumulate more experience of the kinds of ugly consequences that can result from letting unknown failures slide. – Dan Bryant Jun 24 '10 at 21:34
  • @Dan - Don't get me wrong I never eat exceptions. It is just that people throw around these rules saying that under no circumstance should they be broken. – ChaosPandion Jun 24 '10 at 21:44
  • @Chaos, I don't think of it as a rule so much as a warning. If I stumble into a minefield (and survive), then I figure it's worth mentioning where I found the mines. – Dan Bryant Jun 24 '10 at 21:54
  • Maybe I've just never been in a situation where crashing for a user was actually better than not crashing, but it's hard for me to imagine one. – Gabe Jun 25 '10 at 03:28
  • @Gabe - I'm dealing with that situation right now. Months of data is missing because the process that was supposed to store the data has been silently failing. I'm about to release a new version that doesn't swallow any errors. Now the problems will be addressed when they arise rather than months later when it's too late. – Jeffrey L Whitledge Jun 25 '10 at 15:13
  • @Gabe - It boils down to this: if it's OK for a piece of code to silently fail, then that piece of code should not have been put in the program in the first place. If a feature is so unnecessary that nobody needs to know when it breaks, then it's useless bloat that should be deleted immediately. – Jeffrey L Whitledge Jun 25 '10 at 15:17
  • First of all, "not crashing" is different from "silently failing". Second of all, trying to create a directory that already exists or deleting a file that's not there aren't even bugs, so why assume that all exceptions are due to corruption? Third, it's well-known that the Flash plug-in is the top cause of browser crashes, yet it hasn't been fixed and I can't avoid it, so in a similar vein I can't ask my customers to fix or avoid other vendors' buggy plug-ins. – Gabe Jun 25 '10 at 16:54
  • @Jeffrey L. Whitledge: I would consider things like progress bar updates as something which may legitimately silently fail, but are not useless. The *user* will care whether the progress bars get updated, but the *code* won't. If a program allows the user to open a window showing the states of tasks in progress and the user closes that window asynchronously causing the BeginInvokes to fail, silently ignoring such errors would seem entirely appropriate. – supercat Mar 21 '11 at 16:18
  • @supercat - Why would the progress window throw exceptions just because it has been closed? Why would those unnecessary exceptions not be caught and ignored by the code that knows they're useless (i.e., the progress bar updating code)? Why should the long-running process that invokes the progress updates need to distinguish between important exceptions and unimportant ones? – Jeffrey L Whitledge Mar 22 '11 at 12:58
  • @Jeffery L Whitledge: My main point was that there are times when it is entirely proper for useful code to silently fail. An example would be a property-update event which calls a control's BeginInvoke with semantics ("If you still exist, you should display X"); it may fail if the control gets disposed at the wrong time. Any such event listeners that are going to silently fail should of course catch and stifle events themselves rather than letting them escape; for a number of "property update event" scenarios, though, failing silently is a reasonable behavior. – supercat Mar 22 '11 at 14:54
  • @Jeffery L Whitledge: In many cases, I would aver that events should be invoked in such a way that an exception thrown by the first will not prevent execution of the remainder, but a thrown exception should propagate after all events have executed (things get awkward if multiple events throw exceptions; I wish there were a standard convention for where to store excess exceptions in the events' Data property). Many data structures require property-update events to maintain consistency. If e.g. a property-update handler which is supposed to update a UI element throws an exception... – supercat Mar 22 '11 at 14:59
  • @Jeffery L Whitledge: ...and that prevents the execution of other property-update handlers that were needed to maintain consistency, that could result in data corruption that would have been avoided had event handing been allowed to run to conclusion before propagating the exception. Of course, if one is going to manually invoke event handlers in such a way as to allow full processing before exceptions propagate, one may as well keep the handlers as a list rather than as a MulticastDelegate. – supercat Mar 22 '11 at 15:03
2

A simple but important rule:

Every defect in code should be as lethal as possible as early as possible.

In this way, the bugs are found and fixed, and the software grows ever stronger. What the others are saying is correct; never catch an exception that you don't expect and know how to deal with.

Jeffrey L Whitledge
  • 58,241
  • 9
  • 71
  • 99
  • 1
    Everyone keep saying this but it is not always so simple. What if they have no control over who subscribes to the event? What if it is imperative that processing continues? – ChaosPandion Jun 24 '10 at 21:27
  • 1
    @Chaos, in that case, you need to structure your events in a way that allows for isolation. For example, set up a client/server model with isolated processes. Now a client can crash and the server can happily keep processing, even if the clients are buggy. If you have simple callback events with no isolation boundaries, then there is no protection from an unruly client interfering with your processing. The process can't guarantee its imperative (to continue processing _correctly_) and it's irresponsible for the process to pretend that it can. – Dan Bryant Jun 24 '10 at 21:38
  • Dan: Assuming this is all managed code, how would you expect a buggy exception handler to interfere with the main process? – Gabe Jun 25 '10 at 04:42
  • @Gabe, I'm assuming you mean event handler. The problem is that it likely lives on a class that has access to shared context. For instance, it may be using the database or it may be sending commands to some piece of machinery. If an exception occurs, it's possible that some damage has already been done, but if you ignore the unknown exception, you simply allow the damage to compound. That said, you can minimize the possibility for damage through good use of defensive coding practices. Better yet, handle known exceptions at the service layer and they won't show up in the event handler. – Dan Bryant Jun 25 '10 at 16:18
  • 1
    Dan: Yes, I meant an event handler. My point was that the OP seemed to have a situation where his code wasn't coupled with the event handler's code. I mean, why assume that the damage is limited to the app and only crash the app? You don't know what kinds of horrible things this event handler was tied to, so why not crash the whole OS? – Gabe Jun 25 '10 at 16:59
  • @Gabe, The OS creates isolation boundaries precisely so that it doesn't have to crash if a process fails. If something fails in the kernel, however, Windows will blue-screen, Linux will kernel-panic, etc. It's possible to limit the causalities, but it requires careful design and an understanding of what's being contained. If the exception is unknown, then very significant isolation is required (such as an AppDomain or second process) to guarantee internal state is preserved. – Dan Bryant Jun 25 '10 at 17:54
  • Dan: There seem to be two main reasons to crash when an unknown exception hits: one is to force you to debug it, and the other is to prevent further corruption. If you're trying to prevent further corruption, why assume that the whole app is corrupted? Why assume only the app is corrupted? Why assume that crashing the app won't cause further corruption? – Gabe Jun 25 '10 at 19:46
  • @Dan Bryant: I would aver that while it may be sensible for an unexpected exception that occurs in code that mutates an object to invalidate the object in question (so any further attempt to use the object will throw an exception, possibly with a saved copy of the earlier exception) I see no reason to expect corruption in objects that code wasn't expected to mutate. Many events are designed to mutate only objects outside the one raising them. A failure of one such handler should imply nothing about the object raising the event, nor about other handlers. – supercat Aug 03 '11 at 15:38