Since clean-up functions can’t fail, you have to soldier on


Clean-up functions can't fail, so what do you do if you encounter a failure in your clean-up function?

You have to keep cleaning up.

Some people like to follow this pattern for error checking:

HRESULT Function()
{
  hr = SomeFunction();
  if (FAILED(hr))) goto Exit;

  hr = AnotherFunction();
  if (FAILED(hr))) goto Exit;

  ... and so on until ...

  hr = S_OK;

Exit:
    return hr;
}

And some like to put it inside a cute flow control macro like

#define CHECK_HRESULT(hr) if (FAILED(hr)) goto Exit;

or even

#define CHECK_HRESULT(f) if (FAILED(hr = (f))) goto Exit;

Whatever floats your boat.

But you have to be careful if using this pattern in a clean-up function, because you might end up not actually cleaning up. For example:

HRESULT Widget::Close()
{
    HRESULT hr;
    CHECK_HRESULT(DisconnectDoodad(m_hDoodad));
    m_hDoodad = nullptr;
    for (int i = 0; i < ARRRAYSIZE(GadgetArray); i++) {
        CHECK_HRESULT(DestroyGadget(m_rghGadget[i]));
        m_rghGadget[i] = nullptr;
    }

    hr = S_OK;

Exit:
    return hr;
}

What if there is an error disconnecting the doodad? (Maybe you got RPC_E_SERVER_DIED because the doodad lives on a remote server which crashed.) The cleanup code treats this as an error and skips destroying the gadget. But what can the caller do about this? Nothing, that's what. Eventually you get a bug that says, "On an unreliable network, we leak gadgets like crazy."

Or worse, what if you're doing this in your destructor. You have nowhere to report the error. The caller simply expects that when the object is destroyed, all its resources are released.

So release as much as you can. If something goes wrong with one of them, keep going, because there's still other stuff to clean up.

Related: Never throw an exception from a destructor.

Bonus chatter: Yes, I know that you can avoid this problem by wrapping the Doodad and Gadget handles inside a class which disconnects/destroys on destruction. That's not my point.

Comments (20)
  1. Kevin says:

    Presumably, this applies equally well to the disposal pattern (known as IDisposable/using in C# and various other names in other languages).  The Dispose() method generally shouldn't fail.

  2. David Crowell says:

    If the VB world, this would be the one valid case for On Error Resume Next

  3. Joshua says:

    Ah yes and the real fun one. CloseHandle can return IO errors, but the handle is closed anyway.

    This leads to the thing in Java that leads to most people cursing checked exceptions. Closing an InputStreamReader by static definition can throw IOException but it really can't as the underlying problem only happens when flushing.

  4. Jim Lyon says:

    "Cleanup functions can't fail": Right.

    "you have to soldier on": Wrong.

    When your cleanup function fails for unknown reasons, your program no longer has any idea of what's going on. It should die. If its execution was important, somebody will restart it.

    Of course, when you designed your program, you did plan for sudden, abrupt death, right? After all, you might suffer an I/O error during paging, the cat might trip over the power cord, or any of hundreds of other unfortunate events may occur. One of your design goals should be to ensure that sudden death of your program is an inconvenience, not a disaster.

    [I agree with you. If it's a failure for an unknown reason, you should die immediately, because your state has been corrupted. But if it's a failure like "Could not close the connection to the server because the server crashed", well, that's okay because the server crashed so the connection is already closed. -Raymond]
  5. Jim Lyon says:

    @Joshua: This isn't true. In Windows, CloseHandle either succeeds or returns ERROR_INVALID_HANDLE. No other error is possible. CloseHandle makes no attempt to flush cached I/O, or any other such thing.

    If you ever get an error from CloseHandle, you should die immediately. Why?

    Usually, this means that you tried to close a handle twice. If you do this, 999 times out of 1000, it's perfectly innocuous. But that thousandth time, the handle value has been reused, and you're randomly closing a handle that belongs to another component in your process. This leads to chaos, and an eventual bug filed against a component that is merely an innocent victim of your bug.

  6. Mike Dimmick says:

    @David Crowell: There is no valid use for On Error Resume Next. Ever.

  7. mh says:

    This probably shouldn't have had to be said, but maybe anyone reading this should append an invisible "…unless the alternative is worse".

  8. Joshua says:

    @Jim Lyon: I had a test case that forced it (using a named pipe).

  9. avakar86 says:

    @Joshua: source code or it didn't happen. I am convinced that NtClose can only return STATUS_SUCCESS, STATUS_INVALID_HANDLE or STATUS_HANDLE_NOT_CLOSABLE and CloseHandle will likely only return their win32 equivalents.

  10. I would go further and say that Widget::Close should return void, not HRESULT.

  11. Gabe says:

    Maurits: If Widget::Close returns void, how would you signal an error like "widget already closed", "invalid widget", or "widget not opened yet"? Do you throw exceptions in exception-unsafe code? Do you assume it will never happen? Do you prevent the opportunity to debug double-close bugs?

  12. cheong00 says:

    @Gabe: For the most time, .Close() should be made to be safe to be called repetitively in .NET world. And by calling .Close() you've indicated you no longer need it.

    That's why you should make it returns void. You may provide a property to check the widget's status if you want, but for most of the time the status only have meaning if you attach a debugger to it or trying to reuse the object (by opening it again). In such case you can just let it fail at reopen.

  13. Dave says:

    @Mike Dimmick: If the scope is strictly limited, it's a very useful construct. Suppose you're in an Excel app and you want to clear a range if it exists (your app will create it under some circumstances and not others) to return the sheet to a known state. What's wrong with:

    On Error Resume Next

    shWork.Range("InterestingRange").Clear

    On Error Goto UsualErrorHandler

    as opposed to trying to figure out if the name exists, then trying to ascertain if it corresponds to a range, etc, before finally clearing it? Please explain what you don't like about this.

  14. Medinoc says:

    >The Dispose() method generally shouldn't fail.

    This, very much. And especially not by throwing a CommunicationObjectFaultedException.

  15. > how would you signal an error like "widget already closed", "invalid widget", or "widget not opened yet"

    Closing an already-closed widget may not be an error at all, if you adopt the convention that closing is an idempotent operation.

    You could signal the other errors by asserting or throwing an exception, if you were so inclined.

    I have noticed, though, that cleanup routines which return HRESULTs are usually called by clients that just drop the return value on the floor.

  16. alegr1 says:

    @Joshua:

    FlushFileBuffers

  17. John Doe says:

    @Dave, that's a recipe for unmaintainable and unreliable code.  It's prettier and tidier, but I much prefer to (actually, I just do) either let the errors pop or check the arguments myself to do optional stuff like that.

    Think about it, that's just try { … } catch (Exception e) {}.  I never, ever want that, specially for an expression with two dots, a complex indexed property access and a method call.

  18. immibis says:

    @Dave: There are other errors that could occur. What if the worksheet is protected? If you had explicit checks, you'd crash if the worksheet is protected (as intended) but without them you continue as if the range didn't exist. (Result: Protecting a worksheet can corrupts data)

    But it's a tradeoff between development speed and reliability. VBA heavily favours the former.

  19. DaveK says:

    >"Yes, I know that you can avoid this problem by wrapping the Doodad and Gadget handles inside a class which disconnects/destroys on destruction. That's not my point."

    It kind of is, actually, isn't it? I mean, your point is that you should structure your code so that everything gets cleaned up even in the face of failures during earlier clean-ups, and that would be one way of doing so. Did you perhaps mean something like "Discussing the merits or otherwise of specific ways of avoiding this problem is not my point"?

    [I'm discussing the more general issue, particularly if the language you're using doesn't support destructors (for example, if you're designing a C API) or if the lifetimes of the Doodad and Gadget objects are not function-scope. (For example, you might have "Connect" and "Disconnect" methods.) -Raymond]
  20. cheong00 says:

    > I have noticed, though, that cleanup routines which return HRESULTs are usually called by clients that just drop the return value on the floor.

    Btw, I've seen people drops return values that should never be dropping on the floor like ReadFile() on a file transfer routine.

Comments are closed.