From e88efb458ffd8ef035837bdb71113f71d52614a4 Mon Sep 17 00:00:00 2001 From: blue Date: Tue, 24 Dec 2024 14:59:58 +0200 Subject: [PATCH] Cursors get closed after transaction that open them --- CHANGELOG.md | 1 + README.md | 5 +-- src/CMakeLists.txt | 1 + src/cursor.h | 5 +-- src/cursor.hpp | 80 +++++++++++++++++++++++++++++------------- src/icursor.h | 32 +++++++++++++++++ src/storage.cpp | 69 +++++++++++++++++++++++++++++++++++- src/storage.h | 38 +++++++++++--------- src/transaction.cpp | 23 +++++++++--- src/transaction.h | 9 +++-- test/cachecursor.cpp | 2 +- test/storagecursor.cpp | 16 ++++++++- 12 files changed, 225 insertions(+), 56 deletions(-) create mode 100644 src/icursor.h diff --git a/CHANGELOG.md b/CHANGELOG.md index 608c121..356decc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Improvements - Less dereferencing - Transactions are now closed with the database +- Cursors are now closed if the public transaction that opened them got closed ### Bug fixes - SIGSEGV on closing database with opened transactions diff --git a/README.md b/README.md index 9c6a152..6046d65 100644 --- a/README.md +++ b/README.md @@ -1,13 +1,14 @@ # LMDBAL - Lightning Memory Data Base Abstraction Level [![AUR license](https://img.shields.io/aur/license/lmdbal?style=flat-square)](https://git.macaw.me/blue/lmdbal/raw/branch/master/LICENSE.md) -[![AUR version](https://img.shields.io/aur/version/lmdbal?style=flat-square)](https://aur.archlinux.org/packages/lmdbal/) +[![AUR qt5 version](https://img.shields.io/aur/version/lmdbal-qt5?style=flat-square)](https://aur.archlinux.org/packages/lmdbal-qt5/) +[![AUR qt6 version](https://img.shields.io/aur/version/lmdbal-qt6?style=flat-square)](https://aur.archlinux.org/packages/lmdbal-qt6/) [![Liberapay patrons](https://img.shields.io/liberapay/patrons/macaw.me?logo=liberapay&style=flat-square)](https://liberapay.com/macaw.me) [![Documentation](https://img.shields.io/badge/Documentation-HTML-green)](https://macaw.me/lmdbal/doc/html) ### Prerequisites -- a compiler (c++ would do) +- a c++ compiler (g++ would do) - Qt 5 or 6 or higher (qt5-base or qt6-base would do) - lmdb - CMake 3.16 or higher diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 9332fd8..a717e80 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -16,6 +16,7 @@ set(HEADERS cache.hpp operators.hpp transaction.h + icursor.h ) target_sources(${LMDBAL_NAME} PRIVATE ${SOURCES}) diff --git a/src/cursor.h b/src/cursor.h index c77d4e9..65a947d 100644 --- a/src/cursor.h +++ b/src/cursor.h @@ -24,11 +24,12 @@ #include "base.h" #include "storage.h" #include "transaction.h" +#include "icursor.h" namespace LMDBAL { template -class Cursor { +class Cursor : public iCursor { friend class Storage; private: enum State { /** /** @@ -84,6 +85,10 @@ LMDBAL::Cursor::Cursor(Cursor&& other): if (id != 0) storage->cursors[id] = this; + if (state == openedPublic) + storage->attachCursorToTransaction(id, cursor, this); + + other.freed(); } @@ -110,6 +115,9 @@ LMDBAL::Cursor& LMDBAL::Cursor::operator = (Cursor&& other) { other.state = closed; storage->cursors[id] = this; + + if (state == openedPublic) + storage->attachCursorToTransaction(id, cursor, this); } return *this; @@ -184,7 +192,20 @@ bool LMDBAL::Cursor::empty () const { */ template void LMDBAL::Cursor::terminated () { - close(); //for now it's the same, but if I ever going to make writable cursor - here is where it's gonna be different + switch (state) { + case openedPublic: + storage->_mdbCursorClose(cursor); + + state = closed; + break; + case openedPrivate: + storage->closeCursorTransaction(cursor); + + state = closed; + break; + default: + break; + } } /** @@ -205,17 +226,11 @@ void LMDBAL::Cursor::open () { if (empty()) throw CursorEmpty(openCursorMethodName); - storage->ensureOpened(openCursorMethodName); switch (state) { - case closed: { - TransactionID txn = storage->beginReadOnlyTransaction(); - int result = storage->_mdbCursorOpen(txn, &cursor); - if (result != MDB_SUCCESS) - storage->throwUnknown(result, txn); - - storage->transactionStarted(txn, true); + case closed: + storage->openCursorTransaction(&cursor); state = openedPrivate; - } break; + break; default: break; } @@ -250,6 +265,7 @@ void LMDBAL::Cursor::open (const Transaction& transaction) { if (result != MDB_SUCCESS) storage->throwUnknown(result); + storage->attachCursorToTransaction(id, cursor, this); state = openedPublic; } break; default: @@ -285,15 +301,24 @@ void LMDBAL::Cursor::renew () { TransactionID txn = storage->_mdbCursorTxn(cursor); storage->abortTransaction(txn); storage->transactionAborted(txn); - [[fallthrough]]; + + txn = storage->beginReadOnlyTransaction(); + int result = storage->_mdbCursorRenew(txn, cursor); + if (result != MDB_SUCCESS) + storage->throwUnknown(result, txn); + + storage->transactionStarted(txn, true); + state = openedPrivate; } case openedPublic: { + storage->disconnectCursorFromTransaction(id, cursor); TransactionID txn = storage->beginReadOnlyTransaction(); int result = storage->_mdbCursorRenew(txn, cursor); if (result != MDB_SUCCESS) storage->throwUnknown(result, txn); storage->transactionStarted(txn, true); + storage->attachCursorToTransaction(id, cursor, this); state = openedPrivate; } break; default: @@ -330,17 +355,24 @@ void LMDBAL::Cursor::renew (const Transaction& transaction) { TransactionID txn = storage->extractTransactionId(transaction, renewCursorMethodName); switch (state) { case openedPrivate: { - TransactionID txn = storage->_mdbCursorTxn(cursor); - storage->abortTransaction(txn); - storage->transactionAborted(txn); - [[fallthrough]]; - } - case openedPublic: { + TransactionID txnOld = storage->_mdbCursorTxn(cursor); + storage->abortTransaction(txnOld); + storage->transactionAborted(txnOld); + int result = storage->_mdbCursorRenew(txn, cursor); if (result != MDB_SUCCESS) storage->throwUnknown(result); state = openedPublic; + } + case openedPublic: { + storage->disconnectCursorFromTransaction(id, cursor); + int result = storage->_mdbCursorRenew(txn, cursor); + if (result != MDB_SUCCESS) + storage->throwUnknown(result); + + storage->attachCursorToTransaction(id, cursor, this); + state = openedPublic; } break; default: break; @@ -360,19 +392,17 @@ void LMDBAL::Cursor::renew (const Transaction& transaction) { template void LMDBAL::Cursor::close () { switch (state) { - case openedPublic: { + case openedPublic: + storage->disconnectCursorFromTransaction(id, cursor); storage->_mdbCursorClose(cursor); state = closed; - } break; - case openedPrivate: { - TransactionID txn = storage->_mdbCursorTxn(cursor); - storage->_mdbCursorClose(cursor); - storage->abortTransaction(txn); - storage->transactionAborted(txn); + break; + case openedPrivate: + storage->closeCursorTransaction(cursor); state = closed; - } break; + break; default: break; } diff --git a/src/icursor.h b/src/icursor.h new file mode 100644 index 0000000..04bc811 --- /dev/null +++ b/src/icursor.h @@ -0,0 +1,32 @@ +/* + * LMDB Abstraction Layer. + * Copyright (C) 2023 Yury Gubich + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#pragma once + +namespace LMDBAL { + +class Transaction; + +class iCursor { + friend class Transaction; +protected: + virtual ~iCursor() {} + virtual void terminated() = 0; +}; + +} diff --git a/src/storage.cpp b/src/storage.cpp index cea474d..4847760 100644 --- a/src/storage.cpp +++ b/src/storage.cpp @@ -50,7 +50,7 @@ LMDBAL::iStorage::iStorage(Base* parent, const std::string& name, bool duplicate LMDBAL::iStorage::~iStorage() {} /** - * \brief A private virtual function I need to close each storage in the database + * \brief A private virtual function to close each storage in the database */ void LMDBAL::iStorage::close() { mdb_dbi_close(db->environment, dbi); @@ -159,6 +159,73 @@ LMDBAL::SizeType LMDBAL::iStorage::count() const { return amount; } +/** + * \brief Links cursor to the transaction it has been opened with + * + * This a service function is designed to be called by from LMDBAL::Cursor + * Cursor must be opened by a public transaction + * + * \exception std::out_of_range thrown if LMDBAL::Transaction pointer wasn't found in the LMDBAL::Base + */ + +void LMDBAL::iStorage::attachCursorToTransaction (uint32_t cursorId, MDB_cursor* cursorHandle, iCursor* pointer) const { + TransactionID txnID = _mdbCursorTxn(cursorHandle); + Transaction* txn = db->transactions.at(txnID); + txn->cursors[cursorId] = pointer; +} + + +/** + * \brief Opens a transaction that is ment to be private to LMDBAL::Cursor, but public to the LMDBAL::Storage + * + * This method is ment to be called from LMDBAL::Cursor + * + * \param[out] cursor - cursor handle + * + * \exception LMDBAL::Closed thrown if you try to open the cursor on a closed database + * \exception LMDBAL::Unknown thrown if there was a problem opening the cursor by the lmdb, or to begin a transaction + */ +void LMDBAL::iStorage::openCursorTransaction (MDB_cursor** cursor) const { + ensureOpened(openCursorTransactionMethodName); + + TransactionID txn = beginReadOnlyTransaction(); + int result = _mdbCursorOpen(txn, cursor); + if (result != MDB_SUCCESS) + throwUnknown(result, txn); + + transactionStarted(txn, true); +} + +/** + * \brief Closes transaction that is private to LMDBAL::Cursor, but public to the LMDBAL::Storage + * + * This method is ment to be called from LMDBAL::Cursor + * + * \param[in] cursor - cursor handle + */ +void LMDBAL::iStorage::closeCursorTransaction (MDB_cursor* cursor) const { + TransactionID txn = _mdbCursorTxn(cursor); + _mdbCursorClose(cursor); + abortTransaction(txn); + transactionAborted(txn); +} + +/** + * \brief Disconnects cursor from the transaction it has been opened with + * + * This a service function is designed to be called by from LMDBAL::Cursor + * Cursor must be still opened by a public transaction + * + * \exception std::out_of_range thrown if LMDBAL::Transaction pointer wasn't found in the LMDBAL::Base + */ + +void LMDBAL::iStorage::disconnectCursorFromTransaction (uint32_t cursorId, MDB_cursor* cursorHandle) const { + TransactionID txnID = _mdbCursorTxn(cursorHandle); + Transaction* txn = db->transactions.at(txnID); + txn->cursors.erase(cursorId); +} + + /** * \brief Storage size (private transaction variant) * diff --git a/src/storage.h b/src/storage.h index c077500..08c37ba 100644 --- a/src/storage.h +++ b/src/storage.h @@ -76,6 +76,11 @@ protected: virtual int drop(TransactionID transaction); virtual SizeType count(TransactionID txn) const; + void openCursorTransaction(MDB_cursor** cursor) const; + void closeCursorTransaction(MDB_cursor* cursor) const; + void attachCursorToTransaction(uint32_t cursorId, MDB_cursor* cursorHandle, iCursor* pointer) const; + void disconnectCursorFromTransaction(uint32_t cursorId, MDB_cursor* cursorHandle) const; + int _mdbOpen(MDB_txn* txn, unsigned int flags = 0); int _mdbPut(MDB_txn* txn, MDB_val& key, MDB_val& data, unsigned int flags = 0); @@ -103,24 +108,25 @@ public: virtual SizeType count(const Transaction& txn) const; protected: - MDB_dbi dbi; /**<\brief lmdb storage handle*/ - Base* db; /**<\brief parent database pointer (borrowed)*/ - const std::string name; /**<\brief this storage name*/ - const bool duplicates; /**<\brief true if storage supports duplicates*/ + MDB_dbi dbi; /**<\brief lmdb storage handle*/ + Base* db; /**<\brief parent database pointer (borrowed)*/ + const std::string name; /**<\brief this storage name*/ + const bool duplicates; /**<\brief true if storage supports duplicates*/ - inline static const std::string dropMethodName = "drop"; /**<\brief member function name, just for exceptions*/ - inline static const std::string countMethodName = "count"; /**<\brief member function name, just for exceptions*/ - inline static const std::string flagsMethodName = "flags"; /**<\brief member function name, just for exceptions*/ + inline static const std::string dropMethodName = "drop"; /**<\brief member function name, just for exceptions*/ + inline static const std::string countMethodName = "count"; /**<\brief member function name, just for exceptions*/ + inline static const std::string flagsMethodName = "flags"; /**<\brief member function name, just for exceptions*/ - inline static const std::string addRecordMethodName = "addRecord"; /**<\brief member function name, just for exceptions*/ - inline static const std::string forceRecordMethodName = "forceRecord"; /**<\brief member function name, just for exceptions*/ - inline static const std::string changeRecordMethodName = "changeRecord"; /**<\brief member function name, just for exceptions*/ - inline static const std::string removeRecordMethodName = "removeRecord"; /**<\brief member function name, just for exceptions*/ - inline static const std::string checkRecordMethodName = "checkRecord"; /**<\brief member function name, just for exceptions*/ - inline static const std::string getRecordMethodName = "getRecord"; /**<\brief member function name, just for exceptions*/ - inline static const std::string readAllMethodName = "readAllRecord"; /**<\brief member function name, just for exceptions*/ - inline static const std::string replaceAllMethodName = "replaceAll"; /**<\brief member function name, just for exceptions*/ - inline static const std::string addRecordsMethodName = "addRecords"; /**<\brief member function name, just for exceptions*/ + inline static const std::string addRecordMethodName = "addRecord"; /**<\brief member function name, just for exceptions*/ + inline static const std::string forceRecordMethodName = "forceRecord"; /**<\brief member function name, just for exceptions*/ + inline static const std::string changeRecordMethodName = "changeRecord"; /**<\brief member function name, just for exceptions*/ + inline static const std::string removeRecordMethodName = "removeRecord"; /**<\brief member function name, just for exceptions*/ + inline static const std::string checkRecordMethodName = "checkRecord"; /**<\brief member function name, just for exceptions*/ + inline static const std::string getRecordMethodName = "getRecord"; /**<\brief member function name, just for exceptions*/ + inline static const std::string readAllMethodName = "readAllRecord"; /**<\brief member function name, just for exceptions*/ + inline static const std::string replaceAllMethodName = "replaceAll"; /**<\brief member function name, just for exceptions*/ + inline static const std::string addRecordsMethodName = "addRecords"; /**<\brief member function name, just for exceptions*/ + inline static const std::string openCursorTransactionMethodName = "openCursorTransaction"; /**<\brief member function name, just for exceptions*/ protected: template diff --git a/src/transaction.cpp b/src/transaction.cpp index 52e3933..d2e6a05 100644 --- a/src/transaction.cpp +++ b/src/transaction.cpp @@ -40,7 +40,8 @@ LMDBAL::Transaction::Transaction(): txn(nullptr), active(false), - parent(nullptr) + parent(nullptr), + cursors() {} /** @@ -49,7 +50,8 @@ LMDBAL::Transaction::Transaction(): LMDBAL::Transaction::Transaction(TransactionID txn, const Base* parent) : txn(txn), active(true), - parent(parent) + parent(parent), + cursors() { parent->transactions[txn] = this; } @@ -60,7 +62,8 @@ LMDBAL::Transaction::Transaction(TransactionID txn, const Base* parent) : LMDBAL::Transaction::Transaction(Transaction&& other): txn(other.txn), active(other.active), - parent(other.parent) + parent(other.parent), + cursors(other.cursors) { if (active) { parent->transactions[txn] = this; @@ -88,6 +91,7 @@ LMDBAL::Transaction& LMDBAL::Transaction::operator=(Transaction&& other) { txn = other.txn; active = other.active; parent = other.parent; + cursors = other.cursors; if (active) { parent->transactions[txn] = this; @@ -105,13 +109,14 @@ LMDBAL::Transaction& LMDBAL::Transaction::operator=(Transaction&& other) { */ void LMDBAL::Transaction::terminate() { if (active) { + closeCursors(); parent->abortTransaction(txn); - parent->transactions.erase(txn); reset(); } } + /** * \brief Resets inner transaction properties to inactive state */ @@ -119,8 +124,16 @@ void LMDBAL::Transaction::reset() { active = false; txn = nullptr; parent = nullptr; + cursors.clear(); } +/** + * \brief Closes attached curors; + */ +void LMDBAL::Transaction::closeCursors () { + for (const std::pair& pair : cursors) + pair.second->terminated(); +} /** * \brief Returns transaction states @@ -197,8 +210,8 @@ void LMDBAL::WriteTransaction::abort() { */ void LMDBAL::WriteTransaction::commit() { if (active) { + closeCursors(); const_cast(parent)->commitTransaction(txn); - parent->transactions.erase(txn); reset(); } diff --git a/src/transaction.h b/src/transaction.h index 81acad6..f37dab0 100644 --- a/src/transaction.h +++ b/src/transaction.h @@ -19,6 +19,7 @@ #pragma once #include "base.h" +#include "icursor.h" namespace LMDBAL { class iStorage; @@ -40,11 +41,13 @@ public: protected: Transaction(TransactionID txn, const Base* parent); void reset(); + void closeCursors(); protected: - TransactionID txn; /**<\brief Transaction inner handler*/ - bool active; /**<\brief Transaction state*/ - const Base* parent; /**<\brief Pointer to the database this transaction belongs to*/ + TransactionID txn; /**<\brief Transaction inner handler*/ + bool active; /**<\brief Transaction state*/ + const Base* parent; /**<\brief Pointer to the database this transaction belongs to*/ + std::map cursors; /**<\brief a collection of cursors curently opened under this transaction*/ }; class WriteTransaction : public Transaction { diff --git a/test/cachecursor.cpp b/test/cachecursor.cpp index 7d9d440..ec15b46 100644 --- a/test/cachecursor.cpp +++ b/test/cachecursor.cpp @@ -402,7 +402,7 @@ TEST_F(CacheCursorTest, CursorRAIIBehaviour) { TEST_F(CacheCursorTest, CornerCases) { transaction.terminate(); - EXPECT_THROW(cursor.current(), LMDBAL::Unknown); + EXPECT_THROW(cursor.current(), LMDBAL::CursorNotReady); cursor.close(); LMDBAL::Cursor emptyCursor = emptyCache->createCursor(); diff --git a/test/storagecursor.cpp b/test/storagecursor.cpp index dde8d82..7e332fb 100644 --- a/test/storagecursor.cpp +++ b/test/storagecursor.cpp @@ -380,7 +380,7 @@ TEST_F(StorageCursorTest, CursorRAIIBehaviour) { TEST_F(StorageCursorTest, CornerCases) { EXPECT_EQ(getTableCursorsSize(), 1); transaction.terminate(); - EXPECT_THROW(cursor.current(), LMDBAL::Unknown); + EXPECT_THROW(cursor.current(), LMDBAL::CursorNotReady); cursor.close(); LMDBAL::Cursor emptyCursor = emptyTable->createCursor(); @@ -414,4 +414,18 @@ TEST_F(StorageCursorTest, CornerCases) { EXPECT_EQ(element.first, reference->first); EXPECT_EQ(element.second, reference->second); EXPECT_THROW(cursor.prev(), LMDBAL::NotFound); + + cursor.close(); +} + +TEST_F(StorageCursorTest, TerminatedTransaction) { + LMDBAL::Cursor emptyCursor = table->createCursor(); + + { + LMDBAL::Transaction txn = db->beginReadOnlyTransaction(); + cursor.open(txn); + EXPECT_NO_THROW(cursor.first()); + } + + EXPECT_FALSE(cursor.opened()); }