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

[FredBill1]

Description

Hi, this pr is to fix issue #3851. The variant 1 here should only be executed when the holder type is a std::shared_ptr, as explained in the issue page.

pybind11/include/pybind11/pybind11.h

Lines 1851 to 1869 in 8b48ff8

	/// Initialize holder object, variant 1: object derives from enable_shared_from_this
	template <typename T>
	static void init_holder(detail::instance *inst,
	detail::value_and_holder &v_h,
	const holder_type * /* unused */,
	const std::enable_shared_from_this<T> * /* dummy */) {
	auto sh = std::dynamic_pointer_cast<typename holder_type::element_type>(
	detail::try_get_shared_from_this(v_h.value_ptr<type>()));
	if (sh) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(std::move(sh));
	v_h.set_holder_constructed();
	}
	if (!v_h.holder_constructed() && inst->owned) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());
	v_h.set_holder_constructed();
	}
	}

pybind11/include/pybind11/pybind11.h

Lines 1885 to 1898 in 8b48ff8

	/// Initialize holder object, variant 2: try to construct from existing holder object, if
	/// possible
	static void init_holder(detail::instance *inst,
	detail::value_and_holder &v_h,
	const holder_type *holder_ptr,
	const void * /* dummy -- not enable_shared_from_this<T>) */) {
	if (holder_ptr) {
	init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible<holder_type>());
	v_h.set_holder_constructed();
	} else if (detail::always_construct_holder<holder_type>::value || inst->owned) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());
	v_h.set_holder_constructed();
	}
	}

A test is also created, which can only be compiled with the bug fixed.

Suggested changelog entry:

[rwgk]

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?)

[FredBill1]

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.

[rwgk]

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.

[FredBill1]

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?

[rwgk]

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