From 68ea7df6a9fc515487d1ee9a98165b3bbf1aa7e8 Mon Sep 17 00:00:00 2001 From: blue Date: Sun, 22 Dec 2024 19:39:35 +0200 Subject: [PATCH] Transactions now get closed with the database --- CHANGELOG.md | 4 +++ CMakeLists.txt | 2 +- src/base.cpp | 50 ++++++++++++++++++------------------- src/base.h | 2 +- src/exceptions.h | 2 +- src/transaction.cpp | 43 +++++++++++++++++++++++++++---- src/transaction.h | 2 ++ test/cachetransaction.cpp | 23 +++++++++++++++++ test/storagetransaction.cpp | 24 ++++++++++++++++++ 9 files changed, 118 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 879f52d..608c121 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/CMakeLists.txt b/CMakeLists.txt index a518d7f..97b21ff 100644 --- a/CMakeLists.txt +++ b/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 ) diff --git a/src/base.cpp b/src/base.cpp index ef932fd..ce28975 100644 --- a/src/base.cpp +++ b/src/base.cpp @@ -66,8 +66,10 @@ LMDBAL::Base::~Base() { */ void LMDBAL::Base::close() { if (opened) { - for (const LMDBAL::TransactionID id : transactions) - abortTransaction(id, emptyName); + for (const std::pair pair : transactions) { + abortTransaction(pair.first, emptyName); + pair.second->reset(); + } for (const std::pair& 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& 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& 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. - * This function is intended to be called from subordinate storage or cache + * Every storage receives notification about this transaction being aborted. + * 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& 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& pair : storages) pair.second->transactionCommited(id); - - transactions.erase(itr); } /** diff --git a/src/base.h b/src/base.h index 749683f..de61ad7 100644 --- a/src/base.h +++ b/src/base.h @@ -85,7 +85,7 @@ public: private: typedef std::map Storages; /**<\brief Storage and Cache pointers are saved in the std::map*/ - typedef std::set Transactions; /**<\brief Piblic transaction IDs are saved in the std::set*/ + typedef std::map Transactions; /**<\brief Piblic transaction IDs are saved in the std::set*/ void commitTransaction(TransactionID id); void abortTransaction(TransactionID id) const; diff --git a/src/exceptions.h b/src/exceptions.h index 6f03bd8..d5f252b 100644 --- a/src/exceptions.h +++ b/src/exceptions.h @@ -40,7 +40,7 @@ public: /** * \brief Thrown if LMDBAL had issues creating or opening database directory */ -class Directory: public Exception { +class Directory : public Exception { public: /** * \brief Creates exception diff --git a/src/transaction.cpp b/src/transaction.cpp index 3d7e3c0..52e3933 100644 --- a/src/transaction.cpp +++ b/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,10 +106,22 @@ LMDBAL::Transaction& LMDBAL::Transaction::operator=(Transaction&& other) { void LMDBAL::Transaction::terminate() { if (active) { parent->abortTransaction(txn); - active = false; + + 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(parent)->commitTransaction(txn); - active = false; + + parent->transactions.erase(txn); + reset(); } } diff --git a/src/transaction.h b/src/transaction.h index 59b6ae9..81acad6 100644 --- a/src/transaction.h +++ b/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(); diff --git a/test/cachetransaction.cpp b/test/cachetransaction.cpp index e0bac76..0fc259d 100644 --- a/test/cachetransaction.cpp +++ b/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); +} diff --git a/test/storagetransaction.cpp b/test/storagetransaction.cpp index 4f04931..237a7c6 100644 --- a/test/storagetransaction.cpp +++ b/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); +}