Does (possibly) failable destructor require to change invariant violation error handling?

Question

First, I'd like to highlight my personal premise on error handling approaches in C++ (as it might be the source of confusion in the first place). C++ Standards, 2004 (Sutter, Aleksandrescu), item 68 states as follows:

Use assert or an equivalent liberally to document assumptions internal to a module (i.e., where the caller and callee are maintained by the same person or team) that must always be true and otherwise represent programming errors (e.g., violations of a function's postconditions detected by the caller of the function)

The item 69 of the same book then discusses that for other errors you should instead employ different error handling strategies:

From this Item onward, this section focuses on dealing with run-time errors that are not due to faulty coding internal to a module or subsystem. (As Item 68 covers separately, prefer to use assertions to flag internal programming errors, ones that are just outright coding errors on some programmer's part.)

Finally, the item 72 explicitly recommends preferring exceptions as a default error-handling strategy:

When harmed, take exception: Prefer using exceptions over error codes to report errors. Use status codes (e.g., return codes, errno) for errors when exceptions cannot be used (see Item 62), and for conditions that are not errors. Use other methods, such as graceful or ungraceful termination, when recovery is impossible or not required.

---

Now imagine that there is a simple RAII wrapper class. The invariant it maintains is the state of the pointer passed, i.e. it's not nullptr (let's ignore other invalid pointer values for simplicity):

class IntHandle {
private:
 int* m_ptr;
public:
 IntHandle(int* ptr) : m_ptr{ ptr }
 {
 if (!m_ptr) {
 throw std::runtime_error{ "The pointer cannot be null" };
 }
 }
 void Increment() noexcept {
 assert(m_ptr);
 ++(*m_ptr);
 }
 operator int() const noexcept {
 assert(m_ptr);
 return *m_ptr;
 }
 ~IntHandle()
 {
 assert(m_ptr);
 delete m_ptr;
 }
};

The constructor checks the pointer passed for nullptr and assumes it's legal (by contract). If nullptr precondition is violated, the client is informed about it with an exception. Other functions, however, don't throw any exceptions, because the class doesn't provide the client code with facilities to invalidate the class invariant in a common-case scenario, so such mistakes are considered programming errors and checked by assert. In reality, however, you often deal with resources where destruction may fail. E.g. assume that instead of int I need to wrap a pointer to a 3'rd-party library object of class Foo with the same semantic, but which is destroyed by call to a specific function with a possibility to fail:

class FooHandle {
private:
 Foo* m_ptr;
public:
 FooHandle(Foo* ptr) : m_ptr{ ptr }
 {
 if (!m_ptr) {
 throw std::runtime_error{ "The pointer cannot be null" };
 }
 }
 void Increment() {
 assert(m_ptr);
 ++(*m_ptr);
 }
 operator Foo() const {
 assert(m_ptr);
 return *m_ptr;
 }
 ~FooHandle()
 {
 assert(m_ptr);
 if (FooDestroy(m_ptr) != 0) {
 // report failure
 }
 }
};

For the client code this is not convenient, because it doesn't have means to gracefully handle the destruction failure scenario, and conventional wisdom says me that the client code needs to be provided with a separate function to free the resource for that:

void Destroy() {
 assert(m_ptr);
 if (FooDestroy(m_ptr) != 0) {
 throw std::runtime_error{ "Failed to destroy the Foo object" };
 }
 m_ptr = nullptr;
}
~FooHandle()
{
 if (!m_ptr) {
 return;
 }
 try {
 Destroy();
 } catch(const std::exception& e) {
 // report error
 }
}

This, however, exposes new facilities for the client code to alter the class invariant, so does it mean that all assert now needs be replaced with exceptions?

• freakish: How can you gracefully handle destruction failure? I find it odd that anyone would create a Destroy() function that can fail, that seems to be the root problem here.
• gnasher729: It looks to me like a nullptr is a programming error that should assert.
• freakish: @AleksandrMedvedev you've linked close syscall. If you read the docs cloesly, you will quickly realize that there's literally nothing you can do with the returned error, except for reporting it (which is not what I would call a graceful handling). From that point of view, whether the call fails or not is irrelevant, since you cannot do anything after making it. You cannot retry it (at least on the same fd).
• Aleksandr Medvedev: @freakish depending on platform (e.g. in HP-UX) you may want to call close multiple times in case it fails under specific circumstances. But even if I ignore this part, logging errors is not quite responsibility of the RAII wrapper to my taste and silently failing in the destructor doesn't let the client code to know the problem happened
• freakish: @AleksandrMedvedev yes, some OS will allow multiple close calls on the same fd. But some won't. That is not portable, and writing code per specific OS is a maintanance nightmare (unless you specifically target only one OS for whatever reason). I strongly discourage you from going down that road, do it as a last resort. Plus there is always a risk that some other thread took ownership of the fd in the meantime, and now you called close on fd that you were not supposed to. Also, I don't see anything wrong with RAII wrapper doing logging. In fact I would say it is necessary.

Answer 1

Ideally, a third party library which provides a method FooDestroy(...) provides also a method to check whether the object was already destroyed, like IsFooDestroyed(...). That will allow to keep the class invariant m_ptr!=nullptr. Any method which is incompatible with a "destroyed" m_ptr needs to look like this :

void Increment() {
 assert(m_ptr);
 if(IsFooDestroyed(m_ptr)) {
 throw std::runtime_error{ "Foo object already destroyed" };
 // or - do nothing - just return, whatever fits best here
 }
 ++(*m_ptr);
}

(you may want to make this thread-safe, but I leave this out for the sake of simplicity, I guess you get the idea.)

Note in your FooHandle example, the wrapper never takes the ownership of the 3rd party pointer, it does not construct the Foo pointer, nor does it delete it. Hence, there could be two or more FooHandle objects sharing the same Foo pointer. When one calls FooDestroy, the others would not get informed about it - just setting m_ptr to nullptr would only inform "ones own" object about the FooDestroy call, but not the others.

In short, it is a bad idea to mix up the call to "FooDestroy" with setting m_ptr to nullptr - better maintain the status independently and outside of any FooHandle object. The Destroymethod should look like this:

void Destroy() {
 assert(m_ptr);
 if (IsFooDestroyed(m_ptr)) {
 return;
 }
 if (FooDestroy(m_ptr) != 0) {
 throw std::runtime_error{ "Failed to destroy the Foo object" };
 }
 // no nullptr assignment here!
}

• Aleksandr Medvedev: I agree with you that destruction and nullification are two different statuses to be checked, however do you mean that now every function (not counting constructors) is supposed to have two checks (one, which throws an exception in case of call-after-destruction, and another is assert for null)?
• Doc Brown: @AleksandrMedvedev: we can only speculate what one has to do for a hypothetical library with a hypothetical wrapper. Are we wrapping 5 function or 50, or 500? Maybe half of the functions don't require a check for the after-destruction state. For example, is ++(*m_ptr) really a problem in "after-destruction" state? You can also bundle an assert statement with IsFooDestroyed in a helper function, so omit assert in most places.
• Basilevs: IsSomethingDestroyed is not an expected or accepted way to manage resources when memory is manually managed. This will cause endless use after free errors. This is unacceptable in C++, may be considered in Java, but even there operation of staus check and update are usually atomic/combined in a single critical section.
• Doc Brown: @Basilevs: but you understood the OP spoke about some C library which does not follow your idea of how a nicely designed library should work? Possibly the reason why they create a wrapper?
• Basilevs: @DocBrown OP has described a nice library. It provides encapsulated constructor and disposer as expected. Your receipe encourages malpractice, not OPs thirdparty lib.

Answer 2

If you want to write reliable C++ code, then there is one rule that you must always obey: a destructor must not throw or allow an exception to pass out of it. The reason is that there cannot be multiple exceptions trying to unwind the stack at the same time. If that happens, the application will be terminated.

The implication of this rule is that if you have to perform a potentially fallible operation during the cleanup in your destructor, you are more limited to how you can respond to failures in that operation.

Instead of reporting the failure to the calling code and letting them deal with it, you will have to deal with it yourself. If the failure is especially serious, this can mean terminating the application, but it is more likely that the appropriate reaction is to log the failure for future (offline) analysis and continue as-if nothing happened.

---

If you decide to modify your resource-handling class to expose a cleanup operation to the user, then you have expanded the valid states of the class with a "cleaned-up" state and you must update all existing methods to properly deal with that new state (even if that is testing for the state and throwing an exception that the method cannot be used in that state).

• Basilevs: Why the first part? OP went to great lengths to explicitly describe how they prevent exceptions in destructor. The whole reason for the question is to avoid throwing from destructor.
• Andrew Henle: @Basilevs There's no reason not to say it again, though.
• Andrew Henle: Instead of reporting the failure to the calling code and letting them deal with it, you will have to deal with it yourself. I'd say "In addition to..." As worded, that sounds like reporting the failure and handling it are mutually exclusive.
• Bart van Ingen Schenau: @AndrewHenle, yes they are mutually exclusive (at least in the way I used 'reporting' and 'handling'). Reporting a failure means that you cannot perform the job you were asked to do, but handling the failure means that you did find a way to do the job, in spite of there being a failure of some kind along the way. Reporting explicitly does not include purely informational messages. When a failure has been handled, there is nothing left for the calling code to do, so there is nothing to report to them.

Answer 3

Yes, assertions can not be used to track a state exposed to clients. Replace all assertions that verify user-modifiable state with exceptions. Do not expose the state otherwise, prefer idempotent disposal.

_{As @GregBurghardt correctly noticed, construction and destruction should be done in the same context, make sure not to pass around a pointer to a third-party resource without associated disposer.}

class FooHandle {
private:
 Foo* m_ptr;
public:
 FooHandle(Arguments arguments) : m_ptr{ ptr }
 {
 m_ptr = FooCreate(arguments); // ensure that resource creation and disposal share component, do not split them
 assert(m_ptr); // Replace with an exception, if user can provide invalid Arguments and break FooCreate
 }
 void Increment() {
 EnsureOpened();
 ++(*m_ptr);
 }
 operator Foo() const {
 EnsureOpened();
 return *m_ptr;
 }
 void Destroy() {
 if (m_ptr && FooDestroy(m_ptr) != 0) { // Do not require clients to track our internal state, make the API idempotent instead
 throw std::runtime_error{ "Failed to destroy the Foo object" };
 }
 m_ptr = nullptr;
 }
 ~FooHandle() {
 try {
 Destroy();
 } catch(const std::runtime_error & e ) {
 // report failure
 }
 }
private:
 void EnsureOpened() {
 if (!m_ptr) {
 throw std::logic_error("The resource is destroyed");
 }
 }
};

• Basilevs: @GregBurghardt could you please review this from the perspective of your deleted answer?