Partial cleanup of tests. WIP. The cleanup uncovered a major problem.

rwgk · rwgk · commit 37dbb55dd8e7 · 2021-06-22T11:01:48.000-07:00
diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h
@@ -52,7 +52,8 @@ High-level aspects:
 #include <typeinfo>
 
 //#include <iostream>
-inline void to_cout(std::string /*msg*/) { /*std::cout << msg << std::endl;*/ }
+//inline void to_cout(std::string msg) { std::cout << msg << std::endl; }
+inline void to_cout(std::string) { }
 
 // pybindit = Python Bindings Innovation Track.
 // Currently not in pybind11 namespace to signal that this POC does not depend
@@ -366,7 +367,7 @@ to_cout("LOOOK smart_holder as_shared_ptr " + std::to_string(__LINE__) + " " + _
 
 inline void guarded_delete::operator()(void *raw_ptr) const {
     if (hld) {
-to_cout("LOOOK guarded_delete call hld " + std::to_string(__LINE__) + " " + __FILE__);
+to_cout("RECLAIM guarded_delete call hld " + std::to_string(__LINE__) + " " + __FILE__);
         assert(armed_flag);
         assert(hld->vptr.get() == raw_ptr);
         assert(hld->vptr_is_released);
diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h
@@ -393,14 +393,14 @@ to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + "
         auto void_raw_ptr = hld.template as_raw_ptr_unowned<void>();
         auto type_raw_ptr = convert_type(void_raw_ptr);
         if (hld.pointee_depends_on_holder_owner) {
-            auto self         = reinterpret_cast<PyObject *>(load_impl.loaded_v_h.inst);
             auto vptr_gd_ptr = std::get_deleter<pybindit::memory::guarded_delete>(hld.vptr);
             if (vptr_gd_ptr != nullptr) {
                 assert(!hld.vptr_is_released);
                 std::shared_ptr<T> to_be_returned(hld.vptr, type_raw_ptr);
                 std::shared_ptr<void> non_owning(
                     hld.vptr.get(),
                     pybindit::memory::noop_deleter_acting_as_weak_ptr_owner{hld.vptr});
+                auto self = reinterpret_cast<PyObject *>(load_impl.loaded_v_h.inst);
                 // Critical transfer-of-ownership section. This must stay together.
                 vptr_gd_ptr->hld          = &hld;
                 vptr_gd_ptr->callback_ptr = dec_ref_void;
@@ -409,6 +409,7 @@ to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + "
                 hld.vptr_is_released = true;
                 Py_INCREF(self);
                 // Critical section end.
+to_cout("FRESHLY released");
                 return to_be_returned;
             }
             auto vptr_ndaawp_ptr = std::get_deleter<
@@ -417,6 +418,7 @@ to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + "
                 assert(hld.vptr_is_released);
                 auto released_vptr = vptr_ndaawp_ptr->passenger.lock();
                 if (released_vptr != nullptr) {
+to_cout("retrieved released");
                     return std::shared_ptr<T>(released_vptr, type_raw_ptr);
                 }
             }
diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp
@@ -13,13 +13,7 @@ namespace {
 struct Sft : std::enable_shared_from_this<Sft> {
     std::string history;
     explicit Sft(const std::string &history) : history{history} {}
-    long use_count() const {
-#if defined(__cpp_lib_enable_shared_from_this) && (!defined(_MSC_VER) || _MSC_VER >= 1912)
-        return this->shared_from_this().use_count();
-#else
-        return -1;
-#endif
-    }
+    long use_count() const { return this->shared_from_this().use_count() - 1; }
     virtual ~Sft() = default;
 
 #if defined(__clang__)
@@ -50,12 +44,16 @@ struct SftSharedPtrStash {
     std::vector<std::shared_ptr<Sft>> stash;
     explicit SftSharedPtrStash(int ser_no) : ser_no{ser_no} {}
     void Add(const std::shared_ptr<Sft> &obj) {
-        obj->history += "_Stash" + std::to_string(ser_no) + "Add";
+        if (obj->history.size()) {
+            obj->history += "_Stash" + std::to_string(ser_no) + "Add";
+        }
         stash.push_back(obj);
     }
     void AddSharedFromThis(Sft *obj) {
         auto sft = obj->shared_from_this();
-        sft->history += "_Stash" + std::to_string(ser_no) + "AddSharedFromThis";
+        if (sft->history.size()) {
+            sft->history += "_Stash" + std::to_string(ser_no) + "AddSharedFromThis";
+        }
         stash.push_back(sft);
     }
     std::string history(unsigned i) {
@@ -76,7 +74,9 @@ struct SftTrampoline : Sft, py::trampoline_self_life_support {
 
 long pass_shared_ptr(const std::shared_ptr<Sft> &obj) {
     auto sft = obj->shared_from_this();
-    sft->history += "_PassSharedPtr";
+    if (sft->history.size()) {
+        sft->history += "_PassSharedPtr";
+    }
     return sft.use_count();
 }
 
@@ -102,4 +102,5 @@ TEST_SUBMODULE(class_sh_trampoline_shared_from_this, m) {
 
     m.def("pass_shared_ptr", pass_shared_ptr);
     m.def("pass_unique_ptr", pass_unique_ptr);
+    m.def("to_cout", to_cout);
 }
diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py
@@ -1,79 +1,89 @@
 # -*- coding: utf-8 -*-
 import pytest
 
-import env  # noqa: F401
-
 import pybind11_tests.class_sh_trampoline_shared_from_this as m
 
-import gc
-import weakref
-
 
 class PySft(m.Sft):
     pass
 
 
-def test_pass_shared_ptr():
+def test_release_and_immediate_reclaim():
     obj = PySft("PySft")
     assert obj.history == "PySft"
-    assert obj.use_count() in [2, -1]  # TODO: Be smarter/stricter.
-    m.pass_shared_ptr(obj)
+    assert obj.use_count() == 1
+    assert m.pass_shared_ptr(obj) == 2
     assert obj.history == "PySft_PassSharedPtr"
-    assert obj.use_count() in [2, -1]
-    uc = m.pass_shared_ptr(obj)
-    assert uc == 2  # +1 for passed argument, +1 for shared_from_this.
+    assert obj.use_count() == 1
+    assert m.pass_shared_ptr(obj) == 2
     assert obj.history == "PySft_PassSharedPtr_PassSharedPtr"
-    assert obj.use_count() in [2, -1]
+    assert obj.use_count() == 1
+
+    obj = PySft("")
+    while True:
+        m.pass_shared_ptr(obj)
+        assert obj.history == ""
+        assert obj.use_count() == 1
+        break  # Comment out for manual leak checking (use `top` command).
 
 
-def test_pass_shared_ptr_while_stashed():
+def test_release_to_cpp_stash():
     obj = PySft("PySft")
-    obj_wr = weakref.ref(obj)
     stash1 = m.SftSharedPtrStash(1)
     stash1.Add(obj)
     assert obj.history == "PySft_Stash1Add"
-    assert obj.use_count() in [2, -1]
+    assert obj.use_count() == 1
     assert stash1.history(0) == "PySft_Stash1Add"
     assert stash1.use_count(0) == 1  # obj does NOT own the shared_ptr anymore.
-    uc = m.pass_shared_ptr(obj)
-    assert uc == 3  # +1 for passed argument, +1 for shared_from_this.
+    assert m.pass_shared_ptr(obj) == 3
     assert obj.history == "PySft_Stash1Add_PassSharedPtr"
-    assert obj.use_count() in [2, -1]
+    assert obj.use_count() == 1
     assert stash1.history(0) == "PySft_Stash1Add_PassSharedPtr"
     assert stash1.use_count(0) == 1
     stash2 = m.SftSharedPtrStash(2)
     stash2.Add(obj)
     assert obj.history == "PySft_Stash1Add_PassSharedPtr_Stash2Add"
-    assert obj.use_count() in [3, -1]
+    assert obj.use_count() == 2
     assert stash2.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add"
     assert stash2.use_count(0) == 2
     stash2.Add(obj)
-    assert obj.history == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
-    assert obj.use_count() in [4, -1]
+    exp_oh = "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
+    assert obj.history == exp_oh
+    assert obj.use_count() == 3
+    assert stash1.history(0) == exp_oh
     assert stash1.use_count(0) == 3
-    assert stash1.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
+    assert stash2.history(0) == exp_oh
     assert stash2.use_count(0) == 3
-    assert stash2.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
+    assert stash2.history(1) == exp_oh
     assert stash2.use_count(1) == 3
-    assert stash2.history(1) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
     del obj
+    assert stash2.history(0) == exp_oh
     assert stash2.use_count(0) == 3
-    assert stash2.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
+    assert stash2.history(1) == exp_oh
     assert stash2.use_count(1) == 3
-    assert stash2.history(1) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
     del stash2
-    gc.collect()
-    assert obj_wr() is not None
-    assert stash1.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add"
-    del stash1
-    gc.collect()
-    if not env.PYPY:
-        assert obj_wr() is None
+    assert stash1.history(0) == exp_oh
+    assert stash1.use_count(0) == 1
 
 
-def test_pass_shared_ptr_while_stashed_with_shared_from_this():
+def test_release_to_cpp_stash_leak():
+    obj = PySft("")
+    while True:
+        stash1 = m.SftSharedPtrStash(1)
+        stash1.Add(obj)
+        assert obj.history == ""
+        assert obj.use_count() == 1
+        assert stash1.use_count(0) == 1
+        stash1.Add(obj)
+        assert obj.history == ""
+        assert obj.use_count() == 2
+        assert stash1.use_count(0) == 2
+        assert stash1.use_count(1) == 2
+        break  # Comment out for manual leak checking (use `top` command).
+
+
+def test_release_to_cpp_stash_via_shared_from_this():
     obj = PySft("PySft")
-    obj_wr = weakref.ref(obj)
     stash1 = m.SftSharedPtrStash(1)
     stash1.AddSharedFromThis(obj)
     assert obj.history == "PySft_Stash1AddSharedFromThis"
@@ -82,11 +92,57 @@ def test_pass_shared_ptr_while_stashed_with_shared_from_this():
     assert obj.history == "PySft_Stash1AddSharedFromThis_Stash1AddSharedFromThis"
     assert stash1.use_count(0) == 3
     assert stash1.use_count(1) == 3
-    del obj
-    del stash1
-    gc.collect()
-    if not env.PYPY:
-        assert obj_wr() is None
+
+
+def test_release_to_cpp_stash_via_shared_from_this_leak_1():  # WIP
+    m.to_cout("")
+    m.to_cout("")
+    m.to_cout("Add first")
+    obj = PySft("")
+    import weakref
+
+    obj_wr = weakref.ref(obj)
+    while True:
+        stash1 = m.SftSharedPtrStash(1)
+        stash1.Add(obj)
+        assert obj.history == ""
+        assert obj.use_count() == 1
+        assert stash1.use_count(0) == 1
+        stash1.AddSharedFromThis(obj)
+        assert obj.history == ""
+        assert obj.use_count() == 2
+        assert stash1.use_count(0) == 2
+        assert stash1.use_count(1) == 2
+        del obj
+        assert obj_wr() is not None
+        assert stash1.use_count(0) == 2
+        assert stash1.use_count(1) == 2
+        break  # Comment out for manual leak checking (use `top` command).
+
+
+def test_release_to_cpp_stash_via_shared_from_this_leak_2():  # WIP
+    m.to_cout("")
+    m.to_cout("AddSharedFromThis only")
+    obj = PySft("")
+    import weakref
+
+    obj_wr = weakref.ref(obj)
+    while True:
+        stash1 = m.SftSharedPtrStash(1)
+        stash1.AddSharedFromThis(obj)
+        assert obj.history == ""
+        assert obj.use_count() == 2
+        assert stash1.use_count(0) == 2
+        stash1.AddSharedFromThis(obj)
+        assert obj.history == ""
+        assert obj.use_count() == 3
+        assert stash1.use_count(0) == 3
+        assert stash1.use_count(1) == 3
+        del obj
+        assert obj_wr() is None  # BAD NEEDS FIXING
+        assert stash1.use_count(0) == 2
+        assert stash1.use_count(1) == 2
+        break  # Comment out for manual leak checking (use `top` command).
 
 
 def test_pass_released_shared_ptr_as_unique_ptr():
