I’ll have to relent on my previous post, exceptions is not a dead topic after all. I’ve recently encountered some mis-uses and nuances that warrant discussion. Consider the exception handling construct:
reserveResources();
try {
performLogic();
} finally {
releaseResources();
}
It’s a classic that is particularly common when performing JDBC operations, the resources in this case being PreparedStatements, Connections and Transactions. Granted, the catch block can play an important role, but let’s ignore that for now.
Consider this scenario; logic failure resulting in an exception, then resource release failure. What exception is propagated – logic failure or the more incidental releasing of resources? Mr.Finally will always win that battle unless you declare otherwise.
It’s staggering the amount of times I’ve seen this oversight in practice. This approach is, of course, very acceptable if the finally exception has greater importance, but that is a rare situation. Too often, especially with non-TDD approaches, developers place exception handling in the back of there mind, exception flows are usually not considered until normal flows are codified. Staggeringly, in some cases I’ve seen code realized in this order as well – code the normal stuff, then add exception handling later. Resist this urge! Exceptions can and should be considered more fundamental aspects of an objects behavior.
So what’s the best approach. Let’s see….
reserverResources();
Throwable originalException = null;
try {
performLogic();
} catch (Throwable throwable) {
originalException = throwable;
} finally {
try {
releaseResources();
} catch (Throwable throwable) {
if (originalException == null) {
originalException = throwable;
} else {
log.error(“Exception releasing resources.”, throwable);
}
}
}
if (originalException != null) {
throw originalException;
}
Or perhaps this:
reserveResources();
try {
performLogic();
} finally {
try {
releaseResources();
} catch (Throwable throwable) {
log.error(“Exception releasing resources.”, throwable);
}
}
In each case the logic exception is preserved. Before I explore the pro’s and con’s of each approach, lets revise some best practices:
- Process exceptions once and only once. Processing typically involves logging and presenting a message to the user.
- Minimize generic catch blocks, such as catch (Exception exc) or catch (Throwable throwable), reducing the probability of gobbling exceptions that would otherwise be propagated.
- Treat exceptions consistently to facilitate maintainability, just like any cross-cutting concern. At its best consistency is achieved through centralized logic, so centralize exception handling - a simple example is web application error handling via the web.xml element declarations. I’ve seen plenty of violations of this rule, logging exceptions at multiple points – the classic violation being logging exceptions at their origin, promoting code scattering.
- Propagate at most one checked exception (see my previous blog on this principle). The exception should have meaning in the context of the method – akin to Domain Driven Design Intention-Revealing Interface principles.
With these practices in mind let’s consider the revised approaches:
Approach One
- Multiple violations of minimizing generic catch blocks.
- Code is relatively verbose – a minor point, but a point nonetheless.
- The exceptions are not re-thrown from a line indicating where the exception occurred (they are now both thrown from the lower if block), so the stack trace will slightly less communicative.
Approach Two
- Violation of minimizing generic catch blocks.
- The exception in the finally block is never propagated. Clients to this code will not be aware of any resource release failures, potentially losing critical information influencing subsequent processing or presentation to the user.
The first approach certainly seems the better of the two evils, but we’ve violated the generic catch block principle in each case. Mr. Checkstyle will explode with the appropriate settings.
IllegalCatch is not legal in all circumstances.
Let’s consider this check in more detail, it minimizes the chance of inadvertent exception consumption. It’s a safeguard for silliness. It is by no means a safeguard for something there is no good reason for doing, like not declaring a class with purely static behaviour as final.
It’s more compelling to violate this principle when developing infrastructure, but it holds as a hard and fast rule for standard application logic. In light of this I’ve changed my stance on this check:
Prevent inadvertent exception consumption in application logic using Checkstyle. For infrastructure development, rely on the pair or a review to prevent inappropriate generic catch blocks. This could be better realized by limiting the applicability of this check to certain packages – unfortunately Checkstyle’s IllegalCatch check does not support this.
I’ve seen a work around to this problem (aka hack) by employing a control flag approach:
int success = 0;
try {
doSomething();
success = 1;
} finally {
if (success == 0) {
// Exception occurred
}
}
Clearly this code is not as communicative and ventures to the days of languages void of exception constructs. Still, it’s another evil that you need to weigh into the equation determining which approach is best, but it’s an option that wouldn’t be considered if IllegalCatch was more flexible.
