| Date: | March 17, 2005 / year-entry #68 |
| Tags: | code |
| Orig Link: | https://blogs.msdn.microsoft.com/oldnewthing/20050317-00/?p=36143 |
| Comments: | 26 |
| Summary: | 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... |
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
So why didn't Because your exception handler itself encountered an exception.
The exception handler is not active during its own
(That the exception handler is not active during its own
In this case, the exception was caught by some outer caller,
causing the remainder of the first (This bug also exists in the proposed alternative to error-checking code posted by an anonymous commenter.) |
Comments (26)
Comments are closed. |
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.
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).
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"
So basically if you do something silly with exceptions you can suffer the same problem as you suffer by default with return values.
OK.
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).
"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).
that’s where in C# you would use "using", of course this is only true for managed objects
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.)
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.
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();
}
}
"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.
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.
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:
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.
This should work just fine:
try{
try{
try{
try{
document.DestroyAll();
finally{document.Close();}}
finally{document.DestroyExtensions();}}
finally{document.DestroyPlugins();}}
finally{document.Destroy();}}
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.
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.
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).
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."
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?
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 :-).
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?
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.
"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.
Richard: the destructor is called in the normal way, since _try{} already ate the exception…
PingBack from http://www.arcavia.com/kyle/blog/?p=191