Skip to content

Commit

Permalink
Only keep python portion around if there's an alias class
Browse files Browse the repository at this point in the history
  • Loading branch information
virtuald committed Feb 3, 2021
1 parent 2b93ea7 commit 5a11a72
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 1 deletion.
8 changes: 8 additions & 0 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1547,6 +1547,14 @@ struct holder_retriever<std::shared_ptr<T>> {
};

static auto get_derivative_holder(const value_and_holder &v_h) -> std::shared_ptr<T> {
// If there's no trampoline class, nothing special needed
if (!v_h.inst->has_alias) {
return v_h.template holder<std::shared_ptr<T>>();
}

// If there's a trampoline class, ensure the python side of the object doesn't
// die until the C++ portion also dies
//
// The shared_ptr is always given to C++ code, so construct a new shared_ptr
// that is given a custom deleter. The custom deleter increments the python
// reference count to bind the python instance lifetime with the lifetime
Expand Down
2 changes: 2 additions & 0 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,8 @@ struct instance {
bool simple_instance_registered : 1;
/// If true, get_internals().patients has an entry for this object
bool has_patients : 1;
/// If true, created with an associated alias class (set via `init_instance`)
bool has_alias : 1;

/// Initializes all of the above type/values/holders data (but not the instance values themselves)
void allocate_layout();
Expand Down
12 changes: 11 additions & 1 deletion include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1282,7 +1282,11 @@ class class_ : public detail::generic_type {
record.type_size = sizeof(conditional_t<has_alias, type_alias, type>);
record.type_align = alignof(conditional_t<has_alias, type_alias, type>&);
record.holder_size = sizeof(holder_type);
record.init_instance = init_instance;
if (has_alias) {
record.init_instance = init_alias_instance;
} else {
record.init_instance = init_instance;
}
record.dealloc = dealloc;
record.default_holder = detail::is_instantiation<std::unique_ptr, holder_type>::value;

Expand Down Expand Up @@ -1555,6 +1559,12 @@ class class_ : public detail::generic_type {
init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
}

/// Sets the `has_alias` flag in the instance and calls init_instance
static void init_alias_instance(detail::instance *inst, const void *holder_ptr) {
inst->has_alias = true;
init_instance(inst, holder_ptr);
}

/// Deallocates an instance; via holder, if constructed; otherwise via operator delete.
static void dealloc(detail::value_and_holder &v_h) {
// We could be deallocating because we are cleaning up after a Python exception.
Expand Down
15 changes: 15 additions & 0 deletions tests/test_smart_ptr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -430,4 +430,19 @@ TEST_SUBMODULE(smart_ptr, m) {
.def("set_object", &SpBaseTester::set_object)
.def("is_base_used", &SpBaseTester::is_base_used)
.def_readwrite("obj", &SpBaseTester::m_obj);

// For testing that a C++ class without an alias does not retain the python
// portion of the object
struct SpGoAway {};

struct SpGoAwayTester {
std::shared_ptr<SpGoAway> m_obj;
};

py::class_<SpGoAway, std::shared_ptr<SpGoAway>>(m, "SpGoAway")
.def(py::init<>());

py::class_<SpGoAwayTester>(m, "SpGoAwayTester")
.def(py::init<>())
.def_readwrite("obj", &SpGoAwayTester::m_obj);
}
20 changes: 20 additions & 0 deletions tests/test_smart_ptr.py
Original file line number Diff line number Diff line change
Expand Up @@ -378,3 +378,23 @@ def test_shared_ptr_arg_identity():
tester.set_object(None)
pytest.gc_collect()
assert objref() is None


def test_shared_ptr_goaway():
import weakref

tester = m.SpGoAwayTester()

obj = m.SpGoAway()
objref = weakref.ref(obj)

assert tester.obj is None

tester.obj = obj
del obj
pytest.gc_collect()

# python reference is no longer around
assert objref() is None
# C++ reference is still around
assert tester.obj is not None

0 comments on commit 5a11a72

Please sign in to comment.