Return-code vs. Exception handling

(Originally authored Feb 2008.  This is a revision of an older post from before I began blogging on the internet at large.  It’s been edited for style, not content)

This is one of those “religous wars” in programming language theory; return code handling (RCH) vs exception handling (EH).  Firstly, I am biased.  Secondly, I will try to remain objective as well, partitioning observations of code using these mechanism from how I feel this reflects on the utility of either mechanism.  I tend to _prefer_ EH in general, but not universally, and generally follow the viewpoint that “exceptions are for exceptional things”.  With that aside…

Raymond Chen wrote an article a while back on RCH vs EH[2]. There, he points out accurately some facts about code review of RCH and EH, concluding that it’s generally harder to conclude the failure-safety of some example the RCH code than some comparable EH code.

He’s right.  And he’s wrong.  His article is incomplete.  Lets start with defining the playing field.

Return Code Handling

Below, we have two versions of RCH-style code. All failures are communicated via return code. The left version is an erronous example, the failure paths have not yet been considered. The right version has corrected this fault.

BOOL ComputeChecksum(LPCTSTR pszFile, DWORD* pdwResult)
{
    HANDLE h = INVALID_HANDLE; 
    HANDLE hfm = INVALID_HANDLE; 
    void *pv;
    DWORD dwHeaderSum;

    h = CreateFile(...);

    hfm = CreateFileMapping(h, ...);

    pv = MapViewOfFile(hfm, ...);

    CheckSumMappedFile(... &dwHeaderSum, pdwResult);

    UnmapViewOfFile(pv);
    CloseHandle(hfm);
    CloseHandle(h);
    return TRUE;
}
BOOL ComputeChecksum(LPCTSTR pszFile, DWORD* pdwResult)
{
    HANDLE h = INVALID_HANDLE; 
    HANDLE hfm = INVALID_HANDLE; 
    void *pv = NULL; 
    DWORD dwHeaderSum;
    BOOL result = FALSE;

    if ( (h = CreateFile(...)) == INVALID_HANDLE)
        goto cleanup;
    if ( (hfm = CreateFileMapping(h, ...) 
          == INVALID_HANDLE ) )
        goto cleanup;
    if ( (pv = MapViewOfFile(hfm, ...)) == NULL )
        goto cleanup;

    CheckSumMappedFile(... &dwHeaderSum, pdwResult);
    result = TRUE;

    cleanup:
    if (pv) UnmapViewOfFile(pv);
    if (hfm != INVALID_HANDLE) CloseHandle(hfm);
    if (h != INVALID_HANDLE) CloseHandle(h);
    return result;
}   

The left side looks very clean, and very clearly does no RCH, so according to Chen’s method it is clearly incorrect. According to [2], the left one is ‘Really Easy’ to write, and ‘Really Easy’ to recognize as being incorrect. The lack of an if-statement every 2 lines is a red flag. Basically, you must explicitly write every possible success and failure code path, even the “uninteresting” ones.

Let’s move on to the EH example from Chen’s post.

Exception Handling (old style)

Bellow is adaptation of both the incorrect EH code from his article, along with a possible fix.

NotifyIcon* CreateNotifyIcon()
{
    NotifyIcon* icon; 

    icon = new NotifyIcon();

    icon.set_text("Blah blah blah");
    icon.set_icon(new Icon(...), GetInfo());
    icon.set_visible(true);

    return icon;
}
NotifyIcon* CreateNotifyIcon()
{
    NotifyIcon* icon = 0;

    icon = new NotifyIcon();
    try {
        icon.set_text("Blah blah blah");
        icon.set_visible(true);
        Info info = GetInfo();
        icon.set_icon(new Icon(...), info);
    } catch (...) {
        delete icon; throw;
    }
    return icon;
}

I totally agree with any and all critics that the above style of code is tough to diagnose. If any of { set_text, set_icon, set_visible, or get_info } can throw, the left code is totally busted. The code to the right is tricky… by moving GetInfo to a separate line, and set_icon to the end, the Icon object can’t be leaked (presumably set_icon is responsible for freeing the Icon in failure conditions). The try-catch will delete NotifyIcon and propogate the exception, but not without having made this program a pain to debug – we will see two exception raise events now instead of just one.

Even worse, tomorrow this code could become incorrect again – if the copy-constructor for Info can throw, the right side code is still wrong. I bet you didn’t think of that. Think you’re an expert at this? Try item #18 in Exceptional C++[3]. I think my first time I found about 16 code paths in the 3 lines of C++ code, barely teetering into ‘Guru’ category, but I spent like 15 minutes on it. Normally you have 5 seconds during a code review, and you won’t find all 23 code paths in 5 seconds. You have to do better than analyzing ever code path.

Applying Raymon Chen’s categories, getting to the correct code is “Really Hard”, and I hope the audience agrees with me. It is hard. Any line of code, any subexpression could throw, and we have to look at all those possible cases. RCH certainly looks better. Sure, we’d have to have like 9 ‘if’ statements to bring the above code in line, but at least then we could see that we covered all the code paths. But there is no general rule we can apply to see if it is trivially incorrect. In RCH, a line without a conditional branch is suspect. In EH (old style), a line without a try-catch after resource acquisition is suspect.

I call this ‘old style’ because it looks a lot like how you’d handle exceptions in C#, or in C++ without any library support back in 1990. Stephan T. Lavavej makes a distinction between “archaic C++” and “modern C++”[4]. And Raymond Chen notes in parting words, “Yes, there are programming models like RAII and transactions, but rarely do you see sample code that uses either.” – let’s remedy this situation.

Exception Handling (modern)

On the left, we have some bad RAII exception handling code, to parallel Chen’s original bad example. On the right, the ‘fixed’ version.

shared_ptr<NotifyIcon>
CreateNotifyIcon()
{
    NotifyIcon*
      icon( new NotifyIcon() );

    icon->set_text("Blah blah blah");

    icon->set_icon(
        shared_ptr<Icon>( new Icon(...) ),
        GetInfo());

    icon->set_visible(true);

    return icon;
}
shared_ptr<NotifyIcon>
CreateNotifyIcon()
{
    shared_ptr<NotifyIcon> 
        icon( new NotifyIcon() );

    icon->set_text("Blah blah blah");

    shared_ptr<Icon> 
        inner( new Icon(...) );
    icon->set_icon(inner, GetInfo());

    icon->set_visible(true);

    return icon;
}

I’m changing the rules. I don’t care how many branches there are, success or failure branches. What I do care about is that, at every point of time, there is exactly one owner for ever resource, and that in a general failure condition, that owner will take care of the resource.

That is the fundamental change. Modern C++ code using RAII does not generally have any explicit code for failure conditions. General failures are always handled non-locally. Local resource management is immediately handled by automatic objects, using RAII. Any raw resources (such as allocating a new object) must immediately be handed off to an RAII object.

Lets take a closer look at what I find wrong with the left example.

  • First line, we have a raw pointer. Bad. RAII rule: raw pointers == suspect code.
  • Second line, set_text. No raw resources. Good.
  • Third line, shared_ptr is mixed in with another call. Bad. Move to a separate line.
  • Last line, set_visible, also good.

How do we know we are done? When there are only two kinds of statements – RAII object initialization, and useful code that doesn’t create unadorned resources. Now, it is admittedly hard to get some RAII classes right in the first place, but that only needs to be done once per class, instead of once per use.

While it is still hard to see all the code paths, it becomes really easy to check if the developer has finished EH. Any resource allocation without RAII will tell you that the developer didn’t finish. Sure, bad RCH code pops out like a rose in a weed garden, but with RAII you can usually skip writing the bad code in the first place – just jump directly to shared_ptr<T>, or shared_factory<HANDLE, close_handle>::type, or scope_guard<T>, etc. You can skip writing the bad code at all, and do all your failure handling via RAII.

Final Thoughts

So, we have a new table for recognizing code:

Difference between good and bad RCH code · Is there an ‘if’ statement for every function call that could fail?

· Is there a cleanup block and goto?

· Is every resource mentioned in the cleanup block?

Difference between good and bad EH (old style) code · Which statements / sub-expressions can throw? Number of potentially throws == number of ‘if’ statements possible in RCH style code.

· Is there a catch-rethrow block for every resource that needs cleanup?

· Make sure there are no ‘goto’ statements – they break C++ destructors

Difference between good and bad EH (modern) code · Are native resources immediately stored in RAII objects?

· Is RAII initialization on a separate line.

· Suspect any usage of delete, or usages of array new (prefer vector to array-new).

And here’s how I believe modern EH with RAII fits in Raymond Chen’s table.

Recognition Task Easy Hard Really Hard
is RCH code bad X
diff between bad & not-bad X
is RCH code not-bad X
is EH (os) code bad X
diff between bad & not-bad X
is EH (os) code not-bad X
is EH (m) code bad X
diff between bad & not-bad X
is EH (m) code not-bad X

Finally, I’ll leave you with my interpretation of RCH vs. EH (modern).

Correct RCH-style Correct-EH(modern)-style
HRESULT
CreateNotifyIcon(NotifyIcon** ppResult)
{
    NotifyIcon*      icon = 0;
    Icon*            inner = 0;
    const wchar_t *  tmp1 = 0;
    HRESULT          hr = S_OK;

    if ( SUCCEEDED(hr) ) {
        icon = new (nothrow) NotifyIcon();
        if ( !icon ) hr = E_OUTOFMEM;
    }

    if ( SUCCEEDED(hr) )
        hr = icon->set_text("Blah blah blah");

    if ( SUCCEEDED(hr) ) {
        inner = new (nothrow) Icon(...);
        if ( !inner )
            hr = E_OUTOFMEM;
        else {
            Info info;
            hr = GetInfo( &info );

            if ( SUCCEEDED(hr) )
                hr = icon->set_icon(inner, info);
            if ( SUCCEEDED(hr) )
                inner = NULL;
        }
    }
    if ( SUCCEEDED(hr) )
        hr = icon->set_visible(true);

    if ( SUCCEEDED(hr) ) {
        *ppResult = icon;
        icon = NULL;
    } else {
        *ppResult = NULL;
    }

    cleanup:
    if ( inner ) delete inner;
    if ( icon ) delete icon;
    return hr; 
}
shared_ptr<NotifyIcon>
CreateNotifyIcon()
{
    shared_ptr<NotifyIcon> icon;//[5]
    shared_ptr<Icon>       inner;

    icon.reset( new NotifyIcon() );
    icon->set_text("Blah blah blah");

    inner.reset( new Icon(...) );
    icon->set_icon(inner, GetInfo());
    icon->set_visible(true);

    return icon;
}

Footnotes:

1 – C++ FAQ Lite, Section 17 “Exceptions and error handling” http://www.parashift.com/c++-faq-lite/exceptions.html

2 – Raymond Chen, “Cleaner, more elegant, and harder to recognize” http://blogs.msdn.com/oldnewthing/archive/2005/01/14/352949.aspx

3 – Herb Sutter, “Exceptional C++”, http://www.amazon.com/Exceptional-C%2B%2B-Engineering-Programming-Depth/dp/0201615622/

4 – Stephan T. Lavavej, aka STL, “Modern C++” http://nuwen.net/14882.html

5 – some may note that auto_ptr (or unique_ptr in C++0x) may provide better efficiency, and safely auto-converts to shared_ptr. That is another alternative.  Just don’t use raw ptrs and make sure to review Effective C++ on auto_ptr safety.

Join the Conversation

5 Comments

  1. While it’s true that RAII helps immensely, note that (1) you still have the “must set visible last” issue, and (2) you moved the goalposts: The original code was in C#, which does not support shared_ptr nor RAII. (In a GC language, all objects conceptually have infinite lifetime.)

  2. Thanks for the feedback.

    Regarding #2, Yes, I moved the goalposts intentionally. To cheat. ^_^ You can see this in…

    Regarding #1, should set_icon fail (throw), the NotifyIcon will be destroyed before the exception leaves this function. So the ‘set visible last’ issue is improved.

  3. I do not understand the “set visible last” problem. You need set visible last in any type of error handling, dont you? None of these handling styles will help make this issue more explicit.

    What kind of help can that knowledge provide if i know that the developer of the code frament tries to implement error handling? I dont get it.

  4. “Even worse, tomorrow this code could become incorrect again – if the copy-constructor for Info can throw, the right side code is still wrong.”

    What do you have in mind? This doesn’t seem obvious.

    1. Fair question. I should have more carefully pointed out that set_icon might take it’s second argument by value, which would then invoke copy construction. This is problematic as the Icon has been allocated but no object owns it yet, so will be leaked if an exception occurs.

Leave a comment

Your email address will not be published.

This site uses Akismet to reduce spam. Learn how your comment data is processed.