Pybind11: 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 56c1edb46d2d02e7c8fe0ad3fac53e2f24f18111 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 f980d76d38dda8e9ce78e68e27b4c2026681bf51.

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 1caeb8d7 -- 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.

[YannickJadoul]

Ah, thanks for checking. I hadn't realized this was the cause! Would be great to get this in for some future release.

But once again, this would break ABI, so it would need to be a 2.6 release? Can we meanwhile cherry-pick the v2.2 changes onto master so this would be included in a hypothetical patch release that would happen before a 2.6 release?

[EricCousineau-TRI]

But once again, this would break ABI, so it would need to be a 2.6 release? [...]

Probably! There is the PYBIND11_INTERNALS_VERSION which Jason had incremented.
Yannick, would you be able to take a glace at the PR and see if there's anything fishy looking?

I will defer to @wjakob on merging since it has downstream implications that you mentioned.
Wenzel, might you be able to take a gander at some point at the updated PR?

Can we meanwhile cherry-pick the v2.2 changes [...]

Overall, my pref would be that we take #1253 on wholesale onto master and avoid #2289, if possible.
Rationale is that I don't think there's an issue with having master introduce ABI breakages.

My understanding is that yes, it's ABI-incompatible, but given the pybind11 encapsulates all internals according to this internal version flag, the libraries will be able to co-exist, they just won't be able to interface Python <-> C++ with each other.

However, if you're intermingling bleeding-edge version of pybind11 on master, not recompiling upstream bindings from source, and expecting them to work, I feel like that may be a bit... sketchy?
(Please correct me if I'm wrong!)

FTR I've filed #2289 as the forward-port, though. I realized the time it took me to type this out was about 3x longer than filing the cherry-pick :P

[wjakob]

I am currently crunching to prepare a keynote talk for a conference next week (Wednesday), I'll take a look after that.

[EricCousineau-TRI]

Sounds great, thank you!!!

[YannickJadoul]

Yannick, would you be able to take a glace at the PR and see if there's anything fishy looking?

Will do!

Overall, my pref would be that we take #1253 on wholesale onto master and avoid #2289, if possible.
Rationale is that I don't _think_ there's an issue with having master introduce ABI breakages.

My understanding is that yes, it's ABI-incompatible, but given the pybind11 encapsulates all internals according to this internal version flag, the libraries will be able to co-exist, they just won't be able to interface Python <-> C++ with each other.

No, but it might matter for determining what's the next increment in version number?

At any rate, I don't have too strong opinions, but if I read this correctly, this was the reason there were these two separate PRs 2 years ago? (Of which unfortunately only one got merged. Maybe we should have an issue that keeps track of which PRs to merge for the next minor version increase?)

[EricCousineau-TRI]

At any rate, I don't have too strong opinions, but if I read this correctly, this was the reason there were these two separate PRs 2 years ago?

True! And yeah, I too am ambivalent, but mainly b/c I'm not sure of the full implications. I will wait for Wenzel's feedback on the ABI-breaking PR after Wed.

Maybe we should have an issue that keeps track of which PRs to merge for the next minor version increase?

Perhaps we can use GitHub's Milestones feature?
Example from another project: https://github.com/osrf/sdformat/milestone/1

[YannickJadoul]

Perhaps we can use GitHub's Milestones feature?

That's what I meant. I just couldn't think of how it was called again :-) I'd be up for that, yes; worth discussing.