40

As a general rule, are there ever any circumstances in which it's acceptable for a method responsible for listening to an event to throw an exception (or allow to be thrown) that the class raising the event will have to handle?

Given that such an exception would stop other listeners to that event from being called subsequently, it seems a bit 'antisocial' to allow this to happen, but on the other hand, if there is an exception, what should it do?

Flynn1179
  • 11,925
  • 6
  • 38
  • 74
  • There is the CancelEventArgs too. http://msdn.microsoft.com/en-us/library/system.componentmodel.canceleventargs.aspx – simendsjo Jun 24 '10 at 23:09
  • I don't understand what you mean, here. You mean on the recieveing side of say Button1_Click, you want to throw an exception? – Robert Seder Jun 24 '10 at 23:11
  • @Rob: Say you have a long running process with an Finished event. 10 methods registers with this. If one of them throws an exception, the rest wont be called. – simendsjo Jun 24 '10 at 23:16
  • `event handler to throw an exception` ? probably a **delegate** can throw an exception, but a handler..?! – serhio Jun 24 '10 at 23:17
  • @serhio: I believe he is asking whether a delegate attached to the event (ie: the method handling the event) should ever throw - – Reed Copsey Jun 24 '10 at 23:18
  • @Rob: Yes, although it might not be my code that's throwing the exception; for example that button_click might save a file which throws an IOException of some description. Probably not the best example though, I can't see many reasons to have more than one handler for a button click, but you get the idea; rather than a button click, it could be a 'data received' event which one handler needs to respond to by saving it, and a second responds to by updating the screen, for example. Even if the save failed, I'd still want the screen updated. – Flynn1179 Jun 24 '10 at 23:19
  • @Reed Copsey: So, first of all, he should understand what he asks. Maybe if he understand the difference he could explain better, change the question, or even the question could disappear. – serhio Jun 24 '10 at 23:21
  • 2
    Valid point; I've clarified it a little, referring to a 'method responsible for listening to an event' rather than an 'event handler'. – Flynn1179 Jun 24 '10 at 23:26

5 Answers5

29

Throwing an exception from a event handler is in many ways similar to throwing an exception from a IDisposable.Dispose method (or a C++ destructor). Doing so creates havoc for your caller because you leave them with little option.

  1. Ignore the exception and let it propagate. This breaks their contract to inform all listeners of an event. This is a very real problem if anyone above them on the stack catches the exception.
  2. Catch it call the other handlers and rethrow. But what happens if one of the others throw as well?
  3. Swallow the exception. This is just bad in general. Event sources should have no knowledge of their caller and hence can't know what they're swallowing.
  4. Crash the process because you're toast.

Of all of these #4 is the best option. But this is rarely done and can't be counted on.

I think in your component you really only have a few options

  • You are calling the code which is throwing and are in the best position to handle the exception. If it's not handleable by you then it's unreasonable to expect it to be handled by anyone else. Hence crash the process and be done with it.
  • Don't call the API which throws
JaredPar
  • 733,204
  • 149
  • 1,241
  • 1,454
  • 1
    "This breaks their contract to inform all listeners" - disagree. There is no contract to inform all listeners if an exception is thrown. "Catch it call the other handlers and rethrow." - how? .NET events don't provide you with a list of "other handlers". "Crash the process..." - it doesn't crash the process, the exception will be propagated to the code that raised the event, and will be handled like any other exception. The process will only crash if there is no top level handler, again like any other exception. – Joe Jun 25 '10 at 05:52
  • 6
    @Joe the contract for an event is that all listeners will be notified. Failing to notify them is a violation of that contract. What good is the `Button.Click` event if I can't depend on my handler actually executing when the button is clicked? – JaredPar Jun 25 '10 at 06:44
  • 8
    @Joe (cont) in this case having a top level handler is the worst thing that can happen. Throwing in an event handler is a violation of the contract of the vast majority of events and causes broken assumptions in other components. This will almost certainly lead to future errors and / or exceptions in those components. These failures are far from the original real failure and hence much harder to track down. Fail fast and fail loud. – JaredPar Jun 25 '10 at 06:47
  • 1
    @JaredPar - I disagree that there is any such contract when an exception is thrown. An exception could be thrown in an overridden OnClick method before *any* handler is executed - in which case *no* handler is notified. Of course you may have specific situations where you define such a contract in your own app, but it's not true for events in general. Are you saying an ASP.NET Page.Load event handler that doesn't swallow exceptions is breaking a contract? – Joe Jun 25 '10 at 06:54
  • @JaredPar - "having a top level handler is the worst thing that can happen" - but that's exactly what happens in an ASP.NET app if a Page.Load handler throws. Whether there should be a top-level handler or fail-fast is orthogonal to this question - the situation is exactly the same whether the exception is thrown from an event handler or from any other code. – Joe Jun 25 '10 at 06:56
  • "Throwing in an event handler is a violation of the contract of the vast majority of events and causes broken assumptions in other components" - examples would help to understand what you mean here. And why it represents the "vast majority" of cases. – Joe Jun 25 '10 at 06:59
  • @Joe The standard example is if you know that 2 events must be ordered or paired. Take for example a transaction Opened / Closed pair. It's reasonable to write code against the opening event coming before the closed event. However that breaks down if someone ahead of you throws during the Opened, the exception is caught above and the transaction is later closed. At this point I receive a close before an open and my assumptions are violated and leads to an error in my code. In this case the exception is not caught, a bug report is filed and investigation starts with me. – JaredPar Jun 25 '10 at 07:12
  • @Joe (cont) it's reasonable to say the blame lies in the Transaction for not ensuring all of it's contracts were met in the face of an exception. But how could it do so? It has now sent an Open sucessfully to a fraction of it's listeners. It still owes them a Close (but not the rest). It could try to raise the Close event for only those who saw the Open but what if one of them throws? Now suddenly it has 2 exceptions and a whole bucket of lot of bad state to deal with. – JaredPar Jun 25 '10 at 07:15
  • @JaredPar - sound a bit contrived, and an odd way to implement transactions. I agree that you can design specific cases where your contract requires event handlers to swallow exceptions, even if I think that such a design is fragile. What I disagree with is that such a contract exists in the general case, and certainly not for the "vast majority" of events in the .NET Framework. – Joe Jun 25 '10 at 07:48
  • @JaredPar - Although I'm not disputing it, indeed I think it's a damn good thing, but I've never heard of this contract before- is it documented anywhere from Microsoft? – Flynn1179 Jun 25 '10 at 08:19
  • @Joe not contrived at all. That's a real world bug I've encountered and we spent quite some time working out the fix. I disagree about the contract not existing in the vast majority of events. I counter with the question "For what event is it acceptable to not notify all handlers when the event occurs?". If this is acceptable reliable programs cannot be created. – JaredPar Jun 25 '10 at 15:06
  • @JaredPar. ASP.NET Page.Load would be one event for which it's acceptable to not notify all handlers (but only in the case of an exception of course). I disagree with the assertion that this means reliable programs cannot be created, because this would mean that any program with event handlers that don't swallow exceptions was unreliable. And I think almost every .NET app I've ever seen has at least one event handler that doesn't swallow exceptions. But I think we will just have to agree to differ on this... – Joe Jun 25 '10 at 17:40
  • @Joe, to clarify. I don't think many (if any) .Net event handlers swallow exceptions (that would be a bad thing). They have an implicit dependency on their handlers not throwing. – JaredPar Jun 25 '10 at 18:21
  • For two approaches to explicitly passing Exception back (instead of just letting it throw), see my answer to http://stackoverflow.com/questions/17573467/exception-handling-in-delegate/17573857 And although it is for threads, not event handlers, you might find useful the chosen answer to http://stackoverflow.com/questions/5983779/catch-exception-that-is-thrown-in-different-thread Especially the .Net 4, Task / AggregateException example. – ToolmakerSteve Jul 11 '13 at 00:34
7

The only two types of exceptions that should come out of events are serious, potentially process-ending ones like System.OutOfMemoryException or System.DllNotFoundException, and things that are clearly programming errors, like System.StackOverflowException or System.InvalidCastException. Catching and dropping these kinds of exceptions is never a good idea -- let them float up to the top and let the developer decide what to do with them on an application level.

As for the rest... any common or garden-variety exception like System.IO.IOException should be handled inside your event, and you should have some mechanism for returning such error conditions to the caller.

Warren Rumak
  • 3,824
  • 22
  • 30
  • Oo, that just got me thinking.. what about `ThreadAbortException`? – Flynn1179 Jun 24 '10 at 23:36
  • `ThreadAbortException` is an interesting one because it doesn't matter if you catch it or not -- it'll just get rethrown at the end of a catch block. – Warren Rumak Jun 25 '10 at 00:15
  • -1: Exceptions should be only handled inside the event (or elsewhere) if you know what to do with them. And in the general case (e.g. inside a handler for one of the events in the Framework - Page.Load, Button.Click etc) there is no mechanism for returning error conditions to the caller. – Joe Jun 25 '10 at 05:37
  • Ummmm, Joe? You, the app developer, aren't typically the caller of Page.Load or Button.Click so what difference does it make if you can pass back error conditions in those cases? I was talking about cases where you design your own events. And in either case, it's still as I said: Handle what you can handle, and let truly exceptional and programmer-error failures head up the stack. – Warren Rumak Jun 25 '10 at 06:09
  • @Warren, Exceptions are the usual mechanism in .NET for returning errors to a caller. This is as true for event handlers as for any other code. Of course you could design your custom events so that an error code can be returned in the EventArgs, but it's an unusual design and can get complex it you want to allow multiple event handlers to return different codes. And the principle of least surprise says you should follow design patterns already established in the .NET Framework. – Joe Jun 25 '10 at 09:38
  • 2
    @Joe: In many cases, events represent an inversion of control, where the entity raising events does so purely for the benefit of the event recipients, and doesn't really care if the recipients do anything or not. What would be helpful would be a means of invoking the delegates in a multi-cast delegate in such a fashion that if one or more of them throw an exception, all delegates will be called, but an EventException will be thrown with an Exceptions property yielding an IEnumerable, Exception>>. Too bad there's no standard implementation for such a thing. – supercat Mar 22 '11 at 20:51
  • +1 My experience matches what Warren and @supercat are saying. I find Joe's position to be extreme. I agree with Joe that swallowing ALL exceptions is NOT A GOOD THING. However, this notion that allowing exceptions from handlers is no different than from other code, is misguided. For extreme exceptions, from which continuation is meaningless, yes. But .Net uses the Exception mechanism much more widely. A handler that didn't swallow any exceptions at all, would be a disservice to the users of the containing library. – ToolmakerSteve Jul 10 '13 at 23:10
  • Modifying my view on this. Consider http://www.codeproject.com/Articles/36760/C-events-fundamentals-and-exception-handling-in-mu#exceptions this shows a Publisher that iterates thru the handlers attached to an event, with try/catch around each handler call. In this design, it makes sense for handlers to simply pass on any exceptions they don't know what to do with. – ToolmakerSteve Jul 10 '13 at 23:24
  • Joe does make a good point regarding the Framework events that lack a way to return error conditions to the caller. The (archaic) design of those events indeed may force a handler to simply throw up its hands, and let the exception throw. If appropriate, try to design an "out-of-band" mechanism when using these, so you can specify who to hand the exception to. http://stackoverflow.com/questions/17573467/exception-handling-in-delegate/17573857 – ToolmakerSteve Jul 11 '13 at 00:44
  • 1
    @ToolmakerSteve: Since I wrote the above, I've come to realize that the one of the biggest problems with exceptions is that the question of whether code should *react* to an exception is largely orthogonal to whether it will *resolve* the exceptional condition. "Finally" blocks should be more common than "Catch-and-resolve", but "react-without-resolving" should be common as well. If an exception in a block of code could leave an object in a bad state, a "react-without-resolving" block should be used to invalidate the object. – supercat Jul 11 '13 at 02:23
  • 1
    @ToolmakerSteve: The primary argument against "Pokemon" exception handling is that unexpected exceptions may have left objects in an unknown state. I would posit that if an unexpected exception during an operation could corrupt an object, the operation should be wrapped in code which ensures the object will be in an expressly-invalidated (and thus not unknown) state. If the object is critically needed, the fact that it's been invalidated will ensure fail-fast behavior. If the object isn't needed, the fact that it's corrupt shouldn't matter. – supercat Jul 11 '13 at 02:26
  • "and you should have some mechanism for returning such error conditions to the caller." - there is not necessarily any way to access a caller - we're in an event after all. This is the difficulty. – PandaWood Oct 02 '15 at 02:18
3

Some of the answers here suggest it's bad to throw from an event handler ("creates havoc for your caller", "tends to lead to very difficult to handle situations, and unexpected behavior",...).

IMHO this is nonsense.

In the general case, it's perfectly OK to throw from an event handler. Other event handlers won't run of course - neither will the event handler that throws run to the end, nor any other code between the firing of the event and the point where it's caught. So what? It's perfectly normal that code is not executed when an exception is thrown - if you need to guarantee it's executed, then use a finally block.

Of course in any specific case you may want to consider which, if any, exceptions it is appropriate to handle, just as you would with any other code.

As always, there are no hard and fast rules that apply in all circumstances. One of the answers here says "event handlers should be fast ... and close to error free...". A counterexample is the ASP.NET Page.Load event.

A general rule in .NET is that it's almost always a bad idea to swallow all exceptions: this applies to event handlers just as it does to any other code.

So the answer to the original question "are there ever any circumstances in which it's acceptable for a method responsible for listening to an event to throw an exception" is very definitely yes.

Just as the answer to the question "are there ever any circumstances in which it's acceptable for a method responsible for listening to an event to swallow exceptions" is also yes.

Joe
  • 122,218
  • 32
  • 205
  • 338
  • I think calling it 'nonsense' is a bit unfair- certainly for events handled internally I'd probably consider throwing an exception if required, but if I'm writing a handler for a method on someone elses code, I wouldn't have thought it's appropriate to throw (or even allow) exceptions. – Flynn1179 Jun 25 '10 at 08:11
  • 1
    What I'm calling 'nonsense' are the statements "creates havoc...", "very difficult to handle...", "unexpected behavior" which IMHO is just FUD. I'm not saying it's nonsense to catch or avoid throwing exceptions in an event handler some specific case. You should certainly consider what will happen when an exception is thrown from an event handler when designing your app. – Joe Jun 25 '10 at 09:04
  • If something unrecoverable has happened, then it makes sense for the event handler to throw an exception (or allow one to progress from a downstream call). It should just be understood that the class calling the handler through the event will (or at least should) allow the exception to propagate and ultimately kill the entire process or AppDomain, since it doesn't have sufficient context to know how to handle the exception. The other handlers shouldn't be called, since the AppDomain state may no longer be valid, so the question of whether it's breaking a contract is moot. – Dan Bryant Jun 25 '10 at 16:46
  • In my experience, most exceptions that occur in an event should be wrapped inside an error response, e.g. .Net 4's Task / AggregateException (or an adhoc equivalent in earlier versions). Only severe exceptions should be thrown -- with the expectation that this will be caught at a high level, killing whatever was being done. When possible, have the handler inform the handlee "I cant complete my task, and here is why". The reason this is different than non-handler code calls, is that non-handler code has a simple hierarchical, synchronous, control flow. Trivial to put the catch where desired. – ToolmakerSteve Jul 10 '13 at 22:59
  • See http://stackoverflow.com/questions/5983779/catch-exception-that-is-thrown-in-different-thread for a related discussion. And code example for AggregateException. – ToolmakerSteve Jul 10 '13 at 23:01
  • @ToolmakerSteve, I think Task is a specific case, providing a pattern to solve the problem of passing exceptions between threads. For a handler to say "I can't complete my task, and here is why" rather than throwing requires the EventArgs to support the pattern, which isn't generally the case. – Joe Jul 11 '13 at 07:07
  • @Joe Yes, I agree. It is only for threads, not for events. I mentioned it on the off-chance that someone reading might have a lightbulb go off in their head, and consider whether there might be an entirely different design approach to what they are trying to do. Possibly off-purpose here. Thanks for clarifying! – ToolmakerSteve Jul 11 '13 at 07:57
  • Throwing exceptions to signal un-recoverable situations is **always preferable** that using _error-flags_ and/or, even worse(!), _globals_. I cannot find a reason why should an event-handler be exempted from the above rule? I have only seen bad code from programmers not respecting that, so please do not preach bad habits. It just useful to always describe the behavior of the event-source in the case of exceptions. – ankostis Jun 10 '14 at 16:15
1

In an ideal world, event handlers shouldn't raise exceptions. Raising an exception in an event handler tends to lead to very difficult to handle situations, and unexpected behavior. As you mentioned- this blocks subsequent event handlers from seeing the event, and the exception propagates into the event producer's code since it raised the event.

Ideally, event handlers should be fast (if they're long running, they should try to schedule the work in a separate thread) and as close to error free as possible.

Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
  • "Ideally, event handlers should be fast" - what about Page.Load in an ASP.NET app? There are no general rules here. Propagating an exception from an event handler is perfectly OK, and generally leads to the easy-to-handle situation of an exception propagating to the top level handler. – Joe Jun 25 '10 at 05:39
0

In an ideal word, exceptions should not exist. In a developer real word, exceptions are always expected and should be intercepted (caught), propagated or inhibited as situation asks.

So

are there ever any circumstances in which it's acceptable for a method responsible for listening to an event to throw an exception

Yes. You can expect an exception from every method, responsible or not to an event.

To catch almost every exception from a Windows application use:
AppDomain.CurrentDomain.UnhandledException
Application.ThreadException

serhio
  • 28,010
  • 62
  • 221
  • 374