7

Consider the following C API:

void BeginTransaction(State *s);
void AddToTransaction(State *s, Object *value);
void CommitTransaction(State *s);

void Foo(State *s, Object *value)
{
    BeginTransaction(s);
    AddToTransaction(s, value);
    CommitTransaction(s);
}

I created a C++ class to wrap this API:

class StateTransaction
{
public:
    StateTransaction(State *s) : state_(s)
    {
        BeginTransaction(state_);
    }

    StateTransaction(const StateTransaction &st) = delete;

    void Add(Object *value)
    {
        AddToTransaction(state_, value);
    }

    ~StateTransaction()
    {
        CommitTransaction(state_);
    }

private:
    State *state_;
}

void Foo(State *s, Object *value)
{
    StateTransaction st(s);
    st.Add(value);
    // When st goes out of scope, the transaction automatically gets committed
}

Assuming that none of the functions can fail, is this a good use case for RAII? My main problem with it right now is that in order to force the transaction to be committed before some other code, I need to put the object in a new scope:

void Foo(State *s, Object *value)
{
    {
        StateTransaction st(s);
        st.Add(value1);
    }

    // Code which relies on transaction being completed
}

And of course, there's the danger that someone will come along and delete the braces because they look useless. That could be fixed with a comment like /* DO NOT DELETE BRACES */, but at that point it seems safer to just use the C API.

So, is the RAII wrapper a good idea? Or should I stick to the C style?

gnat
  • 21,442
  • 29
  • 112
  • 288
Andrew Sun
  • 251
  • 2
  • 9
  • The comment should not be `/* DO NOT DELETE BRACES */`. That doesn't tell the other guy why. The comment should be something like `// Braces create new scope for transaction`, right above the first brace. It's a good idea if it accomplishes your purpose in a reliable and reasonably elegant way. – Robert Harvey Dec 12 '16 at 17:09
  • @RobertHarvey Well of course I wouldn't write it like that in real code ;-p I just meant that I feel the version with the comment and braces is more complex than the non-RAII version (it's immediately obvious what `BeginTransaction()` and `CommitTransaction()` do) – Andrew Sun Dec 12 '16 at 17:14
  • 2
    I thought RAII was for memory management. Does the memory management strategy differ in the two cases? I'm not sure calling this RAII is accurate. – Robert Harvey Dec 12 '16 at 17:19
  • @RobertHarvey I considered the transaction itself as a "resource", which is released (committed) when it goes out of scope. – Andrew Sun Dec 12 '16 at 17:23
  • Then your choices are to dispose of that resource immediately, or wait until the entire object goes out of scope. Which one would you say is preferable? – Robert Harvey Dec 12 '16 at 17:25
  • 1
    In my mind, **transactions** are involved with the notion that the calling client may choose to abort/cancel the transaction vs. commit, and also that a commit may fail for other reasons (conflict with other transaction), thus requiring the client to retry or do something else. I don't see any of that here in C or in C++. So, maybe these aren't the kind of transactions I'm thinking of? – Erik Eidt Dec 12 '16 at 17:33
  • @ErikEidt I suppose the name isn't that accurate. I was thinking more along the lines of push/pop matrix operations in OpenGL, which should never fail/be aborted. – Andrew Sun Dec 12 '16 at 17:44

4 Answers4

13

In general using RAII to implement transactions is a good idea. However, you have to be careful when doing anything in the destructor.

  • A destructor cannot return a value indicating success/failure.
  • The destructor really should not throw exceptions, as the destructor may be invoked during exception handling.

This means we cannot really use destructors to represent operations that can fail.

Because the destructor will be invoked during normal control flow and during exception handling, it could happen that the transaction is not in fact complete:

StateTransaction transaction;
transaction.Add(x);
throw "oops";
transaction.Add(y);

Your current design would happily commit the "x" value, and miss the "y" value. This isn't a very transactional transaction, if the transaction may be incomplete.

Destructor-based transactions are more useful if they are implemented as rollbacks – the destructor will roll back the transaction unless it is explicitly committed. Implicit committing doesn't quite work.

To auto-commit only when no exception occurred, use normal control flow. If you want to avoid the possibility that the control flow exited normally but the programmer forgot to call Commit, then a callback-based solution would work (C++11):

template<class TransactionBuilder>
void transaction(TransactionBuilder callback) {
  State transaction;
  BeginTransaction(&transaction);
  callback(&transaction);
  CommitTransaction(&transaction);
}

transaction([&](State* transaction) {
  AddToTransaction(transaction, x);
  throw "oops";
  AddToTransaction(transaction, y);
});
amon
  • 132,749
  • 27
  • 279
  • 375
4

Using BeginTransaction and EndTransaction directly can give different results that using RAII.

If we compare:

BeginTransaction(s);
AddToTransaction(s, value);
CommitTransaction(s);

To:

{
    StateTransaction st(s);
    st.Add(value1);
}

There's a significant difference if the step of adding something to the transaction throws an exception. In the first case, throwing means execution in that scope will stop, resources cleaned up, and restart at the nearest matching exception handler.

In the RAII case, since the transaction is a resource that was created in that scope, it will be destroyed before exiting from that scope.

So, if the first AddToTransaction throws an exception, the transaction will be left hanging open, but in the second, it will be committed.

This probably doesn't make much difference if you only add a single item to the transaction, but consider a transaction in which we add several items:

BeginTransaction(s);
AddToTransaction(s, value1);
AddToTransaction(s, value2);
AddToTransaction(s, value3);
AddToTransaction(s, value4);
CommitTransaction(s);

Now, what happens if (for example) adding value 4 to the transaction throws? We haven't done anything to close the transaction--we have an open transaction with values 1-3 just "hanging".

In this case, we probably want to abort the transaction, not commit it though. So the dtor should probably look something like this:

~StateTransaction()
{
    if (std::uncaught_exceptions() > 0)
        AbortTransaction(state);
    else
        CommitTransaction(state_);
}

This way we know the transaction will always be closed--but it has at least a little intelligence about whether to abort or commit the transaction.

If we write code to do the same using CommitTransaction/AbortTransaction directly, it ends up pretty messy. We typically end up with something like:

try {
    BeginTransaction()
    AddToTransaction()
    AddToTransaction()
    CommitTransaction();    
}
catch (...) {
    AbortTransaction();
}

If we try to handle this without exceptions at all, life gets even worse in a hurry. We typically end up with either deep nesting:

if (BeginTransaction())
    if (AddToTransaction())
        if (AddToTransaction())
            CommitTransaction();
        else
            AbortTransaction();
    else
        AbortTransaction();

I think a block without an obvious reason for existence is (by far) the cleanest solution that actually works.

Jerry Coffin
  • 44,385
  • 5
  • 89
  • 162
  • 1
    When checking `std::uncaught_exceptions()` in a destructor, store the value of `std::uncaught_exceptions()` in the constructor and compare to that value, so that StateTransaction can be used (indirectly) in cleanup code – Caleth Dec 13 '16 at 13:57
0

As to your actual problem, assuming that "CommitTransaction" is poorly named and must be called even in exception cases (per your comments) and your issue is with the scoping of RAII, I see two choices.

Option 1: Comment the scope appropriately. C#-like syntax (C# has "using" for its RAII), or simply a message about the scope. Stylistic choice, just be consistent.

void Foo(State* s, Object *value)
{
    //using(StateTransaction st) 
    {
        StateTransaction st(s);
        st.Add(value);
    }
    // Code which relies on transaction being completed
}

OR

void Foo(State* s, Object *value)
{
    //scope for StateTransaction st
    {
        StateTransaction st(s);
        st.Add(value);
    }
    // Code which relies on transaction being completed
}

Option 2: Refactor your scope into a private method/additional global function.

void FooAddValue(State* s, Object *value)
{
    StateTransaction st(s);
    st.Add(value);
}
void Foo(State* s, Object *value)
{
    FooAddValue(s, value);
    // Code which relies on transaction being completed
}

I prefer option 2. The fact that you feel you need this sub-scoping indicates that your function has multiple responsibilities and is thus violating the SOLID principle of Single Responsibility and should be broken up into multiple functions.

Kevin Fee
  • 2,837
  • 1
  • 10
  • 14
0

Is the RAII wrapper a good idea for this C transactions API or should I stick to the C style?

Consider a RAII wrapper that rolls back the transaction in destructor, with explicit commit (note: I changed the API to use references:

class StateTransaction
{
public:
    StateTransaction(State &state) : state_(s), active_{ true }
    {
        BeginTransaction(&state_);
    }

    ~StateTransaction()
    {
        if(active_)
        {
            RollbackTransaction(&state_); // you didn;t mention this API,
                                         // but your C transactional 
                                         // layer should have something like this
        }
    }

    void Commit() noexcept
    {
        active_ = false;
        CommitTransaction(&state_);
    }

    void Add(Object &value) { ... }

private:
    bool active_;
    State &state_;
};

Client code:

void Foo(State &s, Object &value)
{
    StateTransaction transaction{ s };
    transaction.Add(value);
    // ... may have multiple steps here
    transaction.Commit();
}

If anything throws, (in the multiple steps of the transaction), the transaction.Commit() will not be reached, and the transaction will be automatically rolled back on destruction.

Regarding the artificial scope (and someone removing it by mistake), simply name it and document the reason (instead of documenting what your colleagues should do with it):

void FooTransaction(State & s, Object & value) // name tells you it's a transaction (and
                                             // supposed to be one)
                                             // if it doesn't add a comment about it
{
    StateTransaction transaction{ s };
    transaction.Add(value);
    // ... may have multiple steps here
    transaction.Commit();
}

That said, a RAII wrapper may not be the best top-level abstraction to use. Consider adding the following API on top of it:

void transact(State& s, std::vector<Object*> values)
{
    StateTransaction transaction{ s };
    for(auto& x: values)
        transaction.Add(*x);
    transaction.Commit();
}

Client code:

transact(&state, { value });

This makes it clear what you are doing and that all effects (such as the transaction being committed) have been finished when the function has finished executing.

utnapistim
  • 5,285
  • 16
  • 25