Add 'commit' method to kv transaction

[Bug/Feature]   Do no rely on std::uncaught_exception for two reasons:
                - there is a bug in gcc with rethrow_exception
                - it wasn't possible to update db during stack
                  unwinding.
[Cause]         N/A
[Solution]      N/A
[Verification]  Build, install, run tests

Change-Id: I8e12688efdebb3f2865e6a4512ed331756c69537

diff --git a/src/config/from-kvjson-visitor.hpp b/src/config/from-kvjson-visitor.hpp
index 2624ad3..6b2f6a6 100644 (file)
--- a/src/config/from-kvjson-visitor.hpp
+++ b/src/config/from-kvjson-visitor.hpp
@@ -34,11 +34,10 @@ namespace config {
 
 class FromKVJsonVisitor {
 public:
-    FromKVJsonVisitor(const std::string& db, const std::string& json, const std::string& prefix)
-        : mStorePtr(new KVStore(db))
+    FromKVJsonVisitor(KVStore& store, const std::string& json, const std::string& prefix)
+        : mStore(store)
         , mKeyPrefix(prefix)
         , mIsUnion(false)
-        , mTransaction(mStorePtr->getTransaction())
     {
         mObject = json_tokener_parse(json.c_str());
         if (mObject == nullptr) {
@@ -53,10 +52,9 @@ public:
 
     FromKVJsonVisitor(const FromKVJsonVisitor& v) :
         mObject(v.mObject ? json_object_get(v.mObject) : nullptr),
-        mStorePtr(v.mStorePtr),
+        mStore(v.mStore),
         mKeyPrefix(v.mKeyPrefix),
-        mIsUnion(v.mIsUnion),
-        mTransaction(v.mTransaction)
+        mIsUnion(v.mIsUnion)
     {
     }
     FromKVJsonVisitor& operator=(const FromKVJsonVisitor&) = delete;
@@ -67,18 +65,16 @@ public:
     }
 
 private:
-    std::shared_ptr<KVStore> mStorePtr;
+    KVStore& mStore;
     std::string mKeyPrefix;
     json_object* mObject;
     bool mIsUnion;
-    KVStore::Transaction mTransaction;
 
     // create visitor for field object (visitable object)
     FromKVJsonVisitor(const FromKVJsonVisitor& v, const std::string& name, const bool isUnion) :
-        mStorePtr(v.mStorePtr),
+        mStore(v.mStore),
         mKeyPrefix(key(v.mKeyPrefix, name)),
-        mIsUnion(isUnion || v.mIsUnion),
-        mTransaction(v.mTransaction)
+        mIsUnion(isUnion || v.mIsUnion)
     {
         json_object* object = nullptr;
         if (v.mObject && !json_object_object_get_ex(v.mObject, name.c_str(), &object)) {
@@ -90,10 +86,9 @@ private:
 
     // create visitor for vector i-th element (visitable object)
     FromKVJsonVisitor(const FromKVJsonVisitor& v, int i, const bool isUnion) :
-        mStorePtr(v.mStorePtr),
+        mStore(v.mStore),
         mKeyPrefix(key(v.mKeyPrefix, std::to_string(i))),
-        mIsUnion(isUnion || v.mIsUnion),
-        mTransaction(v.mTransaction)
+        mIsUnion(isUnion || v.mIsUnion)
     {
         json_object* object = nullptr;
         if (v.mObject) {
@@ -107,8 +102,8 @@ private:
     void getValue(const std::string& name, T& t)
     {
         std::string k = key(mKeyPrefix, name);
-        if (mStorePtr->exists(k)) {
-            t = mStorePtr->get<T>(k);
+        if (mStore.exists(k)) {
+            t = mStore.get<T>(k);
         }
         else {
             json_object* object = nullptr;
@@ -138,8 +133,8 @@ private:
 
     int getArraySize(std::string& name, json_object* object)
     {
-        if (mStorePtr->exists(name)) {
-            return mStorePtr->get<int>(name);
+        if (mStore.exists(name)) {
+            return mStore.get<int>(name);
         }
         if (object) {
             return json_object_array_length(object);
@@ -162,7 +157,7 @@ private:
         }
         value.resize(static_cast<size_t>(length));
         FromKVJsonVisitor visitor(*this, name, false);
-        if (mStorePtr->exists(k)) {
+        if (mStore.exists(k)) {
             json_object_put(visitor.mObject);
             visitor.mObject = nullptr;
         }
@@ -175,8 +170,8 @@ private:
     void getValue(int i, T& t)
     {
         std::string k = key(mKeyPrefix, std::to_string(i));
-        if (mStorePtr->exists(k)) {
-            t = mStorePtr->get<T>(k);
+        if (mStore.exists(k)) {
+            t = mStore.get<T>(k);
         }
         else {
             json_object* object = mObject ? json_object_array_get_idx(mObject, i) : nullptr;
@@ -208,7 +203,7 @@ private:
         int length = getArraySize(k, mObject);
         value.resize(static_cast<size_t>(length));
         FromKVJsonVisitor visitor(*this, i, false);
-        if (mStorePtr->exists(k)) {
+        if (mStore.exists(k)) {
             json_object_put(visitor.mObject);
             visitor.mObject = nullptr;
         }

diff --git a/src/config/from-kvstore-visitor.hpp b/src/config/from-kvstore-visitor.hpp
index 3981bfd..8279c97 100644 (file)
--- a/src/config/from-kvstore-visitor.hpp
+++ b/src/config/from-kvstore-visitor.hpp
@@ -26,20 +26,15 @@
 #define CONFIG_FROM_KVSTORE_VISITOR_HPP
 
 #include "config/is-visitable.hpp"
-#include "config/exception.hpp"
 #include "config/kvstore.hpp"
 
-#include <string>
-#include <memory>
-
 namespace config {
 
 class FromKVStoreVisitor {
 public:
-    explicit FromKVStoreVisitor(const std::string& filePath, const std::string& prefix)
-        : mStorePtr(new KVStore(filePath)),
-          mKeyPrefix(prefix),
-          mTransaction(mStorePtr->getTransaction())
+    FromKVStoreVisitor(KVStore& store, const std::string& prefix)
+        : mStore(store),
+          mKeyPrefix(prefix)
     {
     }
 
@@ -52,21 +47,19 @@ public:
     }
 
 private:
-    std::shared_ptr<KVStore> mStorePtr;
+    KVStore& mStore;
     std::string mKeyPrefix;
-    KVStore::Transaction mTransaction;
 
     FromKVStoreVisitor(const FromKVStoreVisitor& visitor, const std::string& prefix)
-        : mStorePtr(visitor.mStorePtr),
-          mKeyPrefix(prefix),
-          mTransaction(visitor.mTransaction)
+        : mStore(visitor.mStore),
+          mKeyPrefix(prefix)
     {
     }
 
     template<typename T, typename std::enable_if<!isVisitable<T>::value, int>::type = 0>
     void getInternal(const std::string& name, T& value)
     {
-        value = mStorePtr->get<T>(name);
+        value = mStore.get<T>(name);
     }
 
     template<typename T, typename std::enable_if<isVisitable<T>::value, int>::type = 0>
@@ -81,7 +74,7 @@ private:
     {
         values.clear();
 
-        size_t vectorSize = mStorePtr->get<size_t>(name);
+        size_t vectorSize = mStore.get<size_t>(name);
         if (vectorSize == 0) {
             return;
         }

diff --git a/src/config/kvstore.cpp b/src/config/kvstore.cpp
index 476d182..49656ae 100644 (file)
--- a/src/config/kvstore.cpp
+++ b/src/config/kvstore.cpp
@@ -96,47 +96,47 @@ void sqliteEscapeStr(::sqlite3_context* context, int argc, ::sqlite3_value** val
 
 } // namespace
 
-struct KVStore::TransactionImpl {
-    TransactionImpl(KVStore& kvStore)
-        : mLock(kvStore.mMutex)
-        , mKVStore(kvStore)
-    {
+KVStore::Transaction::Transaction(KVStore& kvStore)
+    : mLock(kvStore.mMutex)
+    , mKVStore(kvStore)
+    , mIsOuter(kvStore.mTransactionDepth == 0)
+{
+    if (mKVStore.mIsTransactionCommited) {
+        throw ConfigException("Previous transaction is not closed");
+    }
+    if (mIsOuter) {
         mKVStore.mConn.exec("BEGIN EXCLUSIVE TRANSACTION");
     }
+    ++mKVStore.mTransactionDepth;
+}
 
-    ~TransactionImpl()
-    {
-        // be aware of a bug in gcc with uncaught_exception and rethrow_exception
-        if (std::uncaught_exception()) {
-            try {
-                mKVStore.mConn.exec("ROLLBACK TRANSACTION");
-            } catch (ConfigException&) {}
+KVStore::Transaction::~Transaction()
+{
+    --mKVStore.mTransactionDepth;
+    if (mIsOuter) {
+        if (mKVStore.mIsTransactionCommited) {
+            mKVStore.mIsTransactionCommited = false;
         } else {
-            try {
-                mKVStore.mConn.exec("COMMIT TRANSACTION");
-            } catch (ConfigException&) {}
+            mKVStore.mConn.exec("ROLLBACK TRANSACTION");
         }
     }
+}
 
-private:
-    config::KVStore::Lock mLock;
-    config::KVStore& mKVStore;
-};
-
-KVStore::Transaction KVStore::getTransaction()
+void KVStore::Transaction::commit()
 {
-    Lock lock(mMutex);
-
-    auto t = mTransactionImplPtr.lock();
-    if (!t) {
-        t = std::make_shared<TransactionImpl>(*this);
-        mTransactionImplPtr = t;
+    if (mKVStore.mIsTransactionCommited) {
+        throw ConfigException("Transaction already commited");
+    }
+    if (mIsOuter) {
+        mKVStore.mConn.exec("COMMIT TRANSACTION");
+        mKVStore.mIsTransactionCommited = true;
     }
-    return t;
 }
 
 KVStore::KVStore(const std::string& path)
-    : mPath(path),
+    : mTransactionDepth(0),
+      mIsTransactionCommited(false),
+      mPath(path),
       mConn(path)
 {
     setupDb();
@@ -146,7 +146,7 @@ KVStore::KVStore(const std::string& path)
 
 KVStore::~KVStore()
 {
-    assert(!mTransactionImplPtr.lock());
+    assert(mTransactionDepth == 0);
 }
 
 void KVStore::setupDb()
@@ -183,45 +183,44 @@ void KVStore::createFunctions()
 
 void KVStore::clear()
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
     mConn.exec("DELETE FROM data");
+    transaction.commit();
 }
 
 bool KVStore::isEmpty()
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
     ScopedReset scopedReset(mGetIsEmptyStmt);
 
     int ret = ::sqlite3_step(mGetIsEmptyStmt->get());
-    if (ret == SQLITE_DONE) {
-        return true;
-    } else if (ret == SQLITE_ROW) {
-        return false;
-    } else {
+    if (ret != SQLITE_DONE && ret != SQLITE_ROW) {
         throw ConfigException("Error during stepping: " + mConn.getErrorMessage());
     }
+
+    transaction.commit();
+    return ret == SQLITE_DONE;
 }
 
 bool KVStore::exists(const std::string& key)
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
     ScopedReset scopedReset(mGetKeyExistsStmt);
 
     ::sqlite3_bind_text(mGetKeyExistsStmt->get(), 1, key.c_str(), AUTO_DETERM_SIZE, SQLITE_TRANSIENT);
 
     int ret = ::sqlite3_step(mGetKeyExistsStmt->get());
-    if (ret == SQLITE_DONE) {
-        return false;
-    } else if (ret == SQLITE_ROW) {
-        return true;
-    } else {
+    if (ret != SQLITE_DONE && ret != SQLITE_ROW) {
         throw ConfigException("Error during stepping: " + mConn.getErrorMessage());
     }
+
+    transaction.commit();
+    return ret == SQLITE_ROW;
 }
 
 void KVStore::remove(const std::string& key)
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
     ScopedReset scopedReset(mRemoveValuesStmt);
 
     ::sqlite3_bind_text(mRemoveValuesStmt->get(), 1, key.c_str(), AUTO_DETERM_SIZE, SQLITE_STATIC);
@@ -229,11 +228,12 @@ void KVStore::remove(const std::string& key)
     if (::sqlite3_step(mRemoveValuesStmt->get()) != SQLITE_DONE) {
         throw ConfigException("Error during stepping: " + mConn.getErrorMessage());
     }
+    transaction.commit();
 }
 
 void KVStore::setInternal(const std::string& key, const std::string& value)
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
     ScopedReset scopedReset(mSetValueStmt);
 
     ::sqlite3_bind_text(mSetValueStmt->get(), 1, key.c_str(), AUTO_DETERM_SIZE, SQLITE_STATIC);
@@ -243,6 +243,7 @@ void KVStore::setInternal(const std::string& key, const std::string& value)
     if (::sqlite3_step(mSetValueStmt->get()) != SQLITE_DONE) {
         throw ConfigException("Error during stepping: " + mConn.getErrorMessage());
     }
+    transaction.commit();
 }
 
 void KVStore::setInternal(const std::string& key, const std::initializer_list<std::string>& values)
@@ -256,7 +257,7 @@ void KVStore::setInternal(const std::string& key, const std::vector<std::string>
         throw ConfigException("Too many values to insert");
     }
 
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
 
     remove(key);
 
@@ -268,11 +269,12 @@ void KVStore::setInternal(const std::string& key, const std::vector<std::string>
         setInternal(config::key(key, std::to_string(i)),
                     values[i]);
     }
+    transaction.commit();
 }
 
 std::string KVStore::getInternal(const std::string& key, std::string*)
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
     ScopedReset scopedReset(mGetValueStmt);
 
     ::sqlite3_bind_text(mGetValueStmt->get(), 1, key.c_str(), AUTO_DETERM_SIZE, SQLITE_TRANSIENT);
@@ -285,16 +287,21 @@ std::string KVStore::getInternal(const std::string& key, std::string*)
         throw ConfigException("Error during stepping: " + mConn.getErrorMessage());
     }
 
-    return reinterpret_cast<const char*>(sqlite3_column_text(mGetValueStmt->get(), FIRST_COLUMN));
+    std::string value = reinterpret_cast<const char*>(
+            sqlite3_column_text(mGetValueStmt->get(), FIRST_COLUMN));
+
+    transaction.commit();
+    return value;
 }
 
 std::vector<std::string> KVStore::getInternal(const std::string& key, std::vector<std::string>*)
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
 
     unsigned int valuesSize = get<unsigned int>(key);
     std::vector<std::string> values(valuesSize);
     if (valuesSize == 0) {
+        transaction.commit();
         return values;
     }
 
@@ -304,12 +311,13 @@ std::vector<std::string> KVStore::getInternal(const std::string& key, std::vecto
 
     }
 
+    transaction.commit();
     return values;
 }
 
 std::vector<std::string> KVStore::getKeys()
 {
-    Transaction transaction = getTransaction();
+    Transaction transaction(*this);
     ScopedReset scopedReset(mGetKeysStmt);
 
     std::vector<std::string> result;
@@ -317,7 +325,7 @@ std::vector<std::string> KVStore::getKeys()
     for (;;) {
         int ret = ::sqlite3_step(mGetKeysStmt->get());
         if (ret == SQLITE_DONE) {
-            return result;
+            break;
         }
         if (ret != SQLITE_ROW) {
             throw ConfigException("Error during stepping: " + mConn.getErrorMessage());
@@ -326,6 +334,9 @@ std::vector<std::string> KVStore::getKeys()
                                                                             FIRST_COLUMN));
         result.push_back(key);
     }
+
+    transaction.commit();
+    return result;
 }
 
 } // namespace config

diff --git a/src/config/kvstore.hpp b/src/config/kvstore.hpp
index 1b24b36..0717665 100644 (file)
--- a/src/config/kvstore.hpp
+++ b/src/config/kvstore.hpp
@@ -45,7 +45,20 @@ public:
     /**
      * A guard struct for thread synchronization and transaction management.
      */
-    typedef std::shared_ptr<void> Transaction;
+    class Transaction {
+    public:
+        Transaction(KVStore& store);
+        ~Transaction();
+
+        Transaction(const Transaction&) = delete;
+        Transaction& operator=(const Transaction&) = delete;
+
+        void commit();
+    private:
+        std::unique_lock<std::recursive_mutex> mLock;
+        KVStore& mKVStore;
+        bool mIsOuter;
+    };
 
     /**
      * @param path configuration database file path
@@ -113,14 +126,12 @@ public:
      */
     std::vector<std::string> getKeys();
 
-    KVStore::Transaction getTransaction();
-
 private:
     typedef std::lock_guard<std::recursive_mutex> Lock;
 
-    struct TransactionImpl;
-    std::weak_ptr<TransactionImpl> mTransactionImplPtr;
     std::recursive_mutex mMutex;
+    size_t mTransactionDepth;
+    bool mIsTransactionCommited;
 
     void setInternal(const std::string& key, const std::string& value);
     void setInternal(const std::string& key, const std::initializer_list<std::string>& values);

diff --git a/src/config/manager.hpp b/src/config/manager.hpp
index 7c50087..7afde48 100644 (file)
--- a/src/config/manager.hpp
+++ b/src/config/manager.hpp
@@ -114,8 +114,11 @@ void loadFromKVStore(const std::string& filename, Config& config, const std::str
 {
     static_assert(isVisitable<Config>::value, "Use CONFIG_REGISTER macro");
 
-    FromKVStoreVisitor visitor(filename, configName);
+    KVStore store(filename);
+    KVStore::Transaction transaction(store);
+    FromKVStoreVisitor visitor(store, configName);
     config.accept(visitor);
+    transaction.commit();
 }
 
 /**
@@ -130,8 +133,11 @@ void saveToKVStore(const std::string& filename, const Config& config, const std:
 {
     static_assert(isVisitable<Config>::value, "Use CONFIG_REGISTER macro");
 
-    ToKVStoreVisitor visitor(filename, configName);
+    KVStore store(filename);
+    KVStore::Transaction transaction(store);
+    ToKVStoreVisitor visitor(store, configName);
     config.accept(visitor);
+    transaction.commit();
 }
 
 /**
@@ -150,8 +156,11 @@ void loadFromKVStoreWithJson(const std::string& kvfile,
 {
     static_assert(isVisitable<Config>::value, "Use CONFIG_REGISTER macro");
 
-    FromKVJsonVisitor visitor(kvfile, json, kvConfigName);
+    KVStore store(kvfile);
+    KVStore::Transaction transaction(store);
+    FromKVJsonVisitor visitor(store, json, kvConfigName);
     config.accept(visitor);
+    transaction.commit();
 }
 
 /**

diff --git a/src/config/to-kvstore-visitor.hpp b/src/config/to-kvstore-visitor.hpp
index 071dece..5821a66 100644 (file)
--- a/src/config/to-kvstore-visitor.hpp
+++ b/src/config/to-kvstore-visitor.hpp
@@ -28,18 +28,14 @@
 #include "config/is-visitable.hpp"
 #include "config/kvstore.hpp"
 
-#include <string>
-#include <memory>
-
 namespace config {
 
 class ToKVStoreVisitor {
 
 public:
-    ToKVStoreVisitor(const std::string& filePath, const std::string& prefix)
-        : mStorePtr(new KVStore(filePath)),
-          mKeyPrefix(prefix),
-          mTransaction(mStorePtr->getTransaction())
+    ToKVStoreVisitor(KVStore& store, const std::string& prefix)
+        : mStore(store),
+          mKeyPrefix(prefix)
     {
     }
 
@@ -52,21 +48,19 @@ public:
     }
 
 private:
-    std::shared_ptr<KVStore> mStorePtr;
+    KVStore& mStore;
     std::string mKeyPrefix;
-    KVStore::Transaction mTransaction;
 
     ToKVStoreVisitor(const ToKVStoreVisitor& visitor, const std::string& prefix)
-        : mStorePtr(visitor.mStorePtr),
-          mKeyPrefix(prefix),
-          mTransaction(visitor.mTransaction)
+        : mStore(visitor.mStore),
+          mKeyPrefix(prefix)
     {
     }
 
     template<typename T, typename std::enable_if<!isVisitable<T>::value, int>::type = 0>
     void setInternal(const std::string& name, const T& value)
     {
-        mStorePtr->set(name, value);
+        mStore.set(name, value);
     }
 
     template<typename T, typename std::enable_if<isVisitable<T>::value, int>::type = 0>
@@ -79,8 +73,8 @@ private:
     template<typename T, typename std::enable_if<isVisitable<T>::value, int>::type = 0>
     void setInternal(const std::string& name, const std::vector<T>& values)
     {
-        mStorePtr->remove(name);
-        mStorePtr->set(name, values.size());
+        mStore.remove(name);
+        mStore.set(name, values.size());
         for (size_t i = 0; i < values.size(); ++i) {
             setInternal(key(name, std::to_string(i)), values[i]);
         }