From e11d65e86b702a05bcf29ade2c84470eb51eaf77 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sat, 13 Jan 2018 18:05:04 -0400 Subject: [PATCH 1/5] Track patients with unordered_set rather than vector The stored vector of pybind-registered-type patients can grow without bound if a function is called with the same patient multiple times. This commit changes the pybind internal patient storage to an `unordered_set` to avoid the issue. Fixes #1251 --- include/pybind11/detail/class.h | 8 ++++---- include/pybind11/detail/internals.h | 4 ++-- tests/test_call_policies.cpp | 16 +++++++++++++++ tests/test_call_policies.py | 31 ++++++++++++++++++++++++++++- 4 files changed, 52 insertions(+), 7 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 7a5dd0130d..6cb42cd930 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -290,8 +290,8 @@ inline void add_patient(PyObject *nurse, PyObject *patient) { auto &internals = get_internals(); auto instance = reinterpret_cast(nurse); instance->has_patients = true; - Py_INCREF(patient); - internals.patients[nurse].push_back(patient); + auto it = internals.patients[nurse].insert(patient); + if (it.second) Py_INCREF(patient); } inline void clear_patients(PyObject *self) { @@ -300,12 +300,12 @@ inline void clear_patients(PyObject *self) { auto pos = internals.patients.find(self); assert(pos != internals.patients.end()); // Clearing the patients can cause more Python code to run, which - // can invalidate the iterator. Extract the vector of patients + // can invalidate the iterator. Extract the set of patients // from the unordered_map first. auto patients = std::move(pos->second); internals.patients.erase(pos); instance->has_patients = false; - for (PyObject *&patient : patients) + for (PyObject *patient : patients) Py_CLEAR(patient); } diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index e39f38695f..a15b34d9f8 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -70,7 +70,7 @@ struct internals { std::unordered_multimap registered_instances; // void * -> instance* std::unordered_set, overload_hash> inactive_overload_cache; type_map> direct_conversions; - std::unordered_map> patients; + std::unordered_map> patients; std::forward_list registered_exception_translators; std::unordered_map shared_data; // Custom data to be shared across extensions std::vector loader_patient_stack; // Used by `loader_life_support` @@ -111,7 +111,7 @@ struct type_info { }; /// Tracks the `internals` and `type_info` ABI version independent of the main library version -#define PYBIND11_INTERNALS_VERSION 1 +#define PYBIND11_INTERNALS_VERSION 2 #if defined(WITH_THREAD) # define PYBIND11_INTERNALS_KIND "" diff --git a/tests/test_call_policies.cpp b/tests/test_call_policies.cpp index fd24557834..9577d67637 100644 --- a/tests/test_call_policies.cpp +++ b/tests/test_call_policies.cpp @@ -8,6 +8,7 @@ */ #include "pybind11_tests.h" +#include "constructor_stats.h" struct CustomGuard { static bool enabled; @@ -61,6 +62,21 @@ TEST_SUBMODULE(call_policies, m) { .def("returnNullChildKeepAliveChild", &Parent::returnNullChild, py::keep_alive<1, 0>()) .def("returnNullChildKeepAliveParent", &Parent::returnNullChild, py::keep_alive<0, 1>()); + // test_keep_alive_single + m.def("add_patient", [](py::object /*nurse*/, py::object /*patient*/) { }, py::keep_alive<1, 2>()); + m.def("get_patients", [](py::object nurse) { + py::set patients; + for (PyObject *p : pybind11::detail::get_internals().patients[nurse.ptr()]) + patients.add(py::reinterpret_borrow(p)); + return patients; + }); + m.def("refcount", [](py::handle h) { +#ifdef PYPY_VERSION + ConstructorStats::gc(); // PyPy doesn't update ref counts until GC occurs +#endif + return h.ref_count(); + }); + #if !defined(PYPY_VERSION) // test_alive_gc class ParentGC : public Parent { diff --git a/tests/test_call_policies.py b/tests/test_call_policies.py index 7c835599c2..6700ef5435 100644 --- a/tests/test_call_policies.py +++ b/tests/test_call_policies.py @@ -1,6 +1,6 @@ import pytest from pybind11_tests import call_policies as m -from pybind11_tests import ConstructorStats +from pybind11_tests import ConstructorStats, UserType def test_keep_alive_argument(capture): @@ -69,6 +69,35 @@ def test_keep_alive_return_value(capture): """ +def test_keep_alive_single(): + """Issue #1251 - patients are stored multiple times when given to the same nurse""" + + nurse, p1, p2 = UserType(), UserType(), UserType() + b = m.refcount(nurse) + assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b, b] + m.add_patient(nurse, p1) + assert m.get_patients(nurse) == {p1, } + assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b] + m.add_patient(nurse, p1) + assert m.get_patients(nurse) == {p1, } + assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b] + m.add_patient(nurse, p1) + assert m.get_patients(nurse) == {p1, } + assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b] + m.add_patient(nurse, p2) + assert m.get_patients(nurse) == {p1, p2} + assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b + 1] + m.add_patient(nurse, p2) + assert m.get_patients(nurse) == {p1, p2} + assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b + 1] + m.add_patient(nurse, p2) + m.add_patient(nurse, p1) + assert m.get_patients(nurse) == {p1, p2} + assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b + 1] + del nurse + assert [m.refcount(p1), m.refcount(p2)] == [b, b] + + # https://bitbucket.org/pypy/pypy/issues/2447 @pytest.unsupported_on_pypy def test_alive_gc(capture): From 3fb4858be7afe6ed75e22c0026c480c3ea5f55f9 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Fri, 10 Jul 2020 12:02:58 -0400 Subject: [PATCH 2/5] test_call_policies: Minor modifications - Checks `not m.has_patients()` after nurse is gc'd - Uses Python implementation of `refcount` --- tests/test_call_policies.cpp | 10 +++++----- tests/test_call_policies.py | 30 +++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/tests/test_call_policies.cpp b/tests/test_call_policies.cpp index 9577d67637..ce0e28fdd2 100644 --- a/tests/test_call_policies.cpp +++ b/tests/test_call_policies.cpp @@ -70,11 +70,11 @@ TEST_SUBMODULE(call_policies, m) { patients.add(py::reinterpret_borrow(p)); return patients; }); - m.def("refcount", [](py::handle h) { -#ifdef PYPY_VERSION - ConstructorStats::gc(); // PyPy doesn't update ref counts until GC occurs -#endif - return h.ref_count(); + m.def("has_patients", [](uint64_t nurse_id) { + // This assumes that id() and PyObject* are equivalent. + // We use this to allow the original `nurse` object to be garbage collected. + PyObject *nurse_ptr = (PyObject*)nurse_id; + return pybind11::detail::get_internals().patients.count(nurse_ptr); }); #if !defined(PYPY_VERSION) diff --git a/tests/test_call_policies.py b/tests/test_call_policies.py index 6700ef5435..ee23621c3e 100644 --- a/tests/test_call_policies.py +++ b/tests/test_call_policies.py @@ -1,3 +1,5 @@ +import sys + import pytest from pybind11_tests import call_policies as m from pybind11_tests import ConstructorStats, UserType @@ -69,33 +71,43 @@ def test_keep_alive_return_value(capture): """ +def refcount(h): + pytest.gc_collect() + return sys.getrefcount(h) + + def test_keep_alive_single(): """Issue #1251 - patients are stored multiple times when given to the same nurse""" nurse, p1, p2 = UserType(), UserType(), UserType() - b = m.refcount(nurse) - assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b, b] + b = refcount(nurse) + nurse_id = id(nurse) + assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b, b] m.add_patient(nurse, p1) assert m.get_patients(nurse) == {p1, } - assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b] + assert m.has_patients(nurse_id) + assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b] m.add_patient(nurse, p1) assert m.get_patients(nurse) == {p1, } - assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b] + assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b] m.add_patient(nurse, p1) assert m.get_patients(nurse) == {p1, } - assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b] + assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b] m.add_patient(nurse, p2) assert m.get_patients(nurse) == {p1, p2} - assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b + 1] + assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b + 1] m.add_patient(nurse, p2) assert m.get_patients(nurse) == {p1, p2} - assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b + 1] + assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b + 1] m.add_patient(nurse, p2) m.add_patient(nurse, p1) assert m.get_patients(nurse) == {p1, p2} - assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] == [b, b + 1, b + 1] + assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b + 1] del nurse - assert [m.refcount(p1), m.refcount(p2)] == [b, b] + pytest.gc_collect() + assert not m.has_patients(nurse_id) + # Ensure that nurse entry is removed once it goes out of scope. + assert [refcount(p1), refcount(p2)] == [b, b] # https://bitbucket.org/pypy/pypy/issues/2447 From f28f6420da2d13af1f46d4a523d424b5f7ffe6d5 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 9 Aug 2021 17:16:16 +0000 Subject: [PATCH 3/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_call_policies.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/test_call_policies.py b/tests/test_call_policies.py index 71ea32740f..5fa9181da1 100644 --- a/tests/test_call_policies.py +++ b/tests/test_call_policies.py @@ -131,14 +131,20 @@ def test_keep_alive_single(): nurse_id = id(nurse) assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b, b] m.add_patient(nurse, p1) - assert m.get_patients(nurse) == {p1, } + assert m.get_patients(nurse) == { + p1, + } assert m.has_patients(nurse_id) assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b] m.add_patient(nurse, p1) - assert m.get_patients(nurse) == {p1, } + assert m.get_patients(nurse) == { + p1, + } assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b] m.add_patient(nurse, p1) - assert m.get_patients(nurse) == {p1, } + assert m.get_patients(nurse) == { + p1, + } assert [refcount(nurse), refcount(p1), refcount(p2)] == [b, b + 1, b] m.add_patient(nurse, p2) assert m.get_patients(nurse) == {p1, p2} @@ -156,7 +162,7 @@ def test_keep_alive_single(): # Ensure that nurse entry is removed once it goes out of scope. assert [refcount(p1), refcount(p2)] == [b, b] - + # https://foss.heptapod.net/pypy/pypy/-/issues/2447 @pytest.mark.xfail("env.PYPY", reason="_PyObject_GetDictPtr is unimplemented") def test_alive_gc(capture): From e6a5e9b2167cb31a125c31084ccf5f0be2460fa2 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 1 Sep 2021 16:08:53 -0400 Subject: [PATCH 4/5] Skip test on PYPY --- tests/test_call_policies.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_call_policies.py b/tests/test_call_policies.py index 5fa9181da1..5ab4416bb7 100644 --- a/tests/test_call_policies.py +++ b/tests/test_call_policies.py @@ -123,6 +123,7 @@ def refcount(h): return sys.getrefcount(h) +@pytest.mark.xfail("env.PYPY", reason="getrefcount is unimplemented") def test_keep_alive_single(): """Issue #1251 - patients are stored multiple times when given to the same nurse""" From 67e1337aed9baee931de0f61a9187a884e9b2084 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 1 Sep 2021 20:10:04 +0000 Subject: [PATCH 5/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_call_policies.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_call_policies.py b/tests/test_call_policies.py index 38ddfbb8f3..6a9e8159b7 100644 --- a/tests/test_call_policies.py +++ b/tests/test_call_policies.py @@ -4,7 +4,6 @@ import pytest import env # noqa: F401 - from pybind11_tests import ConstructorStats, UserType from pybind11_tests import call_policies as m