From adfd3e170025e7ea628222f341ec1da5671a8f28 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 22 Jun 2021 11:01:48 -0700 Subject: [PATCH] Partial cleanup of tests. WIP. The cleanup uncovered a major problem. --- include/pybind11/detail/smart_holder_poc.h | 5 +- .../detail/smart_holder_type_casters.h | 4 +- ...t_class_sh_trampoline_shared_from_this.cpp | 21 +-- ...st_class_sh_trampoline_shared_from_this.py | 136 ++++++++++++------ 4 files changed, 113 insertions(+), 53 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index e0e7623ce..76675bd86 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -52,7 +52,8 @@ Details: #include //#include -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 index 1e5cc152a..6ebc06c52 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -397,7 +397,6 @@ to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + " auto void_raw_ptr = hld.template as_raw_ptr_unowned(); auto type_raw_ptr = convert_type(void_raw_ptr); if (hld.pointee_depends_on_holder_owner) { - auto self = reinterpret_cast(load_impl.loaded_v_h.inst); auto vptr_gd_ptr = std::get_deleter(hld.vptr); if (vptr_gd_ptr != nullptr) { assert(!hld.vptr_is_released); @@ -405,6 +404,7 @@ to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + " std::shared_ptr non_owning( hld.vptr.get(), pybindit::memory::noop_deleter_acting_as_weak_ptr_owner{hld.vptr}); + auto self = reinterpret_cast(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; @@ -413,6 +413,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< @@ -421,6 +422,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(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 index 40afaf095..439a01d6e 100644 --- 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 { 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> stash; explicit SftSharedPtrStash(int ser_no) : ser_no{ser_no} {} void Add(const std::shared_ptr &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 &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 index cab007973..c654439da 100644 --- 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():