Using `py::keep_alive<N, P>()` may cause `internals.patients[nurse]` to grow without bounds

[EricCousineau-TRI]

Looking at some of the internals, it looks like return_value_policy::reference_internal is only "activated" when an instance is created newly registered:
https://github.com/pybind/pybind11/blob/48e1f9a/include/pybind11/cast.h#L552

However, py::keep_alive<N, P>() looks like it is always activated:
https://github.com/pybind/pybind11/blob/48e1f9a/include/pybind11/pybind11.h#L140
https://github.com/pybind/pybind11/blob/48e1f9a/include/pybind11/attr.h#L442
https://github.com/pybind/pybind11/blob/48e1f9a/include/pybind11/pybind11.h#L1470
https://github.com/pybind/pybind11/blob/48e1f9a/include/pybind11/detail/class.h#L289

A simple fix (for only pybind-registered objects) would be to add a simple check:

#include <algorithm>
...
auto& p = internals.patients[nurse];
if (std::find(p.begin(), p.end(), patient) != p.end()) ...

This would impact performance a little, but would prevent a packrat problem for long(er)-running Python programs.

Not sure about the non-pybind case in keep_alive_impl; however, it looks like this would just leak some reference counting, which may be less of an issue than an array growing.

[jagerman]

Ah, I see what you mean:

#include <pybind11/pybind11.h>
namespace py = pybind11;
struct Foo {};
PYBIND11_MODULE(ka, m) {
    m.def("f", [](py::object o1, py::object o2) {}, py::keep_alive<1, 2>());
    py::class_<Foo>(m, "Foo")
        .def(py::init<>())
        ;

    m.def("patients", [](py::object nurse) {
            auto &internals = py::detail::get_internals();
            py::list ret;
            for (auto p : internals.patients[nurse.ptr()]) {
                ret.append(py::reinterpret_borrow<py::object>(p));
            }
            return ret;
    });
}

results in:

>>> import ka
>>> nurse = ka.Foo()
>>> p1 = ka.Foo()
>>> p2 = ka.Foo()
>>> nurse
<ka.Foo object at 0x7f11a538ddc0>
>>> p1
<ka.Foo object at 0x7f11a53180d8>
>>> p2
<ka.Foo object at 0x7f11a5318148>
>>> ka.f(nurse, p1)
>>> ka.patients(nurse)
[<ka.Foo object at 0x7f11a53180d8>]
>>> ka.f(nurse, p1)
>>> ka.patients(nurse)
[<ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>]
>>> ka.f(nurse, p1)
>>> ka.patients(nurse)
[<ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>]
>>> ka.f(nurse, p1)
>>> ka.patients(nurse)
[<ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>]
>>> ka.f(nurse, p1)
>>> ka.patients(nurse)
[<ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>, <ka.Foo object at 0x7f11a53180d8>]

I think the best solution here is to change the std::vector<PyObject *> to an std::unordered_set<PyObject *>. I'm pretty reluctant to include that in 2.2.2, though (it would break the internals API).

I suppose we could include the linear search you proposed in 2.2.2, and separately fix it in master with an unordered_set.

[EricCousineau-TRI]

Awesome, thank you!

[bstaletic]

This looks resolved. Can we close this?

[YannickJadoul]

Actually, this doesn't seem resolved, when I try the given code:

$ python3.8
Python 3.8.0 (default, Oct 28 2019, 16:14:01) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from example import *
>>> nurse = Foo()
>>> p1 = Foo()
>>> p2 = Foo()
>>> nurse
<example.Foo object at 0x7fbb551ff970>
>>> p1
<example.Foo object at 0x7fbb551ffd30>
>>> p2
<example.Foo object at 0x7fbb551ffdb0>
>>> f(nurse, p1)
>>> patients(nurse)
[<example.Foo object at 0x7fbb551ffd30>]
>>> f(nurse, p1)
>>> patients(nurse)
[<example.Foo object at 0x7fbb551ffd30>, <example.Foo object at 0x7fbb551ffd30>]
>>> f(nurse, p1)
>>> f(nurse, p1)
>>> f(nurse, p1)
>>> patients(nurse)
[<example.Foo object at 0x7fbb551ffd30>, <example.Foo object at 0x7fbb551ffd30>, <example.Foo object at 0x7fbb551ffd30>, <example.Foo object at 0x7fbb551ffd30>, <example.Foo object at 0x7fbb551ffd30>]
>>> 

[EricCousineau-TRI]

Huh... Can I ask which commit you were checking this code on?
The merge of 56c1edb seems like it should've fixed it, and the unittest seems like it might as well...

Does the unittest need to be fixed?
(I will try it out on Friday / Sat)

[YannickJadoul]

@EricCousineau-TRI I'm on the current master, so f980d76.

I also updated the code to make sure I have the right pybind11 version, but that seems to be OK:

#include <pybind11/pybind11.h>

namespace py = pybind11;

struct Foo {};

#define XSTR(s) STR(s)
#define STR(s) #s

PYBIND11_MODULE(example, m) {
    m.def("f", [](py::object o1, py::object o2) {}, py::keep_alive<1, 2>());
    py::class_<Foo>(m, "Foo")
        .def(py::init<>())
        ;

    m.def("patients", [](py::object nurse) {
            auto &internals = py::detail::get_internals();
            py::list ret;
            for (auto p : internals.patients[nurse.ptr()]) {
                ret.append(py::reinterpret_borrow<py::object>(p));
            }
            return ret;
    });

    py::print(XSTR(PYBIND11_VERSION_MAJOR), XSTR(PYBIND11_VERSION_MINOR), XSTR(PYBIND11_VERSION_PATCH));
}

$ python3.8
Python 3.8.0 (default, Oct 28 2019, 16:14:01) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from example import *
2 5 dev1
>>> nurse = Foo()
>>> p1 = Food()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'Food' is not defined
>>> p1 = Foo()
>>> nurse, p1
(<example.Foo object at 0x7f7459e878b0>, <example.Foo object at 0x7f7459e87eb0>)
>>> f(nurse, p1)
>>> patients(nurse)
[<example.Foo object at 0x7f7459e87eb0>]
>>> f(nurse, p1)
>>> patients(nurse)
[<example.Foo object at 0x7f7459e87eb0>, <example.Foo object at 0x7f7459e87eb0>]
>>> f(nurse, p1)
>>> f(nurse, p1)
>>> f(nurse, p1)
>>> patients(nurse)
[<example.Foo object at 0x7f7459e87eb0>, <example.Foo object at 0x7f7459e87eb0>, <example.Foo object at 0x7f7459e87eb0>, <example.Foo object at 0x7f7459e87eb0>, <example.Foo object at 0x7f7459e87eb0>]
>>> 

[EricCousineau-TRI]

Gotcha, thanks!
Looking into 56c1edb, I only just now realized that this is part of tag v2.2.2, branch v2.2 that diverges from master after 1caeb8d -- which Wenzel said as much in the commit -- so that's why it doesn't work on master -- d'oh! :P

FTR @wjakob commented on PR #1253 10 days ago, but Jason may be busy so I'll see if I can update the PR / make a replacement PR.