Cleaner, more elegant, and harder to recognize


It appears that some people interpreted the title of one of my rants from many months ago, "Cleaner, more elegant, and wrong", to be a reference to exceptions in general. (See bibliography reference [35]; observe that the citer even changed the title of my article for me!)

The title of the article was a reference to a specific code snippet that I copied from a book, where the book's author claimed that the code he presented was "cleaner and more elegant". I was pointing out that the code fragment was not only cleaner and more elegant, it was also wrong.

You can write correct exception-based programming.

Mind you, it's hard.

On the other hand, just because something is hard doesn't mean that it shouldn't be done.

Here's a breakdown:

Really easy Hard Really hard
Writing bad error-code-based code
Writing bad exception-based code
Writing good error-code-based code Writing good exception-based code

It's easy to write bad code, regardless of the error model.

It's hard to write good error-code-based code since you have to check every error code and think about what you should do when an error occurs.

It's really hard to write good exception-based code since you have to check every single line of code (indeed, every sub-expression) and think about what exceptions it might raise and how your code will react to it. (In C++ it's not quite so bad because C++ exceptions are raised only at specific points during execution. In C#, exceptions can be raised at any time.)

But that's okay. Like I said, just because something is hard doesn't mean it shouldn't be done. It's hard to write a device driver, but people do it, and that's a good thing.

But here's another table:

Really easy Hard Really hard
Recognizing that error-code-based code is badly-written
Recognizing the difference between bad error-code-based code and not-bad error-code-based code.
Recognizing that error-code-base code is not badly-written
Recognizing that exception-based code is badly-written
Recognizing that exception-based code is not badly-written
Recognizing the difference between bad exception-based code and not-bad exception-based code

Here's some imaginary error-code-based code. See if you can classify it as "bad" or "not-bad":

BOOL ComputeChecksum(LPCTSTR pszFile, DWORD* pdwResult)
{
  HANDLE h = CreateFile(pszFile, GENERIC_READ, FILE_SHARE_READ,
       NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  HANDLE hfm = CreateFileMapping(h, NULL, PAGE_READ, 0, 0, NULL);
  void *pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);
  DWORD dwHeaderSum;
  CheckSumMappedFile(pvBase, GetFileSize(h, NULL),
           &dwHeaderSum, pdwResult);
  UnmapViewOfFile(pv);
  CloseHandle(hfm);
  CloseHandle(h);
  return TRUE;
}

This code is obviously bad. No error codes are checked. This is the sort of code you might write when in a hurry, meaning to come back to and improve later. And it's easy to spot that this code needs to be improved big time before it's ready for prime time.

Here's another version:

BOOL ComputeChecksum(LPCTSTR pszFile, DWORD* pdwResult)
{
  BOOL fRc = FALSE;
  HANDLE h = CreateFile(pszFile, GENERIC_READ, FILE_SHARE_READ,
       NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  if (h != INVALID_HANDLE_VALUE) {
    HANDLE hfm = CreateFileMapping(h, NULL, PAGE_READ, 0, 0, NULL);
    if (hfm) {
      void *pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);
      if (pv) {
        DWORD dwHeaderSum;
        if (CheckSumMappedFile(pvBase, GetFileSize(h, NULL),
                               &dwHeaderSum, pdwResult)) {
          fRc = TRUE;
        }
        UnmapViewOfFile(pv);
      }
      CloseHandle(hfm);
    }
    CloseHandle(h);
  }
  return fRc;
}

This code is still wrong, but it clearly looks like it's trying to be right. It is what I call "not-bad".

Now here's some exception-based code you might write in a hurry:

NotifyIcon CreateNotifyIcon()
{
 NotifyIcon icon = new NotifyIcon();
 icon.Text = "Blah blah blah";
 icon.Visible = true;
 icon.Icon = new Icon(GetType(), "cool.ico");
 return icon;
}

(This is actual code from a real program in an article about taskbar notification icons, with minor changes in a futile attempt to disguise the source.)

Here's what it might look like after you fix it to be correct in the face of exceptions:

NotifyIcon CreateNotifyIcon()
{
 NotifyIcon icon = new NotifyIcon();
 icon.Text = "Blah blah blah";
 icon.Icon = new Icon(GetType(), "cool.ico");
 icon.Visible = true;
 return icon;
}

Subtle, isn't it.

It's easy to spot the difference between bad error-code-based code and not-bad error-code-based code: The not-bad error-code-based code checks error codes. The bad error-code-based code never does. Admittedly, it's hard to tell whether the errors were handled correctly, but at least you can tell the difference between bad code and code that isn't bad. (It might not be good, but at least it isn't bad.)

On the other hand, it is extraordinarily difficult to see the difference between bad exception-based code and not-bad exception-based code.

Consequently, when I write code that is exception-based, I do not have the luxury of writing bad code first and then making it not-bad later. If I did that, I wouldn't be able to find the bad code again, since it looks almost identical to not-bad code.

My point isn't that exceptions are bad. My point is that exceptions are too hard and I'm not smart enough to handle them. (And neither, it seems, are book authors, even when they are trying to teach you how to program with exceptions!)

(Yes, there are programming models like RAII and transactions, but rarely do you see sample code that uses either.)

Comments (116)
  1. Serge Wautier says:

    Look, I didn’t want to ask but there are 1000 guys pushing me in the back and since I’m the smallest one and I wear glasses, they picked me. (They always pick me… snif!)

    So… What’s wrong in ComputeChecksum() version 2 ?

  2. GregM says:

    If it’s still wrong, how is recognizing that code "isn’t bad" helpful? Isn’t any "wrong" code necessarily "bad"? Since your "another verson" example "is still wrong", how would you write it so that it is "right"?

  3. Memet says:

    Raymond,

    I agree that few books actually have solid samples. I remember even back a couple of years ago when Microsoft didn’t have solid samples, until it was understood that most programmers just copy/paste samples into their production code (or so I read about Microsoft’s policy).

    I still think though that it’s a matter of habit whether you see exceptions as difficult or not. From what I understand you are clearly used to error based code.

    In the sample you provided for example, the method doesn’t really follow the RAII model, and it is indeed subtle to spot the difference. But in my world, that function has failed if either new throws. So returning an icon that’s half built but not visible is not really what I’d want (because who knows, 5 lines down that call, someone might just simply set the Visible field to true).

    There are issues with exceptions, which are subtle, for instance unnamed variables a-la:

    SomeClass::SomeClass( void ) : someMember(new SomeOtherClass( ::getType() , blah )

    {

    // some code

    }

    That code is indeed wrong, because the order in which the expression inside the parenthesis is executed is undefined. getType might fail before the new is called. In any case, the object is temporary and will not be destroyed if an exception is thrown. Not cool.

    But for the class factory you show, I think it’s just a question of habit, either allow explicit instantiation:

    myIcon = new NotifyIcon();

    which can throw, in which case myIcon is either NULL or a valid object (no middle ground).

    or make the class factory behave the same way:

    NotifyIcon CreateNotifyIcon()

    {

    try

    {

    NotifyIcon icon = new NotifyIcon();

    icon.Text = "Blah blah blah";

    icon.Icon = new Icon(GetType(), "cool.ico");

    icon.Visible = true;

    return icon;

    }

    catch(…)

    {

    // your cleanup code

    throw; or return NULL;

    }

    }

    Although, I prefer the throwing version because returning NULL takes you back to error mode programming.

  4. Ryan Heath says:

    Serge,

    If CheckSumMappedFile (or one of the another functions) throws an exception, it will leak the handles.

    // Ryan

  5. Of course the great thing about the RAII pattern is that it works whether you’re using exceptions or not – and makes your code much cleaner if you’re not using them.

    I just wish there was a nice concrete and compact way of indicating that "when this object gets unwound, it does its own cleanup". But then, we’d end up with a weird keyboard with all kinds of dingbats on it.

  6. Valdas Sevelis says:

    Of course, if the NotifyIcon would provide a decent constructor (instead of default one), one would not have to guess the order of assignment to instance variables….

    e.g.:

    NotifyIcon CreateNotifyIcon()

    {

    NotifyIcon icon = new NotifyIcon(""Blah blah blah", "cool.ico");

    return icon;

    }

  7. Cooney says:

    Ryan:

    > If CheckSumMappedFile (or one of the another functions) throws an exception, it will leak the handles.

    So, you’re saying that even return code model stuff needs to be exception aware? I thought that the assumption was that the return code sample used returncodes throughout.

  8. CMV says:

    Serge: Also, GetFileSize could fail and return INVALID_FILE_SIZE, which would probably confuse the checksum algorithm.

    Ryan: In principle, yes, but all those functions are in the C-compatible API. Unless I’m missing something, they shouldn’t throw C++ exceptions if they fail.

  9. Hello Raymond,

    I’m the author the citation that you complained about. I went back to read my citation and your article. You are right! Your first article doesn’t object to exceptions per se. Please accept my apology.

    But, I think you will find we are in full agreement regarding the matter at hand. I cited your article in order to introduce a chapter on error propagation in distributed systems. An important theme in this chapter is that exceptions are misleading: it is easy to write beautiful exception code that gives the wrong result. Beauty is no substitute for correctness.

    Cheers,

    Doug

  10. I see two additional problems with ComputeChecksum() version 2.

    First, pdwResult is not checked for a NULL value before passing it into CheckSumMappedFile().

    Second, this code assumes the file being checksummed is less than 4GB. The call to GetFileSize() would yield unexpected results for files greater than 4GB. Also, an attempt to create and view a map of a really large file would probably fail.

  11. lowercase josh says:

    CreateNotifyIcon will leak memory if new Icon throws.

    Exceptions are a lot like compiler-sanctioned macros that hide flow control.

  12. CMV, as far as I know any Win32 API can raise an exception. Take for instance divide by zero, this would raise an exception. Unless you can see the code for the function you’re calling or know for a fact it can’t raise an exception, it’s safer to assume all functions are at least capable of raising an exception. So I think Ryan’s original point about leaked handles is valid.

    P.S. Darn you beat me to pointing out the GetFileSize() problem. :)

  13. Frederik Slijkerman says:

    What language is the CreateNotifyIcon() function written in? If it’s C++, then both versions are incorrect, because they don’t free the NotifyIcon if an exception occurs later.

    Furthermore, I’d like to argue that both functions are otherwise ‘good’ — the design of NotifyIcon is bad because you should be able to set properties in random order. This has nothing to do with exceptions, but with bad component design.

  14. Tom Seddon says:

    I don’t think divide by zero is the kind of exception raymond is talking about…?

  15. Rob Kennedy says:

    Frederik, I don’t think it’s unreasonable for NotifyIcon to require its Icon property be set before changing its Visible property. I might even expect set_Visible to raise an exception if there were nothing to display. That’s an exception that only happens once, though, while the developer is still learning to use the class properly. It might not be ideal, but it’s a reasonable design.

    We know that CreateNotifyIcon isn’t in C++ because of the syntax. So the function does not leak memory. Not permanently, anyway. If there is an exception, then the garbage collector will eventually release the NotifyIcon instance.

  16. DrPizza says:

    "(It might not be good, but at least it isn’t bad.) "

    That is rather a matter of opinion.

    I would say that code that to a cursory glance looks like it’s handling errors properly but isn’t is much worse than code that’s clearly not handling errors. The latter code has the common courtesy to set off alarm bells.

    "(Yes, there are programming models like RAII and transactions, but rarely do you see sample code that uses either.) "

    That’s because sample code is usually there to demonstrate something other than error-handling. Checking return codes in sample code is obfuscatory and frankly meritless.

    Techniques like RAII make writing good code easy, particularly if you’re only demanding a weak exception-safety guarantee.

  17. Jeff Atwood says:

    Subtle, isn’t it.

    When is it ever a good idea to set something to .Visible() prior to it being completely loaded? Doesn’t seem all that subtle to me.

  18. Memet says:

    Brian Friesen, Tom Seddon:

    Divide by Zero is not a C++ runtime exception (provided by the language), it’s a structured exception (SEH) provided by the platform. Two completely unrelated things.

  19. Memet, I am aware of the difference between C++ exceptions and SEH. However a C++ try/catch block can catch both types of exceptions. So Ryan’s original point about leaked handles is still valid. If one of the Win32 APIs raises an exception, regardless of the TYPE of exception, you should still catch it and free resources appropriately.

  20. Waleri says:

    Once upon a time, they told me it is a bad programming style to use goto. Asking me, exceptions are even worse than goto, since you don’t know where your code will end up. Yes, they are safe and yes, they will unwind your stack (probably, I really hope so), but still they are a complex goto to me. I think I’ll agree with Raymond on that one – I am not smart enough to handle them.

    By the way, Memet’s example above is a good example how things can go from "bad" to "worse" – well, at least if C++ is used.

    As for Raymond’s "not bad" version, question is, why should I bother checking intermediate error values, since every API used checks input too?

    HANDLE hFile = CreateFile();

    HANDLE hMap = CreateFileMapping();

    LPVOID ptr = MapViewOfFile();

    if (ptr) CalcCheckSum();

    UnmapViewOfFile(ptr);

    CloseHandle(hMap);

    CloseHandle(hFile);

    As simple as that. Imagine you have a lot of preconditions to handle… adding additional if’s will produce something like:

    if (cond)

    if (another)

    if (yet another)

    if (we still running)

    if (gosh this maybe last one)

    DoSomethingUseful();

    now imagin every if with corresponding else, then imagine you have to rearrange couple conditions? In C++ it could be much more readable, without any execptions, will full error checkings and so on

    CFile file = CreateFile();

    if (!file) return;

    CFileMap map = CreateFileMap();

    if (!map) return;

    CFileView view = MapViewOfFile();

    if (!view) return;

    DoSomethid();

    return;

    Viva C++

  21. Waleri says:

    P.S. how do I enter code lines with indents here?

  22. Raymond Chen says:

    When is it ever a good idea to set

    > something to .Visible() prior to

    > it being completely loaded?

    Tell that to the person who wrote the sample code I was quoting.

    > Doesn’t seem all that subtle to me.

    Okay, fine "Visible" carries too much semantic weight. Here’s something less obvious:

    Node ExtendPath(Item item)

    {

    Node node = new Node();

    path.Add(node);

    node.item = item;

    node.color = item.GetColor();

    return node;

    }

    Waleri: But what if you have methods that crash on invalid parameters? For example,

    IEnumConnectionPoints penum;

    pcpc->EnumConnectionPoints(&penum);

    penum->Next(); // oops

    In your C++ version you’re conveniently glossing over the complexity of, for example, making sure the file map is destroyed after the view (because the view relies on the file map). Are you going to use reference counting?

    Other issues brought up in the comments I already have plans to discuss later, so I’ll let them go for now.

  23. Jeff Atwood says:

    Other issues brought up in the comments I already have plans to discuss later, so I’ll let them go for now.

    OK, but one last thing: if I got an exception on New Icon(), the code would terminate execution immediately and get kicked to the registered global exception handlers.

    So unless I’m missing something, any code downstream of New Icon()is effectively moot anyway; the program will abend. Note that we CAN’T say this about return codes. The program plows on heedlessly.. you can’t "forget" an exception because you abend.

  24. Geoffrey says:

    Somebody,

    Because your code is broken. At the very least, you need to declare the pv, hfm, and h variables outside of the try scope. And you need to initialize those variables to null, null, and INVALIDATE_HANDLE_VALUE respectively. And then you need to check against those values before calling UnmapViewOfFile, CloseHandle, CloseHandle, etc.

    – Geoff

  25. Waleri says:

    Waleri: But what if you have methods that crash on invalid parameters? For example,

    >>>IEnumConnectionPoints penum;

    pcpc->EnumConnectionPoints(&penum);

    penum->Next(); // oops

    Well, this isn’t exactly the case

    ptr->Method() isn’t DoSomething(ptr);

    In first case we’ll crash, second case depends on DoSomething implementation.

    I agree, that this isn’t the best way to do things, I prefer the C++ version. My point was that using many nested if/else and/or try/catch makes things unreadble and makes execution workflow hard to trace

    >>> In your C++ version you’re conveniently glossing over the complexity of, for example, making sure the file map is destroyed after the view (because the view relies on the file map). Are you going to use reference counting?

    Well, in this case they will be destroyed in correct order. If I have multiple mappings from a single map, then perhaps other implementation will apply. Frankly I really don’t know what to do if for some reason view unmapping fails. I never had this problem – when object is created correctly, it is always destroyed correctly.

  26. Somebody says:

    The compiler will easily catch the declaration errors, and I assumed that the CloseHandle functions would just do nothing when called on an already closed handle. But the point of my question was not to get into the minutia of c++/c# and file function behaviours. My question is "What is generally wrong with exceptions that make them more complicated than checking return values?" The only difference I see between them is that exception systems just hides the return code and provides syntax for you to decide where you want to deal with it.

  27. Vorn says:

    If only C++ compilers enforced the throw() declaration. Then we’d always know where we end up, and all we’d have to worry about is screwing up operation order. Which can be pretty heinous in itself, but at least we don’t have to worry about our exceptions crashing the program because we haven’t handled them.

    Java is the only language I know of that has an enforced exception declaration… if only people actually used it.

    Vorn

  28. Somebody says:

    "Once upon a time, they told me it is a bad programming style to use goto. Asking me, exceptions are even worse than goto, since you don’t know where your code will end up. Yes, they are safe and yes, they will unwind your stack (probably, I really hope so), but still they are a complex goto to me."

    Try-catch blocks are no more a goto than if-else blocks, while blocks, or for loops. They are just another structure to do structured programming with.

  29. Alex says:

    (continued)

    And it would probably be a good idea to assign pdwResult to a fail-state.

    The problem with exceptions is that you have to know each exception that could be thrown by the functions that you’re calling, and handle them, or explicitly ignore them.

  30. Waleri says:

    >>"What is generally wrong with exceptions that make them more complicated than checking return values?"

    Well, nothing’s wrong if you develop in an evnvironment that doesn’t rely on the programmer to "cleanup". If one have to close handles, release memory, etc, exceptions aren’t the best choice.

  31. autist0r says:

    Exceptions are cool because you can provide information about the error to the catcher in a clean fashion, they can for example retrieve the error with ::GetLastError() or read errno…

    But writing good exception-error-l33t-code needs to be done from scratch and thought like multithreaded code… Not so hard… If you’re trained. :) Requires a paranoid mind methinks.

    Somebody, your code is wrong, you shouldn’t try to close valid handles values… I would rather do it like this :

    try

    {

    HANDLE h = ::CreateFile(blah blah);

    if (h == INVALID_HANDLE_VALUE)

    throw CSome1337Exception();

    // … do something

    ::CloseHandle(h);

    h = INVALID_HANDLE_VALUE;

    }

    catch(…)

    {

    if (h != INVALID_HANDLE_VALUE)

    ::CloseHandle(h);

    throw; // because exception should be handled and displayed at high level only IMHO

    }

    Just my 0.02 € :p

  32. autist0r says:

    omg my code is wrong, of course h should be declared outside the try, all my apologies :(

  33. Rob Kennedy says:

    Waleri, being in an environment that requires programmers to write cleanup code isn’t a problem for exceptions. That’s why Microsoft C++ provides __try/__finally, for instance. Close your handles and release your memory in the finally blocks.

    If you’re using standard C++, then use smart handles and pointers that know to release their resources when the stack unwinder destroys them.

    If you’re using a language that doesn’t have garbage collection, doesn’t have try-finally blocks, and doesn’t have stack-unwindable objects, then I don’t know what you’re supposed to do. It strikes me that such a language shouldn’t be supporting exceptions in the first place.

  34. Somebody says:

    "But writing good exception-error-l33t-code needs to be done from scratch and thought like multithreaded code… Not so hard… If you’re trained. :) Requires a paranoid mind methinks."

    I don’t think that you have to be any more paranoid than when using error codes. You have to look at each function call and decide what should happen if an exception is thrown. If the function modifies any state then a rollback strategy needs to be implemented. This is exactly the same as in error-checking code. You have to check the error code of each function call and do roll-backs if any state is modified. To me, exception handling seems like a more flexible system. You can decide where you do the error handling and it encourages you to use much richer error information than just a stupid number.

  35. Raymond is absolutely correct. But some of the comments are just totally wrong and/or missing the point:

    > I assumed that the CloseHandle functions

    > would just do nothing when called on an

    > already closed handle.

    Handles are just numbers and can be reused.

    Thread A: CloseHandle(1234);

    Thread B: CreateMutex(…); // returns 1234

    Thread A: CloseHandle(1234);

    See the problem?

    > as far as I know any Win32 API can raise an > exception.

    Not under normal conditions. If you call CreateThread for example and it throws a SEH exception then the only safe thing to do is to kill your process.

    There are a few exceptions to this (some APIs are documented to report failures with SEH exceptions and/or are using them internally) but in general most win32 APIs do not use SEH, and are not guaranteed to be SEH-safe.

    > However a C++ try/catch block can catch

    > both types of exceptions.

    This is a bug in VC++. It’s been fixed in Whidbey – with default options, catch(…) no longer catches SEH exceptions.

    > Isn’t this a problem with unchecked

    > exceptions or am I missing something?

    How would it help if the function declaration had a throws list? Detecting bad code in the Raymond’s example would still be hard.

    Not to mention that "checked" exceptions in Java are only checked if you ignore everything derived from RuntimeError (or whatever it’s called).

  36. You have to look at each function call and decide what should happen if an exception is thrown […]. This is exactly the same as in error-checking code.

    Right, you have to do the same (or similar) things in both cases. The difference is that with error codes, the results of this process are obvious – you have the error checking code right there in front of you. If there is no error checking then obviously there is a problem.

    With exceptions you are never sure – you might have a bunch of function calls and no try/catch/finally’s. Is this because the code is buggy, or because somebody has carefully examined it and proved that it’s exception-safe?

    It’s possible that if you train yourself to constantly think about exception safety you might eventually be able to spot bugs in large amounts of code that uses exceptions even more efficiently than with error checking. But I think most people do not have this ability.

  37. GregM says:

    Serge,

    Well, for one, it won’t compile.

    void *pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);

    if (pv) {

    DWORD dwHeaderSum;

    if (CheckSumMappedFile(pvBase, …

    should be

    void *pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);

    if (pv) {

    DWORD dwHeaderSum;

    if (CheckSumMappedFile(pv, …

  38. Somebody says:

    "Thread A: CloseHandle(1234);

    Thread B: CreateMutex(…); // returns 1234

    Thread A: CloseHandle(1234);

    See the problem? "

    Yes. I also assumed they were thread-safe. Just like the CreateHande functions. But that still does not explain that dealing with all of this is any more of a hassle using try-catch blocks.

  39. Somebody says:

    "The difference is that with error codes, the results of this process are obvious – you have the error checking code right there in front of you."

    Yes. But all of that error checking code obfuscates what the function is supposed to do. It camouflages the logic of the function with all of the nested ifs. I guess it depends on what is more important to you at the time. Do you want to find out what the function does? If yes, than having all of the logic exploded by the error code makes things harder. If all you care about is the error handling code, than having it all visible and in one place makes it easier.

  40. Raymond Chen says:

    Somebody: I guess I don’t know what you mean by "thread safe" then. The handle functions are thread safe in the sense that you can safely use a handle created on any thread on any other thread, and you can use a handle simultaneously on two threads. Destroying an object twice is unrelated to thread safety.

    Yes, it’s annoying seeing all the error handling code – but at least you know that it’s there. You can see that an error value was checked or ignored. But with exceptions you can’t tell whether an exception was left unhandled by mistake or on purpose.

  41. Somebody says:

    I’ve never really seen an offcial definition but "thrad safe" to me means that one thread can not affect the state of another thread.

  42. Somebody says:

    I forgot to add "… unless explicitly given permission by the other thread."

  43. Jonas Maurus says:

    Isn’t this a problem with unchecked exceptions or am I missing something? If you’d have to explicitly check every exception like you have to in Java, this problem would go away, right?

    Mind you, I’m not saying Java is better or worse than any .Net technology, that’s not the argument I want to make, I just needed an example.

    In my mind, the .Net Exception model is useful to raise Exceptions for errors you don’t *expect* to happen and use errorcodes for the rest, while in the Java world Exceptions are indeed useful to propagate problems through the code. I also was under the impression that the class library was designed with this idea in mind, or not?

  44. Raymond Chen says:

    "one thread cannot affect the state of another thread" -> By that definition, not even ‘new’ is thread safe. ‘new’ affects a global shared resource (namely the free store) and therefore has cross-thread consequences.

  45. Matt Barry says:

    [quote]This is a bug in VC++. It’s been fixed in Whidbey – with default options, catch(…) no longer catches SEH exceptions.[/quote]

    This is a bug? Richter’s book states that Microsoft added this feature to the compiler. I always thought it convenient that you could rely on catch(…) for access violations thrown by memory mapped files.

  46. Somebody says:

    "By that definition, not even ‘new’ is thread safe."

    Well no. ‘New’ is supposed to raise an exception when it can’t allocate memory. One thread calling ‘new’ has no effect on another thread that has already called ‘new’. It will have an effect on the other thread’s ability to call ‘new’ again but that is part of the interface of ‘new’. I guess, my more detailed definition of a "thread safe" API is that a thread will not cause any behavior in another thread that is not explicitly defined by the API.

  47. What would be nice is if there were an "rollback" block in C# (or whatever language). The semantics would be:

    try {

    DoSomething();

    } rollback {

    UndoSomething();

    }

    would be exactly equivalent to:

    try {

    DoSomething();

    } catch (Exception e) {

    try {

    UndoSomething();

    } catch {}

    throw e;

    }

    (I’m assuming, here, that the right thing to do on a failure of the Undo step is to ignore it. I don’t know what correct practice is here, but since you can only return one error condition from the function (regardless of whether you’re using exceptions or error codes) it seems preferable that external code should see the original error, rather than the error-cleaning-up-the-original-error. An alternative would be to throw the second error, but add the original to a chain of "causes". I don’t think .NET has such a thing, and it would really need to be a tree, because both exceptions might already have causes by the time they got here.)

    However you handle it, I think that like "using", it’s a common enough pattern in correct code to be worth having syntax for. Consider the Database example from your original post:

    public void GenerateDatabase() {

    try {

    CreatePhysicalDatabase();

    CreateTables();

    CreateIndexes();

    } rollback {

    DeletePhysicalDatabase();

    }

    }

    The only gotcha to remember here is that DeletePhysicalDatabase has to remember to handle the case where the database *didn’t* get created yet because CreatePhysicalDatabase() itself failed. Depending on whether CreatePhysicalDatabase() is itself atomic (or does its own cleanup correctly on failure) you could instead write:

    public void GenerateDatabase() {

    CreatePhysicalDatabase();

    try {

    CreateTables();

    CreateIndexes();

    } rollback {

    DeletePhysicalDatabase();

    }

    }

    Oh, and as far as your "easier to recognize bad code" point goes, I disagree that it’s necessarily easy to recognize bad code in the error-handling case. If there’s no error handling it might be because the particular functions being called *can’t* fail (int add(int a, int b) would have a hard time failing). You still have to look at each chunk of code to know whether it needs error handling or not. On the other hand, with my proposed rollback clause, you could easily "document" the no-possible-failure or no-handling-needed cases like this:

    try {

    DoSomething();

    } rollback {} // No handling needed for failure

    That also has the advantage that you could write an FxCop rule to ensure that *everything* (for some class of everything) is rolled back properly. Perhaps every method body, for example…

  48. Raymond Chen says:

    It is implicit in all APIs that you cannot pass invalid parameters. Double-closing a handle is an invalid parameter on the second call since the handle has already been closed.

    I can’t believe I have to explain this.

  49. Somebody: consider this example then.

    Thread A: free(0x12345678);

    Thread B: malloc(…); // returns 0x12345678

    Thread A: free(0x12345678);

    This is an obvious bug in the caller and has nothing to do with thread safety of malloc/free.

    The CreateMutex/CloseHandle case is no different.

  50. This is a bug? Richter’s book states that

    > Microsoft added this feature to the

    > compiler. I always thought it convenient

    > that you could rely on catch(…) for access

    > violations thrown by memory mapped files.

    The fact that catch(…) could catch SEH exceptions even when compiled with /EHs (which has been the default since VC5) was most definitely a bug.

    By the way, when dealing with memory mapped files you should be catching EXCEPTION_IN_PAGE_ERROR, not access violations or anything else. Using catch(…) for this purpose is a really bad idea since it will catch everything (access violations, stack overflows, illegal instructions, STATUS_NO_MEMORY etc), making all these other problems impossible to debug.

  51. Somebody says:

    I still don’t see how exceptions make things more difficult. How is the difference between:

    DWORD* ComputeChecksum(LPCTSTR pszFile)

    {

    DWORD* pdwResult;

    HANDLE h = CreateFile(pszFile, GENERIC_READ, FILE_SHARE_READ,

    NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    HANDLE hfm = CreateFileMapping(h, NULL, PAGE_READ, 0, 0, NULL);

    void *pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);

    DWORD dwHeaderSum;

    CheckSumMappedFile(pvBase, GetFileSize(h, NULL),

    &dwHeaderSum, pdwResult);

    UnmapViewOfFile(pv);

    CloseHandle(hfm);

    CloseHandle(h);

    return pdwResult;

    }

    and:

    DWORD* ComputeChecksum(LPCTSTR pszFile)

    {

    DWORD* pdwResult;

    try {

    HANDLE h = CreateFile(pszFile, GENERIC_READ, FILE_SHARE_READ,

    NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    HANDLE hfm = CreateFileMapping(h, NULL, PAGE_READ, 0, 0, NULL);

    void *pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);

    DWORD dwHeaderSum;

    CheckSumMappedFile(pvBase, GetFileSize(h, NULL),

    &dwHeaderSum, pdwResult);

    UnmapViewOfFile(pv);

    CloseHandle(hfm);

    CloseHandle(h);

    }

    catch(Error e) {

    UnmapViewOfFile(pv);

    CloseHandle(hfm);

    CloseHandle(h);

    throw(e);

    }

    return pdwResult;

    }

    any more difficult to spot than the differences in the error-based-code you provided.

  52. Somebody says:

    "It is implicit in all APIs that you cannot pass invalid parameters. Double-closing a handle is an invalid parameter on the second call since the handle has already been closed."

    But it is also implicit in the thread safe API that what is returned from a successful CreateHandle call can not be modified by any other thread unless it is passed to it. You are guaranteed that the handle will be valid until it is closed by the same thread. Should the fact that other threads have bugs mean that this assumption is invalid? I don’t know. But you would have a much more robust system if the answer was no.

  53. Somebody says:

    Yes, by the definition that you gave for thread safety the assumption would not be valid. Still, that’s a pretty pedantic API. I wouldn’t want to use it.

  54. Raymond Chen says:

    I think you’ll find that most handle-based APIs operate under the same rules. Once you close a handle it is illegal to do anything with it (including close it again).

  55. Someone says:

    We don’t need exceptions. E_UNEXPECTED is good for everything!

    … And yet it takes complexity to manage complexity.

  56. Somebody says:

    "I think you’ll find that most handle-based APIs operate under the same rules."

    That may be true, but I don’t think that they should. It is a pretty trivial fix to get them so that they don’t. This is one of those things that can move things towards "do what I mean" instead of "do what I say."

  57. Grant says:

    and even if you discovered that

    > there are two points in the

    > statement "a = foo(b, c) + bar(x,y);"

    > which throw (and may throw a

    > possibly overlapping set of exception

    > types) how the heck can you figure out

    > that you have to clean up the side

    > effects of foo() vs. the side effects

    > of bar()?

    Maybe I’m missing something, but couldn’t this be solved with:

    try {

    d = foo(b, c);

    } catch { … }

    try {

    z = bar(x, y);

    } catch { … }

    a = d + z;

    Although believe me, I’m not thrilled with the notion of having to wrap every single exception generating line of code in it’s own try/catch block.

    If I’m going to have to do that, I’d rather write:

    if ((d = foo(b, c)) != null) {

    if ((z = bar(x, y)) != null) {

    a = d + z;

    } else { … bar failed … }

    } else { … foo failed … }

    Which I guess is partly Raymond’s point.

  58. Cooney says:

    Pavel:

    How would it help if the function declaration had a throws list? Detecting bad code in the Raymond’s example would still be hard.

    Not to mention that "checked" exceptions in Java are only checked if you ignore everything derived from RuntimeError (or whatever it’s called).

    That’s deliberate – under no circumstances should proper code throw a RuntimeException. The other stuff can show up in normal execution. What this means is that normal exceptions are part of a method’s fingerprint and must be handled. Runtime stuff is usually not handled.

  59. Matt Barry says:

    "By the way, when dealing with memory mapped files you should be catching EXCEPTION_IN_PAGE_ERROR, not access violations or anything else. Using catch(…) for this purpose is a really bad idea since it will catch everything (access violations, stack overflows, illegal instructions, STATUS_NO_MEMORY etc), making all these other problems impossible to debug."

    That makes sense. Thank you for clearing that up for me.

  60. Alex says:

    One of the problems I see is that CreateFile and CreateFileMapping don’t throw exceptions.

    Mixing exception-throwing code and error-return code will always result in something that looks really verbose, because you’ve got lots of

    if( something_fails )

    {

    throw SomeException();

    }

    and that doesn’t take into things like saving the error code and embedding it in the exception.

    If you’ve got a system using RAII calling functions that throw exceptions (and the exceptions are documented), exception code works. If you don’t, you end up with the exact same problem that you have with error-code functions.

    The optimium version of the compute checksum function, assuming we had exception-throwing variants of CreateFile and CreateFileMapping, RAII HANDLES and pointers to the mapped file, and wanted to shield callers from normal exceptions would look something like like:

    DWORD* ComputeChecksum(LPCTSTR pszFile)

    {

    DWORD* pdwResult;

    try

    {

    HANDLE_RAII h = CreateFile(pszFile, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    HANDLE_RAII hfm = CreateFileMapping(h, NULL, PAGE_READ, 0, 0, NULL);

    FILEMAPVIEW_RAII pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);

    DWORD dwHeaderSum;

    CheckSumMappedFile(pv, GetFileSize(h, NULL), &dwHeaderSum, pdwResult);

    }

    catch( … )

    {

    }

    return pdwResult;

    }

  61. Jeff Atwood says:

    Per Chris Brumme:

    So, if you are not a programming God like those OS developers, you should consider using exceptions for your application errors. They are more powerful, more expressive, and less prone to abuse than error codes. They are one of the fundamental ways that we make managed programming more productive and less error prone. In fact, the CLR internally uses exceptions even in the unmanaged portions of the engine. However, there is a serious long term performance problem with exceptions and this must be factored into your decision.

    http://blogs.gotdotnet.com/cbrumme/permalink.aspx/d5fbb311-0c95-46ac-9c46-8f8c0e6ae561

    Yes, Raymond Chen is a programming god, but not all of us are so fortunate.

  62. Chris Nahr says:

    I agree with Ian Nowland. The potential issues Raymond’s code shows are resolved by using the proper idiom: first create an object, then complete its initialization, then use it (adding to collection, showing on screen, whatever).

    This sequence is completely natural once you’re used to it, and violations are fairly easy to spot as well — to my eyes anyway. Besides, it naturally lends itself to grouping the initialization steps into a separate "paragraph", or factoring them out into a separate method.

    In fact, I don’t see how this issue relates to error codes vs exceptions at all. Using an object before all required init steps have succeeded is always wrong. It’s just as wrong when you’re using error codes. You’ll at least have wasted time; possibly you’ll have written erroneous data to disk, or caused the screen to flicker, or whatever.

  63. Since I’m opinionated, for once I’ll not restate my whole opinion and instead make some minimalist observations.

    1. The "cleanup"/"restore invariant" problem has several solutions. The best solution probably looks like a combination of RAII (for things which represent resources), try/finally (for restoring invariants in both error and non-error paths) and try/rollback (to use the previous commenter’s name; I don’t have a better one…) Oh and IDisposable is OK too when you have "using" to go along with it. I think that Herb Sutter would argue "isn’t this just destructors with funny syntax?" and I’d tend to agree. Unfortunately no language has all of these.

    2. cleanup/invariant restoration is hard but most of the problems there exist regardless of exceptions or not. I do say most because if expressions can have side effects which need to be rolled back on error, it’s very hard to see just where the error/exit paths are. As much as I’m in support of Raymond on this topic in general (even if he hates my error handling macros), I think that the error of the icon example would be just as unobvious in an error handling world.

    I don’t recall what it’s called but there’s yet another programming style where enforced state transitions are modelled as separate objects with only constructors to demonstrate the state transition. So your code would look something like:

    int Foo(int x) {

    A a(x);

    B b(a, 7);

    C c(b, x, -1);

    return c.ReturnValue();

    }

    thus you constrain against sequencing / potential lost rollbacks by virtue that you can only do the operation implied by the C instance c if you already have a B, which can only be gotten if you already had an A.

    I don’t know how much this is an ivory tower model vs. useful. It seems somewhat crazy to me, but it’s the only suggestion I’ve heard of which makes RAII so ingrained in the pattern that even side effects and cleanup are guaranteed by virtue of using it.

    3. The /really/ hard thing here is catching exceptions. Let’s ignore the insanely bogus try { } catch (…) { /* cleanup */ throw; } pattern because even if you just meant to clean up, you didn’t. You caught the exception and that fact in and of itself causes global side effects which will be hard to wriggle away from.

    I won’t repeat my examples; go to my blog and read. Any statement which has more than one point within it that can raise an exception makes catching exceptions raised by that statement ambiguous at best. Further you can’t tell without whole-program analysis and even if you discovered that there are two points in the statement "a = foo(b, c) + bar(x,y);" which throw (and may throw a possibly overlapping set of exception types) how the heck can you figure out that you have to clean up the side effects of foo() vs. the side effects of bar()?

    In practice you need to ensure that within a scope (e.g. try { /* scope! */ } catch …), there is only one code path which can cause a failure. Otherwise you have no hope of being able to restore state.

    —-

    All this forms a limit to how high quality exception based code can be. If you can convince yourself that "exceptions are fat statuses with a special return value channel, coupled with a non-local goto capability", then arguably status based code has the same fundamental limit.

    The thing is that Raymond’s blog here is right on the nose. It’s way easier for bad exception based code to pass code review and test passes than bad status based code. Maybe we need to get whole-program-analysis engines surgically grafted into our brains at some point but with the modern programming languages it’s impossible to even look at a piece of code and have any idea how bad it is, with all the conversions and operator overloading etc.

    I’m very afraid that this whole fad is going to kill the software industry because we’ve enabled an order more magnitude code to be written per developer but the quality is a fraction of what it was and there is nearly zero perception/understanding that the flaws that exist today are actually flaws at all.

  64. Raymond Chen says:

    "You are guaranteed that the handle will be valid until it is closed by the same thread."

    No, it is valid until closed by *any* thread. HANDLEs do not have thread affinity. They are thread-safe under the more traditional definition I gave earlier: http://weblogs.asp.net/oldnewthing/archive/2005/01/14/352949.aspx#353366

  65. Ian Nowland says:

    To me if you are using exceptions and you are not using RAII or some other well defined and described idiom, I know immediately that the code is badly written. This is *really easy* to determine. The code may work, but it is still badly written.

    Really you’re argument boils down to firstly "exceptions in and of themselves are not a good error handling idiom," which I agree with. However you then try to tack on " as it is very hard to determine whether exception-based code is bad or good you should never write sample code that relies on exceptions for error handling", which from my previous point I totally disagree with – it is very obvious when you are not using exceptions as part of some larger idiom.

    If I was to split your single "exception-based" categorization into "exception-based-without-idiom" and "exception-based-with-idiom” categories, then you’re original tables would become:

    Really Easy:

    – writing bad code of any type

    Hard:

    – writing good error-based or exception-based-with-idiom code

    Impossible:

    – writing good exception-based-without-idiom code (as even though it may work, if something is hard to understand then it is bad)

    And:

    Really Easy:

    – Recognizing that error-code-based code or exception-based-with-idiom code is badly-written

    – recognizing the difference between bad error-code-based code and not-bad error-code-based code.

    – recognizing the difference between exception-based-without-idiom code and exception-based-with-idiom code

    – recognizing that exception-based-without-idiom code is badly or not-badly written (it is always badly written!)

    – determining the normal (i.e. non-error) flow of exception-based-without-idiom and exception-based-with-idiom code.

    Somewhat hard:

    – determining the normal (i.e. non-error) flow of error-code-based code

    Hard:

    – Recognizing that error-code-based or exception-based-with-idiom code is not badly written

    This seems to suggest that there should be no problem with using exception based-code in samples. It should be immediately obvious to an educated reader whether or not it is using a good idiom and is thus "bad" from the point of view of handling errors or not. It also suggests that in production code, as long as you can establish a larger idiom for error handling, then you should use exceptions-with-idiom as opposed to error-code based code.

  66. > under no circumstances should proper code

    > throw a RuntimeException. The other stuff

    > can show up in normal execution.

    http://java.sun.com/j2se/1.4.2/docs/api/java/lang/RuntimeException.html

    Take a look at the list of subclasses.

    If I’m analyzing a piece of code to determine whether it’s exception-safe or not, I most definitely do *not* want to ignore things like IllegalArgumentException simply on the basis that they should never be thrown by "proper code".

  67. autist0r says:

    Somebody, exceptions are harder to use because of the numerous possible hidden path of execution they create, but you are very right on their advantages.

    Some evil example :

    FILE *f;

    try

    {

    f = fopen(blah);

    // blah blah

    fclose(f);

    f = NULL;

    }

    catch(…)

    {

    fclose(f);

    throw;

    }

    The code above is ultra evil and will provoke an error much later in the code in some cases (at the next fopen)… See why ? :)

    Hint, the bug is not present in this code :

    FILE *f = fopen(blah);

    if (!f) return error

    //blah

    fclose(f);

    Why was it considered a but that catch(…) catches SEH ? To me it’s a feature… :p Ok, the clean way is to create a class and do a little SetUnhandledExceptionFilter…

    Also don’t forget that throwing an exception while in a constructor can be ultra evil too.

  68. DrPizza says:

    "The potential issues Raymond’s code shows are resolved by using the proper idiom: first create an object, then complete its initialization, then use it (adding to collection, showing on screen, whatever).

    "

    Er… the proper idiom is that creation and initialization are one and the same; that you can’t create uninitialized (invalid) objects.

  69. Not if the resources used are expensive.

    For example:

    class Icon

    {

    private:

    HICON m_icon;

    public:

    Icon() : m_icon(NULL)

    {};

    Create(RESID id)

    { … }

    }

    So while you don’t have uninitialized member variables, you certainly end up with a not entirely created resource.

  70. Clooney says:

    Raymond started to learn how to program with exceptions :) Way to go! Is it hard for him? Yes it is indeed. But he’s doing a good job. There are still many things he’s clueless of. His statement on RAII made me laugh. Anyway as he goes on and become more familiar with the concept of exceptions, his posts will be more balanced than this one.

  71. Chee Wee says:

    Waleri: But what if you have methods that crash

    >on invalid parameters? For example,

    What to do if there are methods that crash on invalid parameters? Microsoft already decided what to do in that case. Validate all parameters. ;o)

    I can’t remember when that was first implemented in Microsoft code. NT 3.1 or NT 3.5? Or was it even way earlier?

  72. Maxime LABELLE says:

    "Also don’t forget that throwing an exception while in a constructor can be ultra evil too."

    Well, at least in C++, part of a good idiom that makes using exceptions correctly possible is to *indeed* allow constructor to raise exceptions.

    On the other hand, destructors in C++ should never *ever* be allowed to throw. That’s problably the only place where the dreaded

    try { /* cleanup */ } catch (…) {} is not evil.

    That is the basic assumption that underlies RAII, and coupled with the swap idiom, this makes using exceptions very easy and I believe correct as well.

    Of course, the problem wherein the next guy hasn’t got a clue whether a method lacks exception handling on purpose or as an oversight remains ; but that is not a problem with the code, that is a problem with the team communications and guidelines…

  73. Thread Hijack warning:

    Chee Wee: The validation mantra came in on Windows 3.1 (in the name of "crash protection").

    It continued forwards through the OS, and is now generally out of favor (checking for null pointers is considered "ok" but any other validation isn’t).

    The problem of validating beyond null pointer checks is that the validation is utterly pointless, and can in fact introduce massive security holes in your application – Raymond and I have both written about this in the past.

  74. I think Chee Wee was talking about parameter validation for NT system services. This is a totally different thing. The OS is supposed to be isolated from the apps so it can’t afford crashing on invalid parameters. Inside a single process there’s no isolation so trying to avoid crashes on bogus pointers doesn’t make the process more reliable, it only hides bugs.

  75. Pavel, you mean midl /robust? If that’s the case, then you’re absolutely right. But there WAS a historical effort (now thoroughly discredited IMHO) to use SEH to try to validate inputs from within the process as well.

    I think the MIDL /robust flag went in during one of the SPs on NT4, but I may be wrong – the RPC marshaller has been made successively more bulletproof with each release of windows, so…

  76. No I was talking about what happens if you pass garbage parameters to system services like NtCreateProcess etc. The kernel uses SEH when accessing pointers passed to it from user mode.

    But in user mode passing invalid pointers to APIs should (and usually does) crash the process.

  77. DrPizza says:

    "So while you don’t have uninitialized member variables, you certainly end up with a not entirely created resource. "

    The utility of the class you’ve created is decidedly non-obvious. Why would you want an icon with, er, no icon?

  78. DrPizza says:

    "(In C++ it’s not quite so bad because C++ exceptions are raised only at specific points during execution. In C#, exceptions can be raised at any time.)"

    If one’s using an SEH platform, C++ can have asynchronous exceptions too….

  79. Petr Kadlec says:

    Stuart Ballard: I know at least one language that does have catch/rollback/commit/leave blocks.

  80. Martin Everett says:

    Chris Nahr: "The textbook phrase "creation is initialization" only applies on a very basic level. That is, all properties have well-defined values, and accessing the object and its properties doesn’t crash the program."

    Well, I’m not sure if "creation is initialization" is all that commonly used. NI the C++ end of things, the phrase is "resource acquisition is initialization", and it means what it says – that you acquire resources on constructing your object. If you want to change some trivial property later on, then fine. But if your object is acquiring something that may fail to be available, eg opening a file, getting a mutex, it’s better to do it in the constructor.

    The idea is that once you’ve constructed an object, it’s vaild and you can use it. You don’t have to remember to call some separate init() function, or to check if it’s been called already.

    One common mistake in OO code is to have too much changing the state of objects after they’re created. In fact, by moving the initialization stage into constructors where possible, you can often have the majority of your objects constant once they’ve been created. This makes it easier to reason about the state of your program – in particular it becomes a lot easier to share things between threads.

  81. okay, pasting did scewup indentation, sigh.

    http://verein.lst.de/~hch/oldnewthing-error-handling.c has the right version.

  82. Christoph, your code looks an awful lot like a "try-finally" or "using" construct in C#. Your specific example seems to be closing everything down on success as well as failure, but it’s easy to see how you’d modify it to only close stuff down on failure. I’d argue that "using" is much easier to read than your version for the "close everything down regardless" case, and my suggested "try-rollback" construct is easier than the "only close stuff on failure" construct:

    using (Handle handle = CreateFile(…)) {

    using (Handle hfm = CreateFileMapping(…)) {

    using (Something pv = MapViewOfFile(…)) {

    return CheckSumMappedFile(…);

    }

    }

    }

    Handle handle = CreateFile(…);

    try {

    Handle hfm = CreateFileMapping(…);

    try {

    Something pv = MapViewOfFile(…);

    return pv;

    } rollback {

    hfm.Close();

    }

    } rollback {

    handle.Close();

    }

  83. Paul Spendlove says:

    Please, please, PLEASE could anyone posting about exceptions in C++ read Bjarne Stroustrup’s "Appendix E" note on exception handling? It has been present online for more than 4 years, at the location http://www.research.att.com/~bs/3rd_safe0.html. If someone posts and obviously hasn’t read this, they aren’t doing their reputations any favours.

    And a lot of the people posting on this topic have clearly *not* read this, since they are coming up with some common misconceptions that Bjarne’s document addresses:

    Misconception #1: Constructors shouldn’t throw exceptions.

    Misconception #2: Exception safety requires rollback semantics.

    Misconception #3: RAII has anything to do with rollback semantics.

    Misconception #4: You should always use construct-and-swap when copying objects.

    Nope, all wrong. Personally, when I interview C++ programmers, I won’t employ anyone in a non-junior position who hasn’t at least a passing familiarity with:

    – RAII

    – Levels of safety (nothrow/basic/strong)

    – Rules for destructor invocation when exceptions are thrown in constructors

    All you have to do is print off a document that is *free*, and spend a few days going over it. If you can’t or won’t do that, I certainly don’t want anything to do with your code! If you have read the document, and still prefer error codes to exceptions, that’s at least a position I can have some intellectual respect for.

    The deterministic invocation times of destructors in C++ makes exception handling easy. You just need to follow a few simple rules, the main ones being:

    – get yourself a ref-counted pointer template class (from boost, for instance)

    – acquire resources in constructors

    – hold resources in objects that free them when the destructor is called

    – don’t write low-level methods that *both* mutate an object and access its properties

    – only provide rollback where necessary

    Now, if you prefer error codes to exceptions, then that’s up to you. But please don’t write on which you prefer in a public forum without at least having read what the creator of the C++ language has to say on the subject! It won’t impress anyone who has done some background reading.

  84. Chris Nahr says:

    "Why would you want an icon with, er, no icon?"

    Forget about icons. The textbook phrase "creation is initialization" only applies on a very basic level. That is, all properties have well-defined values, and accessing the object and its properties doesn’t crash the program.

    But those well-defined values are not necessarily the *desired* values. Any non-trivial class will have some properties that often won’t receive the intended value during construction. So you set them before you use the object, and that’s what I meant by "completing initialization".

    The alternative would be extra constructor overloads and parameters for every last rarely used property. That’s hardly desirable.

  85. The "using" one confuses me a little. It’ll work in an OO language where everything we do inbetween returns an object that has a destructor as soon as we leave scope. But how would it work with things like:

    error = allocate_backingtore(object, size);

    that needs to be paired with a free_backingstore(object).

    The try/rollback is clearly understandable to me, but still a little hard to read because it’s using up lots of vertical indentation. In KNF I’d become unreadable really soon.

  86. Actually what I’m trying to do is to make my

    life as little hard as possible. And in C/C++ the error checking + goto method seems to be best to me – it’s more readable than traditional error handling + lots of nested ifdefs which is pretty much the only alternative in C and it’s also a lot easier to understand than C++-style exceptions.

    The C# try/rollback does look like the most promising structured version to me indeed, because it makes very clear what’s happening – in fact the goto approch pretty much models it with more primitive language elements.

    Waleri, of course you can insert the statement at the wrong place, but from years of experience with

    complex C codebases I can say that’s much less unlikely than messing something up in huge conditionals, that often even repeat the same cleanup actions multiple times for slightly different errors.

    Note that the Linux kernel doesn’t recreate lots of C++ concepts, but that’s getting a little offtopic here. Feel free to drop me a mail if you want some insight in the programming concepts used there.

  87. I’m a bit suprised no one brought the goto cleanup style used e.g. about everywhere in the Linux kernel up. I’d look something like (hope the pasting doesn’t screw up the indentation):

    BOOL ComputeChecksum(LPCTSTR pszFile, DWORD* pdwResult)

    {

    BOOL fRc = FALSE;

    HANDLE h, hfm;

    void *pv;

    DWORD dwHeaderSum;

    h = CreateFile(pszFile, GENERIC_READ, FILE_SHARE_READ,

    NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if (h == INVALID_HANDLE_VALUE)

    goto out;

    hfm = CreateFileMapping(h, NULL, PAGE_READ, 0, 0, NULL);

    /* XXX(hch): why are we not checking for INVALID_HANDLE_VALUE here? */

    if (!hfm)

    goto out_close_h;

    pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);

    if (!pv)

    goto out_close_hfm;

    if (!CheckSumMappedFile(pv, GetFileSize(h, NULL),

    &dwHeaderSum, pdwResult))

    goto out_unmap_pv;

    fRc = TRUE;

    out_unmap_pv:

    UnmapViewOfFile(pv);

    out_close_hfm:

    CloseHandle(hfm);

    out_close_h:

    CloseHandle(h);

    out:

    return fRc;

    }

    In my opinion this is a lot better to read than any of the variants above, OTOH religious goto-haters will probably try to kill me now..

  88. George Bailey says:

    No wonder all software has bugs. Presumably the readers of this site are as a group at least slightly above average programmers, and yet almost everybody in this thread has posted incorrect code and/or incorrect interpretations of others’ code. How can it be that good programming is still so difficult?

  89. You’re right that I’m definitely thinking C#-centric. I suppose that the way I’m looking at this is as a question of whether the problem Raymond has identified with exceptions is an inherent problem with exceptions as a concept, or an implementation issue with particular languages that use them.

    My uneducated opinion is that in C++ you have an inherent problem trying to use exceptions, which is that you probably need to call into APIs that don’t use them, which means that whatever constructs you’re using for exception handling can’t be used universally. Unless you want to wrap every external call you make in a wrapper which throws an exception when it gets an error value return. And returns an object with a destructor.

    But I’m choosing to regard that as a side issue in this discussion because it doesn’t apply to the question of whether it’s *possible* to use exceptions in a way that doesn’t have these issues in an ideal world.

    Personally I like the indentation of the "using" and "rollback" cases because it reminds me that I really am entering a meaningful scope inside the block; the indentation reflects the actual semantics of what’s going on. If I start getting too nested I’d probably consider that a good reason to break out some of the inner pieces into a separate function, although I can’t say the situation’s ever actually come up.

    Since we’re already inventing syntax I could envision something like:

    using (Handle handle = CreateFile(…),

    Handle hfm = CreateFileMapping(…),

    Something pv = MapViewOfFile(…)) {

    return CheckSumMappedFile(…);

    }

    (obviously that would look nicer if I could indent it)

    which would act identically to the three nested "using" blocks. I can’t envision anything similar for try-rollback though, because I don’t know where the individual rollbacks would have to be placed.

  90. Waleri says:

    Chirstoph,

    Imagine you now have to insert additional code, that allocata some memory, between opening file and creating map. You’ll have to insert a new label at the end, and what if you insert it in a wrong place? What if you later for some reason move the memory allocation prior mapping the view? You’ll have to move the exit label too. Linux kernel you refer is written in C and it tries to create all C++ concepts, including vtables.

    OK, if you *have to* write in C for some reason, then this approach might works, but yet again, if you use a more powerful language like C++, C#, java, you name it, then why you have to make your live harder yourself?

  91. I wonder whether the C# language team could be convinced that try-rollback would be a worthwhile addition to the language in the next release after Whidbey (I imagine it’s far too late to get anything new in for Whidbey itself)

  92. RAII-like concepts can be used to restore state – the sentry object in iostreams is an example.

    Personally I prefer exceptions (or perhaps a using block – but I don’t use C#) over all other sorts of error handling. Checking error codes just makes me sick. Recently, I haven’t used a function that returns serious problems as error codes without writing a throwing wrapper.

    Java has serious deficiencies here. I have actually seen people making comparisons between Java and C++ (or C# and C++ for that matter) where the lack of a finally block is seen as a problem with C++. Personally, I’d rather write:

    {

    std::auto_ptr<Connection> c = getConnection();

    c->foo();

    }

    trusting the destructor to immediately clean up resources and have the constructor throw on error than do this:

    Connection c = null;

    try {

    c = getConnection();

    c.foo();

    } finally {

    if(c != null) {

    c.close();

    }

    }

    Big problem when handling databases in Java. C#’s using goes a long way towards correcting this problem, but I still prefer stack-based destructor calls.

    I guess it’s a matter of how you grow up. I’ve been aware of exception-safety and RAII-like patterns since my earliest days with C++, so they just come naturally.

  93. I wonder if it really is so hard to write good exception based error code. My own experience is that to allow for an exception at any time simply requires transaction based coding. By that I mean writing code in such a way that changes are finally applied/committed in one line. This makes for very clean code, I think, although it requires a little more thought, and sometimes (much) more overhead/resources; the payoff is a much more robust system and much more pleasure in one’s own source (I’m one of those people who likes to spend a few minutes every now and then admiring my own code).

  94. Paul Spendlove says:

    Sebastian – you obviously know what you’re talking about, and I agree that RAII can be used to restore state, of course it can. My saying that it had "nothing to do" with restoring state was a bit strong. I was reacting to people automatically yoking RAII and state restoration together.

    After all, one could correctly state that "semi-colons can be used to restore state", because the lines of code you write that roll back the transaction have a semi-colon at the end. But that’s not what semi-colons are about – they act as a terminator on a statement.

    Similarly, restoring state is not what RAII is about in a direct sense – it’s about ensuring that objects are valid when they are defined and that they release resources in their destructor.

    I should perhaps have said that "while RAII is a useful tool to help provide rollback semantics, this is not its primary function".

  95. Michael says:

    I think Raymond your statement, that exceptions are generally harder to write/read is based on a somewhat "narrow-view" example.

    I think this is equivalent code:

    if( ! doSome1() ) {//assuming bool returns

    //error handling

    }

    if( ! doSome2() ) {

    //error handling

    //how can it be any more easy to spot

    //missing cleanup for doSome1() effects here?

    //wouldn’t you need ScopeGuard like code here

    //to do it by the RAII principle?

    }

    and

    try{ doSome1(); }

    catch( Exception ) {

    //error handling

    }

    try{ doSome2(); }

    catch( Exception ) {

    //error handling

    }

    What i mean, is you CAN do line by line

    error handling with exceptions as well.

    It might look ugly and be not as efficient,

    but i’m not sure what a C# or C++ would generate for the two cases.

    Unfortunately writing code without try/catch

    that does the same as:

    try{

    doSome1();

    doSome2();

    }catch( … ) {

    is more cumbersome.

    Not always doSome1() has persistent Side-Effects.

    As always there is no one-size-fits-all error handling strategy.

    Sometimes exceptions are better sometimes error codes.

    I think the smartest thing is to think what’s the best strategy for the individual library.

    It would be a pity not to use exceptions due to a fundamentalistic viewpoint.

    Often for low-level libraries exceptions are not used for performance reasons alone, not because they are difficult.

  96. harm says:

    I’d vote for a new exception mechanism :)

    if (code_works(code))

    {

    code();

    }

  97. AndyY says:

    little fix:

    return error_code == 0;

    :))

  98. AndyY says:

    my opinion: goto and error tracing:

    BOOL ComputeChecksum( LPCTSTR pszFile, DWORD* pdwResult )

    {

    int error_code = 1;

    HANDLE h = 0, hfm = 0;

    void *pv = 0;

    DWORD dwSize = 0;

    error_code = 1;

    h = CreateFile(pszFile, GENERIC_READ, FILE_SHARE_READ,

    NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if( h == INVALID_HANDLE_VALUE )

    goto _end;

    error_code = 2;

    hfm = CreateFileMapping(h, NULL, PAGE_READ, 0, 0, NULL);

    if( hfm == INVALID_HANDLE_VALUE )

    goto _end;

    error_code = 3;

    pv = MapViewOfFile(hfm, FILE_MAP_READ, 0, 0, 0);

    if( !pv )

    goto _end;

    error_code = 4;

    dwSize = GetFileSize( h, NULL );

    if( dwSize == INVALID_FILE_SIZE )

    goto _end;

    error_code = 5;

    if( !CheckSumMappedFile( pv, dwSize,

    pdwResult, pdwResult) )

    goto _end;

    error_code = 0;

    _end:

    if( error_code )

    trace( "[-] ComputeChecksum error: %d/%08X", error_code, GetLastError() );

    if( pv ) UnmapViewOfFile(pv);

    if( hfm ) CloseHandle(hfm);

    if( h ) CloseHandle(h);

    return TRUE;

    }

  99. Tal Rotbart says:
    1. All exceptions are checked. Enforce this via compiler options or choice of language.

      2. If someone writes empty catch block– fire him/her. Catch blocks at the very least must log the error/wrap it in an API exception.

      3. Catches should at least deal with the specific thrown exception type, and in 90% of the cases also the generic exception/throwable as a fallback. API functions should always catch the generic exception type (to catch runtime errors) and wrap it in an API exception that must be checked. API functions must always ensure freeing resources allocated by them, see #4.

      4. Always free resources in a ‘finally’ clause. __finally in M$ C++. And structure your code to fit.

      5. Drink Jolt.
  100. Corey Nelson says:

    Chris Nahr mentioned "I don’t see how this issue relates to error codes vs exceptions at all. Using an object before all required init steps have succeeded is always wrong. It’s just as wrong when you’re using error codes." I’d like to expand on his comment.

    Seems to me that if one was going to compare error-codes to exceptions one should do so on the same piece of code. The mistake in the icon code isn’t hard to spot because the code is exception based. One could make the exact same mistake with error-code based code. It would look something like this:

    NotifyIcon CreateNotifyIcon()

    {

    NotifyIcon icon = new NotifyIcon();

    icon.setText("Blah blah blah");

    icon.setVisible(true);

    icon.setIcon(new Icon(GetType(), "cool.ico"));

    return icon;

    }

    It is just as obvious that this code doesn’t check error-codes as it is obvious that Raymonds original example doesn’t check for exceptions. And the mistake of setting the visibility before the icon is just as subtle. Am I wrong?

    Corey Nelson

  101. Raymond Chen says:

    Notice, however, that the correct exception-based code doesn’t catch exceptions either. Therefore, you can’t say "If it doesn’t catch exceptions, it’s wrong." So do you tell the difference between good code and bad code if you can’t rely on the presence of a "catch"?

    I originally had a different example but changed it at the last second. My original example was

    Node AddStep(NodeInfo info)

    {

    Node node = new Node();

    Path.Add(node);

    node.color = info.color;

    return node;

    }

    Observe that the fix to this code does not catch any exceptions. You just have to rearrange the lines of code slightly.

  102. Corey Nelson says:

    Hmm, I’m not so sure that is the correct exception based code. I think it’s important to compare the two systems on the same peice of code. So, may I ask, if this is the correct exception based code:

    NotifyIcon CreateNotifyIcon()

    {

    NotifyIcon icon = new NotifyIcon();

    icon.Text = "Blah blah blah";

    icon.Icon = new Icon(GetType(), "cool.ico");

    icon.Visible = true;

    return icon;

    }

    then what would the correct error-code based code be? Would it be this?:

    NotifyIcon CreateNotifyIcon()

    {

    NotifyIcon icon = new NotifyIcon();

    icon.setText("Blah blah blah");

    icon.setIcon(new Icon(GetType(), "cool.ico"));

    icon.setVisible(true);

    return icon;

    }

  103. Raymond Chen says:

    It would be something like this. (Note: I wouldn’t actually write it this way in real life; I’m just trying to save space.)

    NotifyIcon* CreateNotifyIcon()

    {

    HICON hicon = NULL;

    bool success = false;

    // assuming you’re using a non-throwing new

    NotifyIcon* picon = new NotifyIcon();

    success = picon && picon->SetText("blah blah") && (hicon = LoadIconFromSomewhere("cool.ico")) != NULL && picon->SetIcon(hicon) && picon->SetVisible(true);

    if (!success) { delete picon; picon = NULL; }

    if (hicon) DestroyIcon(hicon);

    return picon;

    }

  104. Corey Nelson says:

    Are you comparing C++ to Java? I think this further obfuscates a fair comparison of the language independent concepts of returning error-codes and throwing exceptions. But since your example of correct error-code based code looks like C++ and your example of correct exception based code takes advantage of garbage collection, I’ll finish this comment with the assumption that you are.

    So wouldn’t this error-code based code:

    NotifyIcon* CreateNotifyIcon()

    {

    HICON hicon = NULL;

    bool success = false;

    // assuming you’re using a non-throwing new

    NotifyIcon* picon = new NotifyIcon();

    success = picon && picon->SetText("blah blah") && (hicon = LoadIconFromSomewhere("cool.ico")) != NULL && picon->SetVisible(true) && picon->SetIcon(hicon);

    if (!success) { delete picon; picon = NULL; }

    if (hicon) DestroyIcon(hicon);

    return picon;

    }

    be just as incorrect as your example of incorrect exception based code? And the mistake just as difficult to spot?

    I believe the point of your article was that a mistake is harder to recognize in exception based code than error-code based code. Perhaps there is a better example to demonstrate your point. A fair example that uses the same language, same series of instructions and the same mistake in both the error-code and exception based versions.

  105. Raymond Chen says:

    Alas, it appears that I failed to make clear my point *again*.

    My point is not that it’s easier/harder to find mistakes in error-based or exception-based code. It’s that it’s harder to **tell the difference** between code where the author thought about errors/exceptions and code where the author didn’t.

    In the code fragment above, the author clearly thought about errors. Perhaps there’s a mistake in it, but at least you know that the author cared.

    With exception-based code, you often can’t tell the difference. Hence the title: "harder to recognize".

Comments are closed.

Skip to main content