RAII wrapper for SQLite transactions

Question

I'm using this nice C++ wrapper to work with SQLite3 in my project. I wanted to handle errors gracefully and automatically rollback if piece of code fails, so wrote this simple helper class:

class SqlTransaction { // RAII class to rollback on error
    public:
        SqlTransaction(std::shared_ptr<sqlite::database> db_): db(db_) { *db << "begin;"; };
        ~SqlTransaction() { if (!finished) rollback(); };
        void commit() { if (!finished) { *db << "commit;"; finished = true; } };
        void rollback() { if (!finished) { *db << "rollback;"; finished = true; } };

        // Disable both copying and moving
        SqlTransaction(const SqlTransaction&) = delete;
        SqlTransaction& operator=(const SqlTransaction&) = delete;
        SqlTransaction(SqlTransaction&&) = delete;

    private:
        std::shared_ptr<sqlite::database> db;
        bool finished = false;
    };

It's to be used like this:

SqlTransaction trans(db);
// work with db here, may throw if something goes wrong
trans.commit();

Answer 1

There's not a lot of code here to be reviewed, but I'll have a go.

• A small efficiency gain is possible, by moving the db_ argument in the initializer list, rather than copying it:

  SqlTransaction(std::shared_ptr<sqlite::database> db_)
    : db{std::move(db_)}
  { *db << "begin;"; };
  

• It's not necessary to delete the move constructor, as explicitly deleting the copy constructor prevents the move constructor being implicitly provided. However, if you feel that doing so improves clarity, you should probably delete move assignment, too.

• We should document that the class isn't thread-safe. It's quite reasonable that we should use it from only one thread, but we need to be clear to our users about that. Alternatively, we could make it thread-safe by making finished an std::atomic<bool> and using its test-and-set method, exchange().

• Since rollback() tests finished, there's no need to duplicate that in the destructor - just call rollback() unconditionally. It may be worth taking steps to avoid it throwing when called there - destructors that throw need to be handled with extreme care.

  ~SqlTransaction() { try { rollback(); } catch (...) { /* ignore */ } };
  

I have nothing specific to SQLite, as I've not used that library myself (I'm surprised it has to parse string commands, rather than having methods for those operations, though).

Answer 2

It's a good idea to encapsulate transaction-management.

You don't go anywhere far enough though. Use std::uncaught_exceptions() to automate it more. (Before C++17, you have to use some non-standard ways to get that count.)

Also, while rolling back must never fail, committing may. So, mark noexcept and noexcept(false) as appropriate.

Last but not least, while copying a std::shared_ptr is not really expensive, moving it is much cheaper still.

class SqlTransaction {
public:
    SqlTransaction(std::shared_ptr<sqlite::database> db_)
    : db(std::move(db_))
    { *db << "begin;"; }
    ~SqlTransaction() noexcept(false) {
        auto current = std::uncaught_exceptions();
        if (count == current)
            do_commit();
        else if (count < current)
            do_rollback();          
    }
    void commit() {
        if (std::exchange(count, max_count) != max_count)
            do_commit();
    }
    void rollback() noexcept {
        if (std::exchange(count, max_count) != max_count)
            do_rollback();
    }
private:
    void do_commit() {
        *db << "commit;";
    }
    void do_rollback() noexcept {
        *db << "rollback;";
    }

    SqlTransaction(const SqlTransaction&) = delete;
    SqlTransaction& operator=(const SqlTransaction&) = delete;

    std::shared_ptr<sqlite::database> db;
    int count = std::uncaught_exceptions();
    static constexpr max_count = std::numeric_limits<int>::max();
};

Answer 3

Throw an exception on invalid use

You have these two functions:

void commit() { if (!finished) { *db << "commit;"; finished = true; } };
void rollback() { if (!finished) { *db << "rollback;"; finished = true; } };

These allow me to do write this code:

SqlTransaction tx(db);
db << "some query...";
tx.commit();
tx.rollback();

Should the end result be committed or rolled back? The call to rollback() is not doing anything here, but if I wrote it in the code I probably really meant for the transaction to roll back. So this is without doubt a programming error. The same goes for calling commit() after a rollback(), and calling commit() or rollback() multiple times for the same transaction is probably also bad. So you should throw an exception in these cases, either a std::logic_error or a custom exception derived from it:

void commit() {
    if (finished)
        throw std::logic_error("commit() called on finished transaction");
    *db << "commit;";
    finished = true;
}

void rollback() {
    if (finished)
        throw std::logic_error("rollback() called on finished transaction");
    *db << "rollback;";
    finished = true;
}

You might also consider throwing an exception in the destructor if there was no explicit commit() or rollback().