Bugfix/1402

python_tests/test_memo_no_cache.py

@@ -66,7 +66,7 @@ def test_excluding_no_cache_instances_from_P0_cache(db0_fixture):
     gc.collect()        
     final_cache_size = db0.get_cache_stats()["P_size"]["P0"]
     # make sure cache utilization is low
-    assert abs(final_cache_size - initial_cache_size) < (350 << 10)
+    assert abs(final_cache_size - initial_cache_size) < (400 << 10)
 
 
 def test_fetching_no_cache_objects(db0_fixture):

python_tests/test_memo_singleton.py

@@ -61,8 +61,8 @@ def test_find_singleton_static_scope(db0_fixture):
     assert db0.find_singleton(MemoDataPxSingleton) is None
     obj_1 = MemoDataPxSingleton(789)
     assert db0.find_singleton(MemoDataPxSingleton) is obj_1
-    
-    
+
+
 def test_find_singleton(db0_fixture):
     assert db0.find_singleton(MemoTestSingleton) is None    
     # create on default prefix
@@ -81,4 +81,4 @@ def test_singleton_with_migrations(db0_fixture):
 def test_assembling_dyn_prefix_function(db0_fixture):
     assert __dyn_prefix(MemoTestSingleton) is None
     assert __dyn_prefix(MemoScopedSingleton) is not None
-
+

src/dbzero/bindings/python/iter/PyObjectIterable.cpp

@@ -102,7 +102,7 @@ namespace db0::python
         auto py_iter = PyObjectIteratorDefault_new();
         py_iter->makeNew(py_iterable->ext().iter());
         if (auto *iterator_pool = fixture->tryGet<db0::object_model::ObjectIteratorPool>()) {
-            iterator_pool->add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get()));
+            iterator_pool->add(py_iter->getSharedPtr());
         }
         return py_iter.steal();
     }

src/dbzero/bindings/python/iter/PyObjectIterator.cpp

@@ -3,6 +3,7 @@
 
 #include "PyObjectIterator.hpp"
 #include <dbzero/object_model/tags/ObjectIterator.hpp>
+#include <dbzero/object_model/tags/ObjectIteratorPool.hpp>
 #include <dbzero/workspace/Workspace.hpp>
 #include <dbzero/object_model/tags/TagIndex.hpp>
 #include <dbzero/core/utils/base32.hpp>
@@ -23,6 +24,13 @@ namespace db0::python
     {
         PY_DEALLOC_GUARD();
         PY_API_FUNC
+        auto iterator_ptr = self->getSharedPtr();
+        if (!!iterator_ptr) {
+            auto fixture = iterator_ptr->getFixture();
+            if (auto *iterator_pool = fixture->tryGet<db0::object_model::ObjectIteratorPool>()) {
+                iterator_pool->remove(iterator_ptr.get());
+            }
+        }
         // destroy associated instance
         self->destroy();
         Py_TYPE(self)->tp_free((PyObject*)self);

src/dbzero/object_model/tags/ObjectIteratorPool.cpp

@@ -7,43 +7,43 @@
 namespace db0::object_model
 {
 
-    ObjectIterator *ObjectIteratorPool::getIterator(ObjectSharedExtPtr const &object)
+    std::shared_ptr<ObjectIterator> ObjectIteratorPool::getIterator(const ObjectWeakPtr &object)
     {
-        auto *py_iterator = object.get();
-        if (!py_iterator) {
-            return nullptr;
-        }
-        auto iterator_ptr = py_iterator->getSharedPtr();
-        return iterator_ptr.get();
+        return object.lock();
     }
 
-    void ObjectIteratorPool::add(ObjectSharedExtPtr object)
+    void ObjectIteratorPool::add(const std::shared_ptr<ObjectIterator> &object)
     {
         if (m_closed) {
             return;
         }
-        if (object.get() != nullptr) {
-            m_iterators.push_back(std::move(object));
+        if (!object) {
+            return;
+        }
+        m_iterators.insert_or_assign(object.get(), object);
+    }
+
+    void ObjectIteratorPool::remove(ObjectIterator *object_ptr)
+    {
+        if (!object_ptr) {
+            return;
         }
+        m_iterators.erase(object_ptr);
     }
 
     std::size_t ObjectIteratorPool::detach()
     {
         std::size_t detached_count = 0;
-        auto out = m_iterators.begin();
-        for (auto it = m_iterators.begin(); it != m_iterators.end(); ++it) {
-            auto *iterator = getIterator(*it);
+        for (auto it = m_iterators.begin(); it != m_iterators.end();) {
+            auto iterator = getIterator(it->second);
             if (!iterator) {
+                it = m_iterators.erase(it);
                 continue;
             }
             iterator->detach();
             ++detached_count;
-            if (out != it) {
-                *out = std::move(*it);
-            }
-            ++out;
+            ++it;
         }
-        m_iterators.erase(out, m_iterators.end());
         return detached_count;
     }
 
@@ -59,17 +59,13 @@ namespace db0::object_model
     std::size_t ObjectIteratorPool::cleanup()
     {
         auto old_size = m_iterators.size();
-        auto out = m_iterators.begin();
-        for (auto it = m_iterators.begin(); it != m_iterators.end(); ++it) {
-            if (!getIterator(*it)) {
-                continue;
-            }
-            if (out != it) {
-                *out = std::move(*it);
+        for (auto it = m_iterators.begin(); it != m_iterators.end();) {
+            if (!getIterator(it->second)) {
+                it = m_iterators.erase(it);
+            } else {
+                ++it;
             }
-            ++out;
         }
-        m_iterators.erase(out, m_iterators.end());
         return old_size - m_iterators.size();
     }
 

src/dbzero/object_model/tags/ObjectIteratorPool.hpp

@@ -5,22 +5,21 @@
 
 #include <cstddef>
 #include <cstdint>
-#include <vector>
-#include <dbzero/bindings/python/PyWrapper.hpp>
-#include <dbzero/bindings/python/shared_py_object.hpp>
+#include <memory>
+#include <unordered_map>
 
 namespace db0::object_model
 {
 
     class ObjectIterator;
-    using PyObjectIterator = db0::python::PySharedWrapper<ObjectIterator, false>;
 
     class ObjectIteratorPool
     {
     public:
-        using ObjectSharedExtPtr = db0::python::shared_py_object<PyObjectIterator *, true>;
+        using ObjectWeakPtr = std::weak_ptr<ObjectIterator>;
 
-        void add(ObjectSharedExtPtr object);
+        void add(const std::shared_ptr<ObjectIterator> &object);
+        void remove(ObjectIterator *object_ptr);
         std::size_t detach();
         std::size_t detach(std::uint64_t generation);
         std::size_t cleanup();
@@ -30,11 +29,11 @@ namespace db0::object_model
         bool isClosed() const;
 
     private:
-        std::vector<ObjectSharedExtPtr> m_iterators;
+        std::unordered_map<ObjectIterator *, ObjectWeakPtr> m_iterators;
         std::uint64_t m_detach_generation = 0;
         bool m_closed = false;
 
-        static ObjectIterator *getIterator(ObjectSharedExtPtr const &object);
+        static std::shared_ptr<ObjectIterator> getIterator(const ObjectWeakPtr &object);
     };
 
 }

tests/unit_tests/ObjectIteratorPoolTest.cpp

@@ -165,7 +165,7 @@ namespace tests
         DetachableUniqueAddressIterator *query_ptr = nullptr;
         auto py_iter = makePyIterator(fixture, query_ptr);
 
-        pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get()));
+        pool.add(py_iter->getSharedPtr());
 
         ASSERT_EQ(pool.size(), 1u);
         ASSERT_EQ(pool.detach(), 1u);
@@ -182,7 +182,7 @@ namespace tests
         DetachableUniqueAddressIterator *query_ptr = nullptr;
         auto py_iter = makePyIterator(fixture, query_ptr);
 
-        pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get()));
+        pool.add(py_iter->getSharedPtr());
 
         ASSERT_EQ(pool.detach(1), 1u);
         ASSERT_EQ(query_ptr->m_detach_count, 1u);
@@ -224,14 +224,30 @@ namespace tests
         DetachableUniqueAddressIterator *query_ptr = nullptr;
         auto py_iter = makePyIterator(fixture, query_ptr);
 
-        pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get()));
+        pool.add(py_iter->getSharedPtr());
         py_iter->reset();
 
         ASSERT_EQ(pool.cleanup(), 1u);
         ASSERT_EQ(pool.size(), 0u);
         workspace.close();
     }
 
+    TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolDropsDestroyedPythonIteratorsImmediately)
+    {
+        Workspace workspace("", {}, {}, {}, {}, pythonFixtureInitializer());
+        auto fixture = workspace.getFixture(prefix_name);
+        auto &pool = fixture->get<db0::object_model::ObjectIteratorPool>();
+        DetachableUniqueAddressIterator *query_ptr = nullptr;
+        auto py_iter = makePyIterator(fixture, query_ptr);
+
+        pool.add(py_iter->getSharedPtr());
+
+        ASSERT_EQ(pool.size(), 1u);
+        py_iter.reset();
+        ASSERT_EQ(pool.size(), 0u);
+        workspace.close();
+    }
+
     TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolDetachCompactsExpiredNativeIterators)
     {
         Workspace workspace("", {}, {}, {}, {}, pythonFixtureInitializer());
@@ -242,8 +258,8 @@ namespace tests
         auto live_iter = makePyIterator(fixture, live_query_ptr);
         auto expired_iter = makePyIterator(fixture, expired_query_ptr);
 
-        pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(live_iter.get()));
-        pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(expired_iter.get()));
+        pool.add(live_iter->getSharedPtr());
+        pool.add(expired_iter->getSharedPtr());
         expired_iter->reset();
 
         ASSERT_EQ(pool.detach(), 1u);
@@ -261,7 +277,7 @@ namespace tests
         auto py_iter = makePyIterator(fixture, query_ptr);
 
         pool.close();
-        pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get()));
+        pool.add(py_iter->getSharedPtr());
 
         ASSERT_TRUE(pool.isClosed());
         ASSERT_EQ(pool.size(), 0u);
@@ -270,6 +286,23 @@ namespace tests
         workspace.close();
     }
 
+    TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolRegistrationDoesNotIncreasePythonRefcount)
+    {
+        Workspace workspace("", {}, {}, {}, {}, pythonFixtureInitializer());
+        auto fixture = workspace.getFixture(prefix_name);
+        auto &pool = fixture->get<db0::object_model::ObjectIteratorPool>();
+        DetachableUniqueAddressIterator *query_ptr = nullptr;
+        auto py_iter = makePyIterator(fixture, query_ptr);
+        auto *py_object = reinterpret_cast<PyObject *>(py_iter.get());
+        auto refcount_before = Py_REFCNT(py_object);
+
+        pool.add(py_iter->getSharedPtr());
+
+        ASSERT_EQ(Py_REFCNT(py_object), refcount_before);
+        ASSERT_EQ(pool.size(), 1u);
+        workspace.close();
+    }
+
     TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolRegistersForLiveFixturesButNotSnapshots)
     {
         {