Cursors get closed after transaction that open them

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

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

src/CMakeLists.txt

@@ -16,6 +16,7 @@ set(HEADERS
   cache.hpp
   operators.hpp
   transaction.h
+  icursor.h
 )
 
 target_sources(${LMDBAL_NAME} PRIVATE ${SOURCES})

src/cursor.h

@@ -24,11 +24,12 @@
 #include "base.h"
 #include "storage.h"
 #include "transaction.h"
+#include "icursor.h"
 
 namespace LMDBAL {
 
 template <class K, class V>
-class Cursor {
+class Cursor : public iCursor {
     friend class Storage<K, V>;
 private:
     enum State {            /**<Cursor state:*/
@@ -71,9 +72,9 @@ public:
     void current(K& key, V& value) const;
 
 private:
+    virtual void terminated() override;
     void dropped();
     void freed();
-    void terminated();
     void operateCursorRead(K& key, V& value, MDB_cursor_op operation, const std::string& methodName, const std::string& operationName) const;
 
 private:

src/cursor.hpp

@@ -19,6 +19,7 @@
 #pragma once
 
 #include "cursor.h"
+
 #include <iostream>
 
 /**
@@ -84,6 +85,10 @@ LMDBAL::Cursor<K, V>::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<K, V>& LMDBAL::Cursor<K, V>::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<K, V>::empty () const {
  */
 template<class K, class V>
 void LMDBAL::Cursor<K, V>::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<K, V>::open () {
     if (empty())
         throw CursorEmpty(openCursorMethodName);
 
-    storage->ensureOpened(openCursorMethodName);
     switch (state) {
-        case closed: {
+        case closed:
-            TransactionID txn = storage->beginReadOnlyTransaction();
+            storage->openCursorTransaction(&cursor);
-            int result = storage->_mdbCursorOpen(txn, &cursor);
-            if (result != MDB_SUCCESS)
-                storage->throwUnknown(result, txn);
-
-            storage->transactionStarted(txn, true);
             state = openedPrivate;
-        } break;
+        break;
         default:
             break;
     }
@@ -250,6 +265,7 @@ void LMDBAL::Cursor<K, V>::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<K, V>::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<K, V>::renew (const Transaction& transaction) {
     TransactionID txn = storage->extractTransactionId(transaction, renewCursorMethodName);
     switch (state) {
         case openedPrivate: {
-            TransactionID txn = storage->_mdbCursorTxn(cursor);
+            TransactionID txnOld = storage->_mdbCursorTxn(cursor);
-            storage->abortTransaction(txn);
+            storage->abortTransaction(txnOld);
-            storage->transactionAborted(txn);
+            storage->transactionAborted(txnOld);
-            [[fallthrough]];
+
-        }
-        case openedPublic: {
             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<K, V>::renew (const Transaction& transaction) {
 template<class K, class V>
 void LMDBAL::Cursor<K, V>::close () {
     switch (state) {
-        case openedPublic: {
+        case openedPublic:
+            storage->disconnectCursorFromTransaction(id, cursor);
             storage->_mdbCursorClose(cursor);
 
             state = closed;
-        } break;
+        break;
-        case openedPrivate: {
+        case openedPrivate:
-            TransactionID txn = storage->_mdbCursorTxn(cursor);
+            storage->closeCursorTransaction(cursor);
-            storage->_mdbCursorClose(cursor);
-            storage->abortTransaction(txn);
-            storage->transactionAborted(txn);
 
             state = closed;
-        } break;
+        break;
         default:
             break;
     }

src/icursor.h

@@ -0,0 +1,32 @@
+/*
+ * LMDB Abstraction Layer.
+ * Copyright (C) 2023  Yury Gubich <blue@macaw.me>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+namespace LMDBAL {
+
+class Transaction;
+
+class iCursor {
+    friend class Transaction;
+protected:
+    virtual ~iCursor() {}
+    virtual void terminated() = 0;
+};
+
+}

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)
  *

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*/
+    MDB_dbi dbi;                /**<\brief lmdb storage handle*/
-    Base* db;                                                                   /**<\brief parent database pointer (borrowed)*/
+    Base* db;                   /**<\brief parent database pointer (borrowed)*/
-    const std::string name;                                                     /**<\brief this storage name*/
+    const std::string name;     /**<\brief this storage name*/
-    const bool duplicates;                                                      /**<\brief true if storage supports duplicates*/
+    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 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 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 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 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 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 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 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 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 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 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 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 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 <class K, class V>

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<const uint32_t, iCursor*>& 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<Base*>(parent)->commitTransaction(txn);
-
         parent->transactions.erase(txn);
         reset();
     }

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*/
+    TransactionID txn;                      /**<\brief Transaction inner handler*/
-    bool active;                /**<\brief Transaction state*/
+    bool active;                            /**<\brief Transaction state*/
-    const Base* parent;         /**<\brief Pointer to the database this transaction belongs to*/
+    const Base* parent;                     /**<\brief Pointer to the database this transaction belongs to*/
+    std::map<uint32_t, iCursor*> cursors;   /**<\brief a collection of cursors curently opened under this transaction*/
 };
 
 class WriteTransaction : public Transaction {

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<uint64_t, std::string> emptyCursor = emptyCache->createCursor();

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<uint64_t, std::string> 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<uint64_t, std::string> emptyCursor = table->createCursor();
+
+    {
+        LMDBAL::Transaction txn = db->beginReadOnlyTransaction();
+        cursor.open(txn);
+        EXPECT_NO_THROW(cursor.first());
+    }
+
+    EXPECT_FALSE(cursor.opened());
 }