Tuesday, July 10, 2007

Voting for Good Exception Handling

When getting ready for a conference I gave for Sun (Java 7 - A lot to be waiting for !), I came across this blog: Voting for Checked Exceptions. It was in response of Neal Gafter's blog: Removing Language Features?.
The list of comments is very long, and I never found the time to read them, until now. I really liked what went on there (except the personal attacks), and I want to try to summarize the arguments, as I view them. The arguments are also exposed in Neal's blog comments but there was a lot less arguments...
I'm not trying to be impartial, since for me it's clear: Checked Exceptions should go.
Page 49 of the presentation has the bullets that summarize my view on Checked Exceptions.

So here are the valid arguments I saw in the blog comments:

Checked Exceptions avoid the digging of what can go wrong

Description:

In other languages that does not have checked exceptions, it takes a good amount of time, thinking, QA cycles, and production crashes before you know which exception types can be thrown by a specific method.
Basically, since the API does not declare what it can throws, as a caller you need to "guess" what can happen or catch everything and try to re-throw what should not have been catch.

Validity:

The point is totally valid, but for me it does not make the balance tip on either side of the checked/unchecked issue.
Good API should declare a good throws clause list of what can happen. This is good design and OO practice. And a caller that knows what to do in case of an exception should catch them and handle them correctly. This is valid for Exceptions in general, checked or unchecked. The issue is that in Java nobody declares unchecked exceptions in the throws clause, it looks stupid. I don't know why, I think it's just a stupid habit.

Still there is a point against checked exceptions here. As an API writer, you are "forcing" the caller to catch ALL of the "checked" exceptions declared in your throws clause. This simple fact, breaks OO, forces some design concept on the caller (where to put the exception handler and method throws clauses), and generate all the "bloat".


Checked Exceptions helps you remember that you need to handle them

Description:
During coding you encounter methods throwing exceptions and so you need to handle them at some point. So, with checked exceptions, for a certain class or module, the list of exceptions to handle is well define.
This point was made especially when some lazy developers will ignore unchecked exceptions all together. If the compiler will not bother, exceptions are ignore.

Validity:
Here the point fails on multiple accounts:
1) Forcing the catching of an exception on a lazy developer is worse than let him go along. You end up with things like:
try {
o = getObject(id);
}
catch (PersistenceException e) {
// Should not happen I just saved it before
}
Which is my personal nightmare on many projects. When the code is full of these it's totally impossible to debug, understand the behavior, or move forward. It's desperate. Simply put:
"Empty catch block are worse than no catch at all"

2) Listing and catching all checked exceptions does not cover your needs in exception handling. Even in Java there are unchecked exceptions, and in today's framework you have more and more of them. So exceptions will slip through anyway.

3) Putting the error handling in the middle of the logical code is a very bad practice. You end up copy/pasting catch block all over, and if you need a small change in error handling, well... forget it.
The error handling is a layer of your module/component/architecture, that should be transparent and decoupled from your code.
This is the only way to make sure you are handling correctly ALL exceptions. This is the power and design in modern framework using AOP Interceptors. So, like in Spring, JBossMC, EJB3, Hibernate, you need a framework layer to catch Throwable and find out which exception handler should take care of what at which layer. Perfect, unbeatable, robust, user friendly, HA, and more.


You don't have to handle checked exception just pass them on

Description:
If inside a method you are calling a method with checked exceptions and you don't know how to handle it, just add it to your (big) list of throws clause, someone up the call stack will take care of it.

Validity:
This is the most non valid argument for checked exceptions, and the main reason why Java should have only unchecked exceptions.
I don't know many projects where any developer can just add a checked exception to the signature without blinking (the same for removing one). Checked Exceptions makes API throws clause fixed for eternity. If you add, you suffer, if you remove, you're lynched...
So, the only way is to create non meaningful generic Super Exception class (like IOException) that every methods throws.
Basically, this argument contradict the 2 previous ones which are a lot more valid.
On the other hand this feature is mandatory for normal coding. Everybody agree:
"If you don't know what to do with an exception, don't handle it"

This is critical, for "robust" application, so you need to find a way to answer this argument. Here is the proof that checked exceptions are contradicting their own benefits.


Checked Exceptions are recoverable, Unchecked are bugs

Description:
The principle of separation between checked/unchecked is well defined by ELH:
"as in section 8 of Effective Java. [...] checked exceptions are for unpredictable environmental conditions such as I/O errors and and XML well-formedness violations while unchecked exceptions are for program failures that should be caught during testing, such as array index out of bounds or null pointers."

Validity:
Classifying, and understanding what the error IS, is a critical step in good error handling. You need to send a good type of exception, for the good type of error, with the maximum amount of information (which file, to read, to write, which DB, which XML tag, ...).
Forcing yourself to create meaningful Exception is a difficult step, but one that always pays off. Add all the parameters you can, create meaningful messages, and use the good exception class.
But, the separation "unpredictable environmental conditions" vs. "program failures" is highly subjective, and always false once you climb one layer in your architecture. The kind of comment "Should not happen I just saved it before" proves my point.
Typing is good (IOException, NPE), but to enforce the global decision for all Java software ever written that IOException is an "unpredictable environmental conditions" does not make any sense.
Furthermore, the "forcing" of handling of "unpredictable environmental conditions" generates a lot of "development by exceptions". Basically, if you know that the file may not be there, I really prefer if the developer just create a File() object and test file.exists() instead of waiting for IOException.

Experiences with Checked Exceptions

1) In my experience to handle correctly an exception you need a lot of infrastructure information: How to display errors to the user (Web or Fat client), How to log, How to translate (code, language), How to trap, Severity detector, transaction access, ...
And to provide all this information to the inner logical code that need to wrap checked exceptions is a big burden for the application. Just pass the exception, my exception handler will handle it correctly.
Since, creating an exception handler with IOC and/or AOP that has access to all the above managers, is quite easy and clean.

2) We are doing lately a lot of migration to JPA, and more than 50% of our headaches are on checked exceptions. We found out that checked exceptions promotes indirectly the bad habit of development by exceptions:
try {
o = getObject(
id);
}
catch (PersistenceException e) {
// Object not found create it
o = create();
}
And changing the API signatures, removing FinderException from EJB 2.0, or the annoying CloneableException is a true nightmare.

3) Good coding with checked exceptions is sometimes very very tedious. For example the correct management of FileInputStream.close() and it's IOException is getting out of hand:
FileInputStream is = null;
try {
is =
new FileInputStream(fileName);

// Some work...
int n = is.read();

// Need to close here so caller will know file in unstable state
is.close();
// Need to set null to avoid double close
is = null;
}
catch (IOException e) {
throw new PersistenceException("Error reading file "+fileName, e);
}
finally {
if (is != null) {
try {
is.close();
}
catch (IOException ignore) {
// Well I try to close, but it does not close
// SO may be I should close ;-)
log.debug("Ignoring on close of ", ignore);
}
}
}

Conclusion
Independently, of the discussion on closures, I really hope that the compiler enforcement of catching checked exceptions will be removed in the openJDK very soon...

3 comments:

Hans said...

Wow, no wonder people find checked exceptions tedious/ugly when they see code like the last example. As as a CTO, you might have thought of:

try {
FileInputStream in = new FileInputStream(fileName);
try {
in.read();
}
finally {
in.close();
}
}
catch (IOException e) {
throw new PersistenceException(e);
}

Fewer lines, easer to read. If you really want separate handling of the FileInputStream.read errors and the others, then add a catch on the inner try.

Anonymous said...

The IOException on close can indeed happen: file has write-behind cache to network drive and network share goes away. On all other cases, it cannot.

If it does happen, the application is so likely to be dead that it might as well be thrown unchecked.

Anonymous said...

Good post.