CODEWITHSUNDEEP
c++foreachexception
Christophe Fuzier
Christophe Fuzier

Reputation: 759

Exception-safe for loop

Consider the following code.

#include <cassert>
#include <stdexcept>
#include <vector>

struct Item {
    Item() : m_enabled(false) {}

    void enable()
    {
        m_enabled = true;
        notify_observers(); // can throw
    }
    bool enabled() const {return m_enabled;}

private:
    bool m_enabled;
};

struct ItemsContainer {
    ItemsContainer() : m_enabled(false) {}

    void enable()
    {
        bool error = false;
        for(Item & item : m_items) {
            try {
                item.enable();
            } catch(...) {
                error = true;
            }
        }
        m_enabled = true;
        if(error) throw std::runtime_error("Failed to enable all items.");
    }
    bool enabled() const
    {
        for(Item const & item : m_items) assert(item.enabled() == m_enabled);
        return m_enabled;
    }

private:
    std::vector<Item> m_items;
    bool m_enabled;
};

In my situation, Item is implemented as shown (I cannot change it) and I am trying to implement ItemsContainer. I don't exactly know how to deal with exceptions.

  1. In the proposed implementation, when enabling the container, we enable all the items even if notifying the observers of one of them throws, and in the end, we potentially throw an exception to notify the caller that there was a problem with (at least) one of the observers. However the state of the container is modified. Is it counter-intuitive? Should I try to give the strong exception guarantee to ItemsContainer::enable by disabling the already enabled items in case of failure and keeping initial state unchanged?

  2. In ItemsContainer::enabled, are the assertions reasonable? Since I don't have the control on Item and no exception guarantee is documented, it could occur that the instructions m_enabled = true; and notify_observers(); are swapped. In this case, ItemsContainer::enable could break the internal state of the container.

It is not the first time I face such a situation. Are there best practice rules?

NB: I reduced the code sample to the minimum, but in fact, Item::enable is a setter: Item::set_enabled(bool) and I want to implement the same thing for ItemsContainer. This means that items can be disabled if enabling fails, but here again, without specification of the exception guarantee.

Upvotes: 2

Views: 168

Answers (2)

Sebastian Redl
Sebastian Redl

Reputation: 72054

If the library you depend on does not give you behavior guarantees in exceptional cases, then you cannot give dependent behavior guarantees. If it gives you the basic guarantee, you can work with that to a point, but potentially at very high cost.

In your case, for example, you want to maintain the invariant that either all items are enable, or none are. So your options are

  1. If any enable function throws an exception, keep going. This only works if you know the state of the item after an exception, but you said this isn't documented, so this option is out. Even if it was documented, can you be sure that the observers are prepared to deal with this? What does it mean if the first of many observers of a single item throws - do the other observers get called? Does your entire application go into an invalid state if an item is enabled but the observers never knew?
  2. When an exception is thrown, go back through the enabled items and disable them again. But is this possible without throwing an exception again? Do the observers need to be informed of this additional change? And couldn't that in turn yield another exception?
  3. Do something like Dennis's setEnabledWithTransaction. Is Item copyable? What does it mean for the observers if an item is copied, but the copy then discarded, or the old object modified?
  4. Maintain just the basic exception guarantee and clear the entire vector when an exception is thrown. This may seem radical, but if you can't find a way to make the above options work, this is the only way to maintain your class invariant that either all items are enabled or none are. If you don't know the state of your items and you can't safely modify it, then you can only throw them away.
  5. Don't give any exception guarantee and leave the container in an inconsistent state. This is of course a very bad option. Operations that don't at least give the basic exception guarantee have no place in a system that uses exceptions.

Upvotes: 2

Dennis
Dennis

Reputation: 3731

Well this seems more like an application choice than an API choice. Depending on your application you may want to do any of:

  1. Fail immediately, leaving all Items in their current state
  2. Reset all Items to previous state (transaction type protection) on failure then throw
  3. Make a "best attempt" to set all.

If you think that your users may want more than one of those then you could easily design it into the API:

bool setEnabled(bool enable, bool failFast = false)
{
    bool error = false;
    for(Item & item : m_items) {
        try {
            enable ? item.enable() : item.disable();
        } catch(...) {
            error = true;
            if(failFast)
                 throw std::runtime_error("Failed to enable all items.");
        }
    }
    m_enabled = true;
    return error;
}

bool setEnabledWithTransaction(bool enable)
{
    bool error = false;
    vector<Item> clone(m_items);
    for(Item & item : m_items) {
        try {
            enable ? item.enable() : item.disable();
        } catch(...) {
           // use the saved state (clone) to revert to pre-operation state.
        }
    }
    m_enabled = true;
    return error;
}

Note that the bool return value indicated success. Alternatively you could return the number of successfully enabled items. This is what you should do if the chance of a failure is non-trivial. If the chance of a failure is very very small, or if a failure indicates something that is a system failure then you should throw.

Upvotes: 1

Related Questions