Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: #3851 allow holder other than std::shared_ptr to be used with class inherit from std::enable_shared_from_this #5027

Closed
wants to merge 3 commits into from
Closed

fix: #3851 allow holder other than std::shared_ptr to be used with class inherit from std::enable_shared_from_this #5027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ded5a99
fix #3851 by replacing `const holder_type *` to `const std::shared_pt…
FredBill1 Feb 15, 2024
d37de31
style: pre-commit fixes
pre-commit-ci[bot] Feb 15, 2024
1924bc6
fix clang deprecated-copy-with-user-provided-dtor
FredBill1 Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1848,11 +1848,12 @@ class class_ : public detail::generic_type {
}

private:
/// Initialize holder object, variant 1: object derives from enable_shared_from_this
template <typename T>
/// Initialize holder object, variant 1: holder is shared_ptr and object derives
/// from enable_shared_from_this
template <typename T, typename U>
static void init_holder(detail::instance *inst,
detail::value_and_holder &v_h,
const holder_type * /* unused */,
const std::shared_ptr<U> * /* dummy */,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it conceivable that someone out there is currently using a holder_type that needs this "variant 1"?

(It's just that we don't have a corresponding unit test?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I don't think someone would use a custom holder type that expects to be constructed from a std::shared_ptr<T>&& rather than a T*. If the custom holder type wraps a std::shared_ptr<T> and utilizes std::enable_shared_from_this, it could call shared_from_this manually in the T* constructor.

I apologize if I'm wrong. I'm not very familiar with this project and just happened to encounter the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure

I'm not sure, too. (That's not a great basis for making a decision about this PR.)

Looking in another direction, is it true that you need this PR to support your use case here?

https://github.com/openvinotoolkit/openvino/pull/22872/files#diff-5f0826fd3644f29a91bfbfa7f3a1b0be49edfc6a83ee022a798e25bc858f22f5

My reading of it:

  • You are manufacturing a pybind11 holder to tie the lifetime of ov::Model to the lifetime of ov::CompiledModel.

Is that a correct interpretation?

If yes, that's a very unobvious and convoluted way to implement a lifetime dependency.

If model does indeed have to outlive compiled_model, the obvious solution would be to simply add a private member here and populate if from the icompiled_model ctor (which already receives a std::shared_ptr<model>):

https://github.com/openvinotoolkit/openvino/blob/3986f55da28ee343780b623621d6b531a38af928/src/inference/dev_api/openvino/runtime/icompiled_model.hpp#L140

    std::shared_ptr<model> m_input_model;

That will also ensure pure C++ code is memory-safer in general.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the advice. This is indeed a convoluted way to do that. I've figured out a way to solve that issue, and this PR would not need to be merged now. Should I close this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great! Yes, please close this PR if you no longer need it.

const std::enable_shared_from_this<T> * /* dummy */) {

auto sh = std::dynamic_pointer_cast<typename holder_type::element_type>(
Expand Down
37 changes: 37 additions & 0 deletions tests/test_smart_ptr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,28 @@ struct SharedFromThisVBase : std::enable_shared_from_this<SharedFromThisVBase> {
};
struct SharedFromThisVirt : virtual SharedFromThisVBase {};

// Issue #3851: object inherited from std::enable_shared_from_this held by custom holder
// instead of std::shared_ptr
struct SharedFromThisForCustomHolder
: std::enable_shared_from_this<SharedFromThisForCustomHolder> {
int value;
SharedFromThisForCustomHolder(const SharedFromThisForCustomHolder &) = default;
explicit SharedFromThisForCustomHolder(int v) : value(v) { print_created(this, toString()); }
virtual ~SharedFromThisForCustomHolder() { print_destroyed(this, toString()); }
std::string toString() const {
return "SharedFromThisForCustomHolder[" + std::to_string(value) + "]";
}
};
template <typename T>
class CustomHolderForSharedFromThis {
std::shared_ptr<T> impl;

public:
explicit CustomHolderForSharedFromThis(T *p) : impl(p) {}
explicit CustomHolderForSharedFromThis(std::shared_ptr<T> &p) : impl(p) {}
T *get() const { return impl.get(); }
};

// test_move_only_holder
struct C {
C() { print_created(this); }
Expand Down Expand Up @@ -286,6 +308,7 @@ PYBIND11_DECLARE_HOLDER_TYPE(T, huge_unique_ptr<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, shared_ptr_with_addressof_operator<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, unique_ptr_with_addressof_operator<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, CustomHolderForSharedFromThis<T>);

TEST_SUBMODULE(smart_ptr, m) {
// Please do not interleave `struct` and `class` definitions with bindings code,
Expand Down Expand Up @@ -413,6 +436,20 @@ TEST_SUBMODULE(smart_ptr, m) {
py::class_<SharedFromThisVirt, std::shared_ptr<SharedFromThisVirt>>(m, "SharedFromThisVirt")
.def_static("get", []() { return sft.get(); });

// Issue #3851: object inherited from std::enable_shared_from_this held by custom holder
// instead of std::shared_ptr
py::class_<SharedFromThisForCustomHolder,
CustomHolderForSharedFromThis<SharedFromThisForCustomHolder>>(
m, "SharedFromThisForCustomHolder")
.def_static("make_as_raw_ptr", []() { return new SharedFromThisForCustomHolder(1); })
.def_static("make_as_custom_holder",
[]() {
return CustomHolderForSharedFromThis<SharedFromThisForCustomHolder>(
new SharedFromThisForCustomHolder(2));
})
.def_property_readonly(
"value", [](const SharedFromThisForCustomHolder &self) { return self.value; });

// test_move_only_holder
py::class_<C, custom_unique_ptr<C>>(m, "TypeWithMoveOnlyHolder")
.def_static("make", []() { return custom_unique_ptr<C>(new C); })
Expand Down
7 changes: 7 additions & 0 deletions tests/test_smart_ptr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,13 @@ def test_shared_ptr_from_this_and_references():
assert y is z


def test_enable_shared_from_this_with_custom_holder():
a = m.SharedFromThisForCustomHolder.make_as_raw_ptr()
b = m.SharedFromThisForCustomHolder.make_as_custom_holder()
assert a.value == 1
assert b.value == 2


def test_move_only_holder():
a = m.TypeWithMoveOnlyHolder.make()
b = m.TypeWithMoveOnlyHolder.make_as_object()
Expand Down
Loading