views:

334

answers:

5

Consider the following program. How is the behaviour that it displays (namely that exceptions will propagate out of an event handler) a "good thing"? As far as I can tell, it could only ever be bad; unexpected exceptions popping up from functions that they shouldn't. In my particular case, it was killing threads. So, is this behaviour actually a good thing in some cases? Is this a reason to say that letting exceptions out of event-handlers is poor design?

static class Program {
    static void Main()
    {
     Foo foo = new Foo();
     foo.SomeEvent += ThrowException;

     try 
     {
      foo.OnSomeEvent();
     } 
     catch (Exception) 
     {
      // This is printed out
      Console.WriteLine("Exception caught!");
     }
    }

    static void ThrowException(object sender, EventArgs e)
    {
     throw new Exception();
    }
}

// Define other methods and classes here
class Foo 
{
    public event EventHandler SomeEvent;

    public void OnSomeEvent()
    {
     SomeEvent(this, EventArgs.Empty);
    }
}
A: 

You need a contract with the event source on whether the event handler can throw exceptions. For example, if a COM object is an event source this is strictly prohibited - exceptions should never cross COM boundary.

sharptooth
Re: my last question: Should this contract always be that event handlers shouldn't throw exceptions (or allow uncaught exceptions) unless the API says you can throw AException, BException, etc?
Matthew Scharley
This contract can be whetever is convenient. If the event source is in .NET it's perfectly reasonable to expect that the handler can throw any .NET exceptions.
sharptooth
+6  A: 

What's your preferred alternative - silently swallowing the exception? I wouldn't like that at all.

Events are just a way of implementing the observer pattern, really. If a listener throws an exception, I'd absolutely expect that exception to be thrown back to the caller. Any other behaviour I can think of would effectively treat the exception as unimportant. The whole point of exceptions is that when something goes wrong, you find out about it quickly and implicitly. You have to explicitly handle exceptions, so that you don't go on your merry way in a corrupt state without realising it.

You make a valid point about whose responsibility it is to handle exceptions. Broadly speaking, I find it best to assume that just about anything can throw an exception at any time. Other than special cases where I know a specific exception may occur and I can handle it, I don't usually catch exceptions other than at the top level - except possibly to wrap and rethrow, or log and rethrow.

Now it's possible that one of your event handlers really shouldn't be throwing an exception - that they haven't really run into an error condition - but what should happen if it's a perfectly reasonable exception which indicates a serious problem? While a program crashing is ugly, it's often better than continuing with some of it malfunctioning, possibly corrupting persisted state etc.

Fundamentally, I don't think the CS/SE field has got error handling "right" yet. I'm not even sure that there is an elegant way of doing the right thing which is simple to express in all situations... but I hope that the current situation isn't as good as it gets.

Jon Skeet
+9000 last paragraph!
Anton Tykhyy
Here's an extension on this then: Is it ever alright to catch `Exception`? One particular case that comes to mind for my current project is processing user generated scripts. Presuming they compile at all, when I try to run them, I have a try/catch(Exception) block around calling the method they just created. Of course, if an error occurs, it's reported to the user so they can fix their script, but is crashing alltogether really the right thing?
Matthew Scharley
This is one of the very few proper uses of catch(Exception). Ideally you'd run the user script in a separate process, so it couldn't interfere with your program; if the script crashes, you just get an exit code like -1073741819 or whatever.
Anton Tykhyy
The script needs to interact with my program (via a plugin style system), so that's not an option, but a good idea also.
Matthew Scharley
It really depends on whether the program can continue with a broken plug-in. You might want to remove the plug-in, or try again, or abort depending on how important it is - and that may be something only the user can decide. For example, if it's an auditing plug-in, they may decide that they absolutely *must not* continue without auditing. If it's a "smiley generator" then it's probably okay to keep going. Note that some exceptions (e.g. stack overflow) *will* bring down the process. Thread aborts can be reset, but only if you're expecting them.
Jon Skeet
You might want to consider running the user script in a separate AppDomain - any exceptions thrown in that domain should probably trigger a domain unload.
Jon Skeet
+1  A: 

My personal prejudice is that not catching exceptions is generally a bad thing. The only "exception" to this rule for me is for simple applciations where the uncaught exception termianates the process, you see the error and fix it.

For multi-threaded apps, if the default behaviour of uncaught exceptions is to zap threads then it seems to me absurd not to catch exceptions. Hence event handlers should not just punt the exception up the stack and hope for the best.

Silently swallowing exceptions is usually bad too, something bad has happended it needs fixing. So perhaps log a message, and then return?

djna
Is it ever right to catch `Exception` then? ie. at the top level of a processing thread to keep it alive after a sanity check.
Matthew Scharley
I'd prefer to crash, restart and send a log. Besides, some exceptions you can't catch (OutOfMemory, StackOverflow and ThreadAbort).
Anton Tykhyy
Many more being added to that uncatchable list in CLR 4.
Daniel Earwicker
You should only catch `Exception` if you want an unexpected exception (indicating a bug) to cause open `finally` blocks to execute (perhaps throwing more exceptions or trashing more state, perhaps deleting temp. files, or the wrong files, due to the corrupted state...)
Daniel Earwicker
It is common in my world (java app servers) for several different applications, developed by different teams to be running in the *same* process. Hence terminating the whole process because one app couldn't handle an event is unpopular. Log and continue is preferred. Overflowing stacks and running out of memory is somewhat anti-social :-)
djna
In .NET you can create separate AppDomains in one process, specifically for that purpose.
Daniel Earwicker
AFAIK a separate AppDomain doesn't protect against the uncatchables, at least in CLR 2.
Anton Tykhyy
So "crash" (earlier comment) in one domain does not affect the other Domains? My rules would be: 1). Problems should not be silently absorbed - if you can count on your caller to log the problem then it's ok to ignore exceptions. If, as the question indicates, ignored exceptions are not logged, catch log and (maybe) rethrow. 2). Do no harm to other processing. If an uncaught expception would terminate an important thread or the whole process perhaps better to log and absorb. If .NET environment tolerates exceptions, then fine let 'em flow!
djna
A: 

An event is nothing more than syntactic sugar around a function call, so it makes sense that it would propagate up to the function that raised the event. What, otherwise, should the behaviour be? The exception has to go somewhere.

Adam Ruth
+1  A: 

The main aspect of exception handling discussed here is: do not catch exception if you don't know how to handle it. But let's talk about the observer pattern, where notifier emits event (or signal) about it's state change and listeners handle it. A good example of a notifier is a button emitting 'clicked' event. Does a button care about who are the listeners and what they do? Not really. If you're a listener and you got this event, then you've got a job to do. And if you can't do it, you have to handle this error or inform user, because passing exception to the button makes no sense - button definitely does not know how to handle errors of listener job. And buttons state changer (probably some message loop) does not either - your application will end up at Main() with a crash.

That's how observer pattern works - event emitters don't know what their listeners are doing and there's very little chance they will handle this exception properly.

Also bear in mind, that if your exception handler throws exception, there may be other listeners that won't receive notification and that may lead to undefined application state.

So my advice is to catch all exceptions at event handler, but figure out how to handle them there. Or else no one will.

Henrikas