Your exception handler can encounter an exception


Consider the following code, written in C# just for kicks; the problem is generic to any environment that supports exception handling.

void ObliterateDocument()
{
 try {
  try {
   document.DestroyAll();
  } finally {
   document.Close();
   document.DestroyExtensions();
   document.DestroyPlugins();
  }
 } finally {
  document.Destroy();
 }
}

Some time later, you find yourself facing an assertion failure from document.Destroy() claiming that you are destroying the document while there are still active plugins. But there is your call to document.DestroyPlugins(), and it's in a finally block, and the whole point of a finally block is that there is no way you can escape without executing it.

So why didn't document.DestroyPlugins() execute?

Because your exception handler itself encountered an exception.

The exception handler is not active during its own finally clause. As a result, if an exception is thrown during document.Close(), the exception handler search begins at the block outside the finally block.

(That the exception handler is not active during its own finally clause should be obvious. It would mean that if an exception were to occur during the finally clause, the program would go into an infinite loop. And it also wouldn't be possible to rethrow a caught exception; your throw would end up caught by yourself!)

In this case, the exception was caught by some outer caller, causing the remainder of the first finally block to be abandoned. The other finally blocks do run since they contain the one that died.

(This bug also exists in the proposed alternative to error-checking code posted by an anonymous commenter.)

Comments (26)
  1. Anonymous says:

    Putting stuff in the finally that can raise an exception gives me the willies. Putting anything in a finally AFTER a step that can raise an exception is right out.

  2. Anonymous says:

    In Java, if document.Close() could throw a checked exception, then the call to it would itself have to be wrapped in its own try/catch block (unless a throws clause for that exception was added to the ObliterateDocument method signature).

  3. Anonymous says:

    I’m more of a C++ programmer, so finally==destructors, and doing anything that can throw in a destructor is outlawed on my team. Its in our coding standard and everything. Presumably as C# coding standards become more mature similar things will start cropping up for finally blocks since they are more or less analagous.

    I don’t know what happens in C# but in C++ if an exception is thrown in (for comparison with the example) the finally block if that is being executed as a result of an exception the process terminates via std::terminate anyway, which (on vc7.1 at least) gives you (or your user) the wonderful error "Application X terminated in an unusual way"

  4. Anonymous says:

    So basically if you do something silly with exceptions you can suffer the same problem as you suffer by default with return values.

    OK.

  5. Anonymous says:

    DrPizza: No. A more accurate summary is: If you don’t wrap every statement that can possibly throw an exception with a try/catch, then you’ll be in a situation where it’s impossible to ensure that you properly cleanup.

    This is especially true if your statements have persistant side effects (like creating files).

  6. Anonymous says:

    "DrPizza: No. A more accurate summary is: If you don’t wrap every statement that can possibly throw an exception with a try/catch, then you’ll be in a situation where it’s impossible to ensure that you properly cleanup.

    This is especially true if your statements have persistant side effects (like creating files)."

    So how is this different from:

    If you don’t check every statement that can possibly return an error code with a if/else, then you’ll be in a situation where it’s impossible to ensure that you properly cleanup.

    This is especially true if your statements have persistant side effects (like creating files).

  7. Anonymous says:

    that’s where in C# you would use "using", of course this is only true for managed objects

  8. Anonymous says:

    I’m pointing this out because I’ve seen this bug in production code – just sharing a tip.

    (In an error-checking model, if you see "if failed goto skip-over-the-rest-of-cleanup" your brain might raise a red flag, "What about cleaning up the plugins?" I’m trying to point out that you need to raise that same flag when reading exception-based code.)

  9. Anonymous says:

    I think there is a another programming issue highlighted here, outside of the exception trap: If you intend to call a method that depends on some state held in the object, you better check the state before you call the method. This is a huge gotcha with the SqlConnection class that I am always cleaning up in other peoples code.

  10. Anonymous says:

    I’ve lived in holy terror of that for years now. Coming from the Delphi world this sort of thing can happen, just as in any language with structured error handling.

    This definatly over-kill for most cases, but this is how I write "some" of my error handlers now:

    void ObliterateDocument()

    {

    try {

    try {

    document.DestroyAll();

    } finally {

    try { document.Close(); } catch(Exception ex){LogError(ex);}

    try { document.DestroyExtensions(); } catch(Exception ex){LogError(ex);}

    try { document.DestroyPlugins(); } catch(Exception ex){LogError(ex); }

    }

    } finally {

    document.Destroy();

    }

    }

  11. Anonymous says:

    "This definatly over-kill for most cases, but this is how I write "some" of my error handlers now:"

    Is there some reason you failed to catch the actual example exception that Raymond pointed out in his article? I.e. document.Destroy.

  12. Anonymous says:

    When my exception handlers start becoming big monstrosities like crisB has shown, I take it as a sign that the function is trying to do too much and needs to be broken down into smaller and simpler components. In this case, the document.DestroyAll() call should do what it’s supposed to do and destroy the extensions and plug-ins itself. The document class should take care of its resources and not pass the buck to the callers.

  13. Anonymous says:

    chrisB … the problem with what you have written is that while you have logged the exception, you haven’t handled it, and those calling ObliterateDocument have no idea something is wrong. It maybe safe in this particular cause, but is not the best as a general solution.

    My recommendation is to have continual embedded try/catch/finally clauses and throw the caught exception:

  14. Anonymous says:

    Kristopher … given .NET’s exception model, you can have certain exceptions raised in otherwise benign code (like an assignment), but it will not preclude the need to make sure other items in your finally are called (if you want to be 100% safe).

    Exceptions, such as ThreadAbortException, can happen at any time, and if you need/want to protect your finally clauses, you need to do something similar to ChrisB.

  15. Anonymous says:

    This should work just fine:

    try{

    try{

    try{

    try{

    document.DestroyAll();

    finally{document.Close();}}

    finally{document.DestroyExtensions();}}

    finally{document.DestroyPlugins();}}

    finally{document.Destroy();}}

  16. Anonymous says:

    The problem is a generic one not specific to exceptions or .NET: what to do if an error happens during recovery from another error. There are exactly 3 options here: ignore, abort process, handle-and-retry. Your fabulous .Java requires one to manually code all 3 of them. A better language like C++ defaults to abort (std::terminate) and requires programmer to manually code the other two. The usual guideline "don’t throw from destructors" sort of forces him to do it. In .Java you could adopt the same rule: "don’t throw from Dispose/Close/whatever". But you are still left with the wrong default.

    You may also want to read this: http://groups-beta.google.com/group/comp.lang.c++.moderated/browse_frm/thread/67046b021cc4adb3/da59a2b51cf277fd#da59a2b51cf277fd for an interesting discussion of this same topic.

  17. Anonymous says:

    As soon as exceptions are broadly used, anything CAN raise an exception. I’m not using iostream but I’m affraid it also throws exceptions around. So, failed file open raises an exception, and failed close can also raise an exception and if you have two close statements in catch and first excepts, the second is not performed. Once everything throws exception, and you try to handle them correctly, you have *more* problems than with checking returns.

    On some places Stroustrup and other insiders mention that they extended language before they knew what the results of it would be. I consider them guilty not because they introduced something to the language, but because they didn’t yell loud enough that such language features shouldn’t be used all around.

    Moreover, the really usefull exceptions are the hardware generated floating point exceptions. And proper floating point is still something that most of the C++ fathers and stepfathers don’t consider enough.

  18. Anonymous says:

    What? You mean you still have to be a good programmer to use exceptions instead of error codes? *gasp!*

    No "foolproof error handling strategy" survives first contact with the general developer population (enemy).

  19. Anonymous says:

    Richard: The VC++ 7.1 help recommends that you don’t use SEH in C++ programs:

    "You can ensure that your code is more portable by using C++ exception handling. Also, C++ exception handling is more flexible, in that it can handle exceptions of any type."

  20. Anonymous says:

    So the cleanup code in document.Close() is failing, and this failure is not being correctly handled. Not too surprising, that. Having cleanup code that may fail is going to make your error handling code more complex, whether you are using exceptions or return values to propagate error information.

    But in general, cleanups should not fail, since they consist of freeing up resources that have already been successfully required. So I’m interested in what made document.Close() fail. What was this routine attempting?

  21. Anonymous says:

    I just hope that this was not Microsoft’s production code :-)). Here’s why:

    1. This looks like general cleanup, so, in .NET, one should use IDisposable.Dispose.

    2. Throwing from cleanup code is unnacteptable practice, as C++/Java/Delphi folks know. If one cannot control this, one should trycatch-wrap offending pieces.

    3. The function obvoiusly leaves the object in an inconsistent state; then, some other function uses the bad object and fails. Really!? My feeling is, the object should probably have been disposed off and entirely forgotten by the program after the problem, but…

    Regarding: "In an error-checking model, if you see "if failed goto skip-over-the-rest-of-cleanup" your brain might raise a red flag, "What about cleaning up the plugins?" I’m trying to point out that you need to raise that same flag when reading exception-based code"

    No. You needn’t raise this flag. BAD ADVICE! In exception-based code cleanup must not throw and must not skip anything. Let’s ask the people from .NET teams about it. I think there is no cleanup that throws in the framework. Or, if there is, they know it’s a bug :-).

  22. Anonymous says:

    BTW – in C++, using SEH, if I say

    {SomeObject obj;

    _try {DoSomething();

    }

    __finally

    {};

    }

    does obj.~SomeObject() get called if DoSomething() raises an exception?

    If not, how *do* I get it called?

  23. Anonymous says:

    Saying that the finally clause is not allowed to throw exceptions is naive. No programmer can reasonably expect to know what does and doesn’t throw an exception. The only thing you can do is catch all possible exceptions like chrisB showed.

    I blame the existence of the finally clause for this problem. Without finally{} there would be no problem.

    I have always viewed throw as a return, so I never use finally. I has always been odd to me to have a lanaguage construct that allows you to do work AFTER you have thrown an exception. It is like allowing a method to continue AFTER it has returned.

  24. Anonymous says:

    "No programmer can reasonably expect to know what does and doesn’t throw an exception."

    This is true. OTOH, I was trying to convey the destructor rule from C++ (i.e. throw is forbidden). From where we stand, finally is analogue to d-tors so the same rule should apply in order to preserve cleanup semantics. It seems great that there is no "finally" in C++, doesn’t it? :-))

    On the matter at hand: I think that in majority of cases, there will be no possibility of exceptions in cleanup code anyhow, so the whole point is moot. In rare cases when it’s are possible, it should be glaringly obvoius (it usually is, I think), try-catched around, well commented, whatever… If not, tough, needs to be fixed.

    The example from the article is bad in this sense: it appears that it is usual to have cleanup code that throws, but, OTOH, it’s not true. It is also known (at least to C++ folks) that it’s a general no-no. Hence my opinion from before.

  25. Anonymous says:

    Richard: the destructor is called in the normal way, since _try{} already ate the exception…

Comments are closed.