Transactions now get closed with the database

CHANGELOG.md

@@ -3,6 +3,10 @@
 # LMDBAL 0.6.0 (UNRELEASED)
 ### Improvements
 - Less dereferencing
+- Transactions are now closed with the database
+
+### Bug fixes
+- SIGSEGV on closing database with opened transactions
 
 # LMDBAL 0.5.4 (November 30, 2024)
 ### Improvements

CMakeLists.txt

@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.16)
 
 project(LMDBAL
-  VERSION 0.5.4
+  VERSION 0.6.0
   DESCRIPTION "LMDB (Lightning Memory-Mapped Database Manager) Abstraction Layer"
   LANGUAGES CXX
 )

src/base.cpp

@@ -66,8 +66,10 @@ LMDBAL::Base::~Base() {
  */
 void LMDBAL::Base::close() {
     if (opened) {
-        for (const LMDBAL::TransactionID id : transactions)
+        for (const std::pair<LMDBAL::TransactionID, LMDBAL::Transaction*> pair : transactions) {
-            abortTransaction(id, emptyName);
+            abortTransaction(pair.first, emptyName);
+            pair.second->reset();
+        }
 
         for (const std::pair<const std::string, iStorage*>& pair : storages)
             pair.second->close();
@@ -259,12 +261,13 @@ LMDBAL::WriteTransaction LMDBAL::Base::beginTransaction() {
  * \brief Aborts transaction
  *
  * Terminates transaction cancelling changes.
- * This is an optimal way to terminate read-only transactions
+ * Every storage receives notification about this transaction being aborted.
+ * This is an optimal way to abort public transactions
  *
  * \param[in] id    - transaction ID you want to abort
  *
  * \exception LMDBAL::Closed    - thrown if the database is closed
- * \exception LMDBAL::Unknown   - thrown if transaction with given ID was not found or if something unexpected happened
+ * \exception LMDBAL::Unknown   - thrown if something unexpected happened
  */
 void LMDBAL::Base::abortTransaction(LMDBAL::TransactionID id) const {
     return abortTransaction(id, emptyName);}
@@ -273,11 +276,13 @@ void LMDBAL::Base::abortTransaction(LMDBAL::TransactionID id) const {
  * \brief Commits transaction
  *
  * Terminates transaction applying changes.
+ * Every storage receives notification about this transaction being committed.
+ * This is an optimal way to commit public transactions
  *
  * \param[in] id    - transaction ID you want to commit
  *
  * \exception LMDBAL::Closed    - thrown if the database is closed
- * \exception LMDBAL::Unknown   - thrown if transaction with given ID was not found or if something unexpected happened
+ * \exception LMDBAL::Unknown   - thrown if something unexpected happened
  */
 void LMDBAL::Base::commitTransaction(LMDBAL::TransactionID id) {
     return commitTransaction(id, emptyName);}
@@ -285,7 +290,9 @@ void LMDBAL::Base::commitTransaction(LMDBAL::TransactionID id) {
 /**
  * \brief Begins read-only transaction
  *
- * This function is intended to be called from subordinate storage or cache
+ * This function is intended to be called from subordinate storage, cache, transaction or cursor.
+ * Every storage receives notification about this transaction being started.
+ * This is an optimal way to begin read-only public transactions.
  *
  * \param[in] storageName   - name of the storage/cache that you begin transaction from, needed just to inform if something went wrong
  *
@@ -299,7 +306,6 @@ LMDBAL::TransactionID LMDBAL::Base::beginReadOnlyTransaction(const std::string&
         throw Closed("beginReadOnlyTransaction", name, storageName);
 
     TransactionID txn = beginPrivateReadOnlyTransaction(storageName);
-    transactions.emplace(txn);
     for (const std::pair<const std::string, LMDBAL::iStorage*>& pair : storages)
         pair.second->transactionStarted(txn, true);
 
@@ -309,7 +315,9 @@ LMDBAL::TransactionID LMDBAL::Base::beginReadOnlyTransaction(const std::string&
 /**
  * \brief Begins writable transaction
  *
- * This function is intended to be called from subordinate storage or cache
+ * This function is intended to be called from subordinate storage, cache, transaction or cursor.
+ * Every storage receives notification about this transaction being started.
+ * This is an optimal way to begin public transactions.
  *
  * \param[in] storageName   - name of the storage/cache that you begin transaction from, needed just to inform if something went wrong
  *
@@ -323,7 +331,6 @@ LMDBAL::TransactionID LMDBAL::Base::beginTransaction(const std::string& storageN
         throw Closed("beginTransaction", name, storageName);
 
     TransactionID txn = beginPrivateTransaction(storageName);
-    transactions.emplace(txn);
     for (const std::pair<const std::string, LMDBAL::iStorage*>& pair : storages)
         pair.second->transactionStarted(txn, false);
 
@@ -334,55 +341,46 @@ LMDBAL::TransactionID LMDBAL::Base::beginTransaction(const std::string& storageN
  * \brief Aborts transaction
  *
  * Terminates transaction cancelling changes.
- * This is an optimal way to terminate read-only transactions.
+ * Every storage receives notification about this transaction being aborted.
- * This function is intended to be called from subordinate storage or cache
+ * This is an optimal way to abort public transactions.
+ * This function is intended to be called from subordinate storage, cache, transaction or cursor
  *
  * \param[in] id            - transaction ID you want to abort
  * \param[in] storageName   - name of the storage/cache that you begin transaction from, needed just to inform if something went wrong
  *
  * \exception LMDBAL::Closed    - thrown if the database is closed
- * \exception LMDBAL::Unknown   - thrown if transaction with given ID was not found or if something unexpected happened
+ * \exception LMDBAL::Unknown   - thrown if something unexpected happened
  */
 void LMDBAL::Base::abortTransaction(LMDBAL::TransactionID id, const std::string& storageName) const {
     if (!opened)
         throw Closed("abortTransaction", name, storageName);
 
-    Transactions::iterator itr = transactions.find(id);
-    if (itr == transactions.end())     //TODO may be it's a good idea to make an exception class for this
-        throw Unknown(name, "unable to abort transaction: transaction was not found", storageName);
-
     abortPrivateTransaction(id, storageName);
     for (const std::pair<const std::string, LMDBAL::iStorage*>& pair : storages)
         pair.second->transactionAborted(id);
-
-    transactions.erase(itr);
 }
 
 /**
  * \brief Commits transaction
  *
  * Terminates transaction applying changes.
- * This function is intended to be called from subordinate storage or cache
+ * Every storage receives notification about this transaction being committed.
+ * This is an optimal way to commit public transactions
+ * This function is intended to be called from subordinate storage, cache, transaction or cursor
  *
  * \param[in] id            - transaction ID you want to commit
  * \param[in] storageName   - name of the storage/cache that you begin transaction from, needed just to inform if something went wrong
  *
  * \exception LMDBAL::Closed    - thrown if the database is closed
- * \exception LMDBAL::Unknown   - thrown if transaction with given ID was not found or if something unexpected happened
+ * \exception LMDBAL::Unknown   - thrown if something unexpected happened
  */
 void LMDBAL::Base::commitTransaction(LMDBAL::TransactionID id, const std::string& storageName) {
     if (!opened)
         throw Closed("abortTransaction", name, storageName);
 
-    Transactions::iterator itr = transactions.find(id);
-    if (itr == transactions.end())     //TODO may be it's a good idea to make an exception class for this
-        throw Unknown(name, "unable to commit transaction: transaction was not found", storageName);
-
     commitPrivateTransaction(id, storageName);
     for (const std::pair<const std::string, LMDBAL::iStorage*>& pair : storages)
         pair.second->transactionCommited(id);
-
-    transactions.erase(itr);
 }
 
 /**

src/base.h

@@ -85,7 +85,7 @@ public:
 
 private:
     typedef std::map<std::string, LMDBAL::iStorage*> Storages;  /**<\brief Storage and Cache pointers are saved in the std::map*/
-    typedef std::set<TransactionID> Transactions;               /**<\brief Piblic transaction IDs are saved in the std::set*/
+    typedef std::map<TransactionID, Transaction*> Transactions; /**<\brief Piblic transaction IDs are saved in the std::set*/
 
     void commitTransaction(TransactionID id);
     void abortTransaction(TransactionID id) const;

src/transaction.cpp

@@ -50,7 +50,9 @@ LMDBAL::Transaction::Transaction(TransactionID txn, const Base* parent) :
     txn(txn),
     active(true),
     parent(parent)
-{}
+{
+    parent->transactions[txn] = this;
+}
 
 /**
  * \brief Moves transaction to a new object
@@ -60,7 +62,11 @@ LMDBAL::Transaction::Transaction(Transaction&& other):
     active(other.active),
     parent(other.parent)
 {
-    other.active = false;
+    if (active) {
+        parent->transactions[txn] = this;
+
+        other.reset();
+    }
 }
 
 /**
@@ -74,13 +80,20 @@ LMDBAL::Transaction::~Transaction() {
  * \brief Move-assigns transaction to the new object
  */
 LMDBAL::Transaction& LMDBAL::Transaction::operator=(Transaction&& other) {
+    if (this == &other)
+        return *this;
+
     terminate();
 
     txn = other.txn;
     active = other.active;
     parent = other.parent;
 
-    other.active = false;
+    if (active) {
+        parent->transactions[txn] = this;
+
+        other.reset();
+    }
 
     return *this;
 }
@@ -93,9 +106,21 @@ LMDBAL::Transaction& LMDBAL::Transaction::operator=(Transaction&& other) {
 void LMDBAL::Transaction::terminate() {
     if (active) {
         parent->abortTransaction(txn);
+
+        parent->transactions.erase(txn);
+        reset();
+    }
+}
+
+/**
+ * \brief Resets inner transaction properties to inactive state
+ */
+void LMDBAL::Transaction::reset() {
     active = false;
+    txn = nullptr;
+    parent = nullptr;
 }
-}
+
 
 /**
  * \brief Returns transaction states
@@ -150,6 +175,12 @@ LMDBAL::WriteTransaction::WriteTransaction(WriteTransaction&& other):
     Transaction(std::move(other))
 {}
 
+LMDBAL::WriteTransaction& LMDBAL::WriteTransaction::operator=(WriteTransaction&& other) {
+    Transaction::operator=(std::move(other));
+
+    return *this;
+}
+
 /**
  * \brief Aborts transaction cancelling all changes
  *
@@ -167,6 +198,8 @@ void LMDBAL::WriteTransaction::abort() {
 void LMDBAL::WriteTransaction::commit() {
     if (active) {
         const_cast<Base*>(parent)->commitTransaction(txn);
-        active = false;
+
+        parent->transactions.erase(txn);
+        reset();
     }
 }

src/transaction.h

@@ -39,6 +39,7 @@ public:
 
 protected:
     Transaction(TransactionID txn, const Base* parent);
+    void reset();
 
 protected:
     TransactionID txn;          /**<\brief Transaction inner handler*/
@@ -53,6 +54,7 @@ public:
     explicit WriteTransaction(WriteTransaction&& other);
     WriteTransaction(const WriteTransaction& other) = delete;
     WriteTransaction& operator = (const WriteTransaction& other) = delete;
+    WriteTransaction& operator = (WriteTransaction&& other);
 
     void commit();
     void abort();

test/cachetransaction.cpp

@@ -279,3 +279,26 @@ TEST_F(CacheTransactionsTest, RAIIResourceFree) {
     EXPECT_EQ(c1->getRecord(221), 14);
 }
 
+TEST_F(CacheTransactionsTest, TransactionTerminationOnClose) {
+    LMDBAL::WriteTransaction txn = db->beginTransaction();
+
+    c1->addRecord(578, 4552, txn);
+
+    EXPECT_EQ(c1->getRecord(578, txn), 4552);
+    EXPECT_EQ(c1->checkRecord(578), false);
+
+    db->close();
+    db->open();
+
+    EXPECT_EQ(txn.isActive(), false);
+    EXPECT_THROW(c1->getRecord(578, txn), LMDBAL::TransactionTerminated);
+    EXPECT_NO_THROW(txn.commit());
+
+    EXPECT_EQ(c1->checkRecord(578), false);
+
+    txn = db->beginTransaction();
+    c1->addRecord(578, 4552, txn);
+    txn.commit();
+
+    EXPECT_EQ(c1->getRecord(578), 4552);
+}

test/storagetransaction.cpp

@@ -277,3 +277,27 @@ TEST_F(StorageTransactionsTest, RAIIResourceFree) {
     std::cout << "checking the final result" << std::endl;
     EXPECT_EQ(t1->getRecord(221), 14);
 }
+
+TEST_F(StorageTransactionsTest, TransactionTerminationOnClose) {
+    LMDBAL::WriteTransaction txn = db->beginTransaction();
+
+    t1->addRecord(543, 229, txn);
+
+    EXPECT_EQ(t1->getRecord(543, txn), 229);
+    EXPECT_EQ(t1->checkRecord(543), false);
+
+    db->close();
+    db->open();
+
+    EXPECT_EQ(txn.isActive(), false);
+    EXPECT_THROW(t1->getRecord(543, txn), LMDBAL::TransactionTerminated);
+    EXPECT_NO_THROW(txn.commit());
+
+    EXPECT_EQ(t1->checkRecord(543), false);
+
+    txn = db->beginTransaction();
+    t1->addRecord(543, 229, txn);
+    txn.commit();
+
+    EXPECT_EQ(t1->getRecord(543), 229);
+}