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 pybind#1251

Author: jagerman

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<detail::instance *>(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);
 }
 

include/pybind11/detail/internals.h

@@ -70,7 +70,7 @@ struct internals {
     std::unordered_multimap<const void *, instance*> registered_instances; // void * -> instance*
     std::unordered_set<std::pair<const PyObject *, const char *>, overload_hash> inactive_overload_cache;
     type_map<std::vector<bool (*)(PyObject *, void *&)>> direct_conversions;
-    std::unordered_map<const PyObject *, std::vector<PyObject *>> patients;
+    std::unordered_map<const PyObject *, std::unordered_set<PyObject *>> patients;
     std::forward_list<void (*) (std::exception_ptr)> registered_exception_translators;
     std::unordered_map<std::string, void *> shared_data; // Custom data to be shared across extensions
     std::vector<PyObject *> 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 ""

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<py::object>(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 {

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,34 @@ 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()
+    assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] ==[3, 3, 3]
+    m.add_patient(nurse, p1)
+    assert m.get_patients(nurse) == {p1, }
+    assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] ==[3, 4, 3]
+    m.add_patient(nurse, p1)
+    assert m.get_patients(nurse) == {p1, }
+    assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] ==[3, 4, 3]
+    m.add_patient(nurse, p1)
+    assert m.get_patients(nurse) == {p1, }
+    assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] ==[3, 4, 3]
+    m.add_patient(nurse, p2)
+    assert m.get_patients(nurse) == {p1, p2}
+    assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] ==[3, 4, 4]
+    m.add_patient(nurse, p2)
+    assert m.get_patients(nurse) == {p1, p2}
+    assert [m.refcount(nurse), m.refcount(p1), m.refcount(p2)] ==[3, 4, 4]
+    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)] ==[3, 4, 4]
+    del nurse
+    assert [m.refcount(p1), m.refcount(p2)] == [3, 3]
+
+
 # https://bitbucket.org/pypy/pypy/issues/2447
 @pytest.unsupported_on_pypy
 def test_alive_gc(capture):