Error handling and exceptionsYes, this is yet another post in the internet talking about using exceptions versus error returns. The topic has been flaming up at my workplace for quite some time now, and I felt that writing a blog post about it during the week-end would help me focus my thoughts and give me time to explain my point with the due care. In case you didn't know, I'm against using exceptions for error handling (maybe having spent many years working with Qt has had an effect on this); that does not mean that I never write code using exceptions: I certainly to my good share of
try
...
catch
when dealing with third-party code (including STL), but you won't find a
throw
in my programs.
I'm not going to write here
all the reasons why I refrain myself from implementing error handling using exceptions; I'd rather like to focus on the one I consider to be the major one, and which I rarely see given the due weight in the debate.
Code safety
I was about to title this “Code readability”, but this is more about code
verifiability, that is making sure that the code is correct and, ultimately, safe. As we all know, code is written once but read many times, and even if it's code you've written yourself, chances are that in a few weeks time you'll have forgotten several details about it; error cases and error handling are one typical thing that doesn't stick in our memory for long.
When I look at a small piece of code, such as the one that can fit into my screen, or which I can read from a merge request diff, I want to be able to ascertain that the code I'm looking at is correct. Let's look at some examples.
A throw
-free project
| assert(track != nullptr); |
|
|
| Car car; |
| car.setMaximumSpeed(90); |
| car.setName("Herbie"); |
|
|
| if (!car.executeLap(track)) { |
| log("Car failed to complete track"); |
| return false; |
| } |
|
|
| Path *path = car.getPath(); |
| if (!path) { |
| log("GPX path could not be retrieved"); |
| return false; |
| } |
|
|
| double temperature = car.engineTemperature(); |
| double boundingRectArea = path->boundingRectArea(); |
I just made this up, so please bear with me if it doesn't make any sense. What I want to show is that code like the above has very few fault risks,
if found in a project which bans returning errors as exceptions: if we exclude out-of-memory errors, that are generally handled by letting the application crash (though you can always catch it if you like), the reader can easily verify that this code is safe. Coding style policies and naming conventions can guarantee that
setMaximumSpeed()
and
setName()
won't have a return value that needs to be checked, and all other method calls either return an error that our code is properly handling, or return some value. Of course, by just looking at this piece of code we cannot know if the
engineTemperature()
method has some other overloaded sibling which accepts passing a reference to a boolean and which could be used to detect an error; so, it may be that our code could be improved in that respect, if we had a look at the header files for the
Car
class — but this does deny the fact that a simple glance at this snippet tells us exactly what errors are handled and what could be going wrong.
Let's look at this code instead:
| assert(track != nullptr); |
|
|
| Car car; |
| car.setMaximumSpeed(90); |
| car.setName("Herbie"); |
|
|
| car.executeLap(track); |
| Path *path = car.getPath(); |
|
|
| double temperature = car.engineTemperature(); |
| double boundingRectArea = path->boundingRectArea(); |
If we continue on the assumption that we are working on a project which bans throwing exceptions, we can immediately say that this code is not safe: we don't know if the car successfully executed a lap on the track, and out process will crash if
boundingRectArea()
is invoked on a null object.
Enter the exception
In a project where exceptions are actively used, the code from the second snippet is not obviously wrong anymore: maybe
executeLap()
cannot throw any exceptions, or, if does, the caller of this snippet is catching the exception? In order to figure out whether this code is correct, I need to see the declaration of the
executeLap()
method, and hope that there's a nice
noexcept
in there; if there isn't, I have to look at its implementation, and recursively descend through all the methods it calls — at which point the safest attitude is just to assume that it can throw. But that's only half of the story, because once I accept the fact that
executeLap()
can throw, I need to check whether the exception is properly handled: I have to check the implementation of all the callers of my method, and if I don't find a
catch
there, I'll have to recursively walk up the tree of their callers.
And indeed even the first snippet, which looked so harmless when exception throwing was banned, suddenly becomes not obviously correct anymore: what if
executeLap()
or
getPath()
also throw an exception? You might say that it would be quite a silly thing to do, and I'd certainly agree; but it may be that indeed they don't throw any exceptions in their implementation, but some of the methods they call does.
A compromise: catch early, catch often
The obvious solution to the above issue is having a policy of handling exceptions right away, and explicitly rethrowing them (or even better, rethrow a different, more appropriate exception) up the stack:
| assert(track != nullptr); |
|
|
| Car car; |
| car.setMaximumSpeed(90); |
| car.setName("Herbie"); |
|
|
| try { |
| car.executeLap(track); |
| Path *path = car.getPath(); |
|
|
| double temperature = car.engineTemperature(); |
| double boundingRectArea = path->boundingRectArea(); |
| } catch (std::runtime_error &e) { |
| log("Car failed to complete track"); |
| throw; |
| } |
What I can tell from the above snippet is that the code is handling errors, and this is somehow a relief. I'm sure some of you would suggest using a more specific catch clause, but for the sake of this example let's assume that this one is fine.
(Quick note: the above example does not catch
std::exception
, because that would also catch the
std::bad_alloc
exception which is typically thrown in out-of-memory situations; my advice is not handle it at all, unless you know what you are doing)
In real life, though, you might find that trying on a rather large block of operations is not enough: suppose that the Car methods all emit the same exception type, and that you need to handle them differently depending on
when they occur. Then you'd need to split up the
try
into smaller blocks, and at that point your code won't look any cleaner than the equivalent code using
if
on return values. Of course if you own the Car class you could modify it to throw different exceptions, in order to keep the more operations inside the
try
block and have more specific catches at the end.
The big catch (pun intended)
Even once you've refactored your methods to get the best out of exceptions (where "best" is highly subjective, but let's assume that it just means that you are happy with your exception-throwing code), there's something that still bothers me, and that's exactly the same thing that proponents of exceptions use as a “pro” in their argumentations: the business logic of your code gets separated from the error handling. You get a nice block of pure logic, not cluttered with error checking, and a catch section (which I call “the big catch”) where error cases are handled.
I really don't see how that makes the code any more readable or safe: sure, the logic is not intertwined with error handling and might help focus on the expected flow of the operations (though, really, I do not think that normal brains have a problem skipping over
if
blocks), but that's hardly what I'm interested in when I want to check that the code is correct. Most of program errors and bugs lie in handling the edge cases and the abnormal situations, the seldomly taken code path, and that's where I focus my attention.
| try { |
| operationA(); |
| if (value > B.maxValue()) { |
| operationB(); |
| } else { |
| operationC(); |
| } |
| operationD(); |
| } catch (ExceptionI &e) { |
| ... |
| } catch (ExceptionII &e) { |
| ... |
| } catch (ExceptionIII &e) { |
| ... |
| } catch (std::runtime_error &e) { |
| ... |
| } |
When I see code like this one, I need to mentally build a mapping of “
operationX()
→ possible exceptions” (which, unless exception naming is making this obvious, requires me to look at the implementation of the
operationX()
functions), and then mentally reconstruct the possible code paths in case
operationX()
fails, for each line of the
try
block.
Not seeing the errors
right there, right away makes the correctness verification
harder, which in turns means that the code becomes less safe. It will make you focus on the best case scenario, while ignoring all those annoying edge cases — too bad that 90% of the bugs are there.
Reading through the ISO C++ propaganda FAQ
I've been given a link to the
C++ FAQ about exceptions, and unfortunately I read it. While there isn't much to argue on the technical side of it, it also carries some misleading statements, which might be true in absolute terms but don't let you see the big picture by not mentioning all that you need to know (which is the fundamental technique behind propaganda). An example is when they mention that eliminating ifs makes for more robust code, without mentioning that the same applies to all code branches, including exceptions.
Another argument that bothered me when I read it is about error propagation; this is the example they make:
| void f1() |
| { |
| try { |
| // ... |
| f2(); |
| // ... |
| } catch (some_exception& e) { |
| // ...code that handles the error... |
| } |
| } |
| void f2() { ...; f3(); ...; } |
| void f3() { ...; f4(); ...; } |
| void f4() { ...; f5(); ...; } |
| void f5() { ...; f6(); ...; } |
| void f6() { ...; f7(); ...; } |
| void f7() { ...; f8(); ...; } |
| void f8() { ...; f9(); ...; } |
| void f9() { ...; f10(); ...; } |
| void f10() |
| { |
| // ... |
| if ( /*...some error condition...*/ ) |
| throw some_exception(); |
| // ... |
| } |
The claim is that this code is more readable than the one with explicit error handling, because all the
f2()
,
f3
, …,
f9()
functions don't have to handle the error occurring in
f10()
. It is indeed a convincing argument, when presented in these terms, but is this really how code looks like? In real life, you'll hardly have a chain on 1-liner functions, all residing next to each other in the same file. The moment that you realize that each one of these
f[i]n[/i]()
functions might be twenty or thirty lines long, and that they might be scattered over different files, and be called not just by
f[i]n-1[/i]()
but by any other function in the codebase, the picture does not look so rosy anymore: we get back to my main point of pain, that is that looking at the code of, say,
f5()
, I will not be able to tell if the errors thrown by it, or by any of the methods invoked by it, are properly handled.
Exceptions in APIs
I'm not really bothered when a library I need to use is throwing exceptions: having to write
| try { |
| Foo::fetch("http://example.com/resource.txt"); |
| } catch (Foo::Exception &) { |
| return false; |
| } |
is not less readable or less safe than the code I'd write if
Foo::fetch()
returned an error code. I still do have a little complaint, because the library author has given himself the right to decide that a failure in his library should be considered a critical fault, whereas it may be that in my program it is an expected failure and using exceptions imposes a penalty which could have been avoided. But I digress.
As long as the library documents which exceptions are thrown, is used by many people (which hopefully means that it has few bugs) and is a library that I don't need to contribute to, wrapping some of its methods in
try
blocks is something I can live with.
One situation where I actually wish that libraries threw an exception is in out-of-memory situations; in that case, of course, I'd expect them to throw nothing else than
std::bad_alloc
, which is the exception emitted by the standard library in such situations. That allows the caller to decide whether to ignore the exception and have the process terminated (which is what I usually do, at least in desktop applications) or try their luck and handle the failure — the latter is not easy, but it can certainly be done.
This is one case where error returns can be problematic, because it's likely that your code would look something like
| if (!Foo::open(fileName)) { // suppose that this returns Error::OutOfMemory |
| log("Failed to open " << fileName); |
| return false; |
| } |
and in this case there's actually a risk that
your code is going to trigger an out-of-memory error in logging the message; this shouldn't be a concern in most cases, but I can imagine some situations where one might want to know which was the exact operation that first incurred in the out-of-memory failure.
So, I'm actually fine with
new
throwing. As for my code, my
throw
statement is actually spelt as
return
.