Dynamic attributes of objects stored as std::shared_ptr's in std::vector's

[lorenzo-rovigatti]

I would like to add dynamic attributes to objects stored as shared pointers in std::vector's. I have read the docs and I'm still not sure whether this is truly possible or no. However, while performing some tests I have discovered an inconsistency that pybind11 devs might be interested in.

This inconsistent behaviour can be reproduced with the following c++ code:

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>
#include <vector>
#include <memory>

namespace py = pybind11;

class Foo {
public:
	Foo() {}
	~Foo() {}
};

class Bar {
public:
	Bar() {
		foos.push_back(std::make_shared<Foo>());
	}
	~Bar() {}
 
	std::vector<std::shared_ptr<Foo>> foos;
};


PYBIND11_MODULE(pybug, m) {
	py::class_<Foo, std::shared_ptr<Foo>> foo(m, "Foo", py::dynamic_attr());
	foo
		.def(py::init<>());
	
	py::class_<Bar> bar(m, "Bar");
	bar
		.def(py::init<>())
		.def_readwrite("foos", &Bar::foos);
}

which can be compiled (at least on my Linux box) with

g++ pybug.cpp -shared --std=c++11 -fPIC `python3 -m pybind11 --includes` -o pybug`python3-config --extension-suffix`

Now, on the python side the following snippet works or not depending whether the line starting with myfoo is commented or not:

import pybug

bar = pybug.Bar()

myfoo = bar.foos[0] # if this line is commented out then the subsequent print line will raise an "AttributeError"
bar.foos[0].b = 2
print(bar.foos[0].b)

Is there some hidden caching going on here (as suggested by this answer)? Is it possible to add dynamic attributes to the instances stored in the foos vector without keeping an explicit reference to each of them?

[bstaletic]

I can't repro this with gcc or clang. I'm suspecting there was a typo or something when you have tried this. Feel free to reopen, if you still have a problem.

[lorenzo-rovigatti]

Hey @bstaletic , thanks for taking the time to investigate my report. I still experience this issue on a just-updated Ubuntu 20.04 box. This is the software I am using:

• Python 3.8.2
• pybind11 2.4.3
• g++ 9.3.0

If you need any other information please let me know.

[bstaletic]

The problem is that I can't reproduce your issue at all, from what you've posted originally. Note that I am using latest master. Even version 2.5.0 is fairly odd.

Can you clone the repo and try with "the latest and greatest"?

[lorenzo-rovigatti]

The same thing happens with the just-cloned version.
Just to be 100% sure, if you comment the myfoo = bar.foos[0] and run the .py script you don't get an AttributeError: 'pybug.Foo' object has no attribute 'b' error?

[bstaletic]

I don't even have myfoo = bar.foox[0] in my script.

[lorenzo-rovigatti]

I'm sorry but what do you mean? The point is that in my case the following script works only if the myfoo = bar.foos[0] is not commented out:

import pybug

bar = pybug.Bar()

myfoo = bar.foos[0] # if this line is commented out then the subsequent print line will raise an "AttributeError"
bar.foos[0].b = 2
print(bar.foos[0].b)

how are you testing the code otherwise?

[bstaletic]

Ah... I skipped print(). Assignment "works" either way. Let me see what's going on...

[YannickJadoul]

@lorenzo-rovigatti Here's whats going wrong: bar.foos[0] creates a new Python object (one that wraps the C++ object), but first checks if this C++ object already as a Python object.

So in the case where you have a variable referencing the Python object, this object is kept alive/not garbage-collected, and the following accesses of bar.foos[0].b will reference the same Python object, setting and getting this attribute on the Python object (without changing the C++ object!).

In the other case, a Python object wrapping the C++ object is created, an attribute b is added, and the Python object is not accessible anymore, it's reference count goes to zero, and it gets garbage collected. Next line, you ask for that same C++ object again, pybind11 notices it's not yet linked to a Python object (because it was garbage collected), and you get a new one which doesn't have the dynamic attribute yet.

How to solve this? This is harder, indeed. The suggestion of the SO answer you reference would work: if you can store it at the C++ side, it'll be persistent.
I'll try something with py::keep_alive, now, and I'll let you know :-)

[YannickJadoul]

I'll try something with py::keep_alive, now, and I'll let you know :-)

Pffff, that's not working :-( Thing is: the Foo Python objects already want to keep Bar alive (because if you hold on to a Foo and the parent Bar disappears, that's ... no good; you'll be referencing dead memory). But if we then e.g. say the Foos need to stay alive until Bar dies, then ... well, reference cycle. In principle, Python is garbage collected and could handle this (although you need a few tricks for C extensions, I think); in practice ... not a great plan :-/

I'm really not sure who's responsibility it is on the C++ side to keep those Python objects referenced. I mean ... you could leak them (increase the refcount by 1 and never decrease), but that's not a solution in general, either, ofc.

[lorenzo-rovigatti]

Thanks @YannickJadoul , I appreciate your attempts and I agree it's probably a very hard issue to solve (and would require some ad-hoc method on c++ side, probably). I guess I'll have to live with this inconsistency :)

[YannickJadoul]

@EricCousineau-TRI, @rwgk, @rhaschke, this already slipped my mind again, but here's another issue related to #2646 (comment)

[EricCousineau-TRI]

Ah, I see - the object has some Python-specific memory that gets lost when pybind11 loses track of the instance (even though the C++ object is still alive and well).

This may influence the solutions that we choose for #1333 - since py::dynamic_attr() relies on py::detail::instance, we can't use the py::wrapper<> approach. In this case, it may be necessary for things like what Ralf mentioned with Boost.Python, where the shared_ptr deleter gets updated with a ref to Python-attached memory.

However, that will break with less flexible holders, e.g. unique_ptr 😿

A reference cycle via keep_alive may fix this (e.g. defer garbage collection to a later generation); however, we need to ensure this is visible to the GC. This reminds me that I need to file a new issue regarding this ;)
EDIT: Done. Filed #2761

[rwgk]

@EricCousineau-TRI pointed me to this issue. Context: redesign related to #2646.

My opinion(s): simply document as unsupported. Dynamic attributes are kind-of dirty to start out with (although they are a must have as an option for dealing with the dirty real world). Adding complexity to pybind11 for use cases like the Foo/Bar example here seems undesirable to me. Additionally, having C++ objects magically drag Python objects with them is highly unobvious, therefore this feature would promote non-ideal designs. Code is read more often than written: it will be best to make things obvious, also on the C++ side. E.g. add b as an attribute in C++ (maybe in a derived class), that's straightforward and clear.