[smart_ptr] enable_shared_from_this enhancement
I'm curious a patch to boost::enable_shared_from_this that makes shared_from_this() work inside constructors would be considered? The implementation I've got in mind (haven't actually tried it yet) would work as follows. I'd add two shared_ptrs to enable_shared_from_this, call them "internal_shared" and "external_shared". external_shared would be assigned the external shared_ptr which first takes ownership of *this. internal_shared would be initialized with the "this" pointer, and a custom deleter which would reset external_shared. internal_shared would be reset right after external_shared is initialized (when an external shared_ptr takes ownership of *this). Any calls to shared_from_this() that occur prior to an external shared_ptr taking ownership of *this (such as in the constructor) would get a copy of internal_shared. So the idea is, external_shared would keep the object alive as long as any copies of internal_shared that got made in the constructor are still around. -- Frank
On Monday 17 March 2008 01:36, Frank Mori Hess wrote:
The implementation I've got in mind (haven't actually tried it yet) would work as follows. I'd add two shared_ptrs to enable_shared_from_this, call them "internal_shared" and "external_shared". external_shared would be assigned the external shared_ptr which first takes ownership of *this. internal_shared would be initialized with the "this" pointer, and a custom deleter which would reset external_shared. internal_shared would be reset right after external_shared is initialized (when an external shared_ptr takes ownership of *this). Any calls to shared_from_this() that occur prior to an external shared_ptr taking ownership of *this (such as in the constructor) would get a copy of internal_shared.
Hmm, actually nevermind. This scheme isn't good enough yet, as I'd also want a weak_ptr copy of internal_shared to recognize the case where the object is still being kept alive by a copy of external_shared. -- Frank
On Monday 17 March 2008 01:42, Frank Mori Hess wrote:
On Monday 17 March 2008 01:36, Frank Mori Hess wrote:
The implementation I've got in mind (haven't actually tried it yet) would work as follows. I'd add two shared_ptrs to enable_shared_from_this, call them "internal_shared" and "external_shared". external_shared would be assigned the external shared_ptr which first takes ownership of *this. internal_shared would be initialized with the "this" pointer, and a custom deleter which would reset external_shared. internal_shared would be reset right after external_shared is initialized (when an external shared_ptr
Sorry for the repeated replies to myself (I probably shouldn't be posting past 2 AM), but I think the answer is to swap internal_shared with the external shared_ptr taking ownership before resetting internal_shared. Then everything is a copy of the original internal_shared, and external_shared preserves the user's deleter.
takes ownership of *this). Any calls to shared_from_this() that occur prior to an external shared_ptr taking ownership of *this (such as in the constructor) would get a copy of internal_shared.
Hmm, actually nevermind. This scheme isn't good enough yet, as I'd also want a weak_ptr copy of internal_shared to recognize the case where the object is still being kept alive by a copy of external_shared.
-- Frank
Frank Mori Hess:
The implementation I've got in mind (haven't actually tried it yet) would work as follows. I'd add two shared_ptrs to enable_shared_from_this, call them "internal_shared" and "external_shared". external_shared would be assigned the external shared_ptr which first takes ownership of *this. internal_shared would be initialized with the "this" pointer, and a custom deleter which would reset external_shared. internal_shared would be reset right after external_shared is initialized (when an external shared_ptr takes ownership of *this). Any calls to shared_from_this() that occur prior to an external shared_ptr taking ownership of *this (such as in the constructor) would get a copy of internal_shared.
Interesting idea. I'm not sure why do you need the two pointers though. How would you cope with the following scenarios: class X: enable_shared_from_this<X> {}; int main() { { X x; } { std::auto_ptr<X> px( new X ); } } ?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 17 March 2008 12:38 pm, Peter Dimov wrote:
Interesting idea. I'm not sure why do you need the two pointers though.
In the last version I posted, the second shared_ptr stores the user's deleter. I didn't see any other way to get the user's deleter into the shared_count for the shared_ptrs already constructed from internal_shared. I imagine the implementation could be trimmed down a bit by using classes internal to shared_ptr's implementation, but I'm most familiar with the external shared_ptr api so I stuck to that. One additional gotcha I thought of, is the internal_shared pointer would need to use a special constructor to avoid triggering the enable_shared_from_this machinery when it is constructed.
How would you cope with the following scenarios:
class X: enable_shared_from_this<X> {};
int main() { { X x; } { std::auto_ptr<X> px( new X ); } }
?
In cases where an X object is never owned by a shared_ptr, the shared_ptrs from shared_from_this() would effectively have a null deleter. If people were using shared_from_this() to test if the object ever really got its ownership passed to a shared_ptr, that code would break. That functionality could be replaced by a query returning a bool that indicated if the enable_shared_from_this object has been owned by a shared_ptr constructor yet. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH3qqJ5vihyNWuA4URAi8UAJ41Pu+dyJbvY0/LGaNfYqQkjAL3gwCgvYVs 27HkOTI3gt/WAif2yQRlto8= =8SV8 -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Here's a proof-of-concept patch to svn trunk for enable_shared_from_this.hpp and shared_ptr.hpp. After applying the patch, you should be able to use shared_from_this() successfully from a constructor. It's only a proof-of-concept because it includes some quick-and-dirty nastiness (it makes the shared_count in shared_ptr public!). - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH3uNL5vihyNWuA4URAtSVAKCLPq3Def6CUWDN4C42YmiLNv9XogCgyHBb j/qQOCA+lo1GUj6tQ5J3N14= =0xDw -----END PGP SIGNATURE-----
Frank Mori Hess:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Here's a proof-of-concept patch to svn trunk for enable_shared_from_this.hpp and shared_ptr.hpp. After applying the patch, you should be able to use shared_from_this() successfully from a constructor. It's only a proof-of-concept because it includes some quick-and-dirty nastiness (it makes the shared_count in shared_ptr public!).
A few comments. I liked your original formulation with two shared_ptr instances more. It's better to avoid direct use of shared_count. The aliasing constructor may prove helpful. I don't see why pn needs to be made public. The sp_enable_shared_from_this logic needs to go into a member function (_init? _adopt?) of enable_shared_from_this. I'm unclear on the purpose of pn.swap(...) in sp_enable_shared_from_this. It seems to me that this would overwrite the user's deleter, breaking get_deleter. I don't see why it's needed. sp_enable_shared_from_this may need to be changed to take a shared_ptr instead of a shared_count. I'm disturbed by the fact that we now provide an easy (beginner-accessible) way to create dangling pointers. Consistent shared_ptr use is supposed to make this hard. I'd be interested to hear your ideas about the new precondition of shared_from_this and its new thread safety. Overall, the direction seems promising, and I thank you for the work so far. A final note: changes to enable_shared_from_this unfortunately expose the fact that the shared_ptr/weak_ptr test suite is incomplete; it doesn't perform the full array of tests with types that derive from enable_shared_from_this. I suspect that we need at least partial coverage here to catch regressions.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 18 March 2008 15:13 pm, Peter Dimov wrote:
I liked your original formulation with two shared_ptr instances more. It's better to avoid direct use of shared_count. The aliasing constructor may prove helpful.
I only switched to using a shared_count because that's what sp_enable_shared_from_this() took as an argument. I started to change sp_enable_shared_from_this to take a shared_ptr argument instead of pn/px, but then started to get nervous that I didn't fully understand why all the code was the way it was and decided to just make it work with the shared_count.
I don't see why pn needs to be made public.
It doesn't and shouldn't be, that was just me wanting to quickly get it to compile to prove to myself that the scheme would really work.
The sp_enable_shared_from_this logic needs to go into a member function (_init? _adopt?) of enable_shared_from_this.
Yes, and it seemed to me like _internal_weak_this (and all the additional members I added) should be made private, although I may be missing something.
I'm unclear on the purpose of pn.swap(...) in sp_enable_shared_from_this. It seems to me that this would overwrite the user's deleter, breaking get_deleter. I don't see why it's needed.
Yes, I think it does break get_deleter(), I hadn't considered that. It's done because all the shared_ptrs created by shared_from_this() before the object is owned by the user's shared_ptr need to be made consistent with the shared_count in the user's shared_ptr. The ethical course would have been to take the shared_count from the user's shared_ptr and copy it into any preexisting shared_ptrs created by shared_from_this(). However, there may be many of those, and there was no list of references to them available. So I took the practical course, and went the opposite direction by copying a shared_count from the preexisting shared_ptrs into the user's shared_ptr (of which there is only one, and which has a handy reference available). Maybe if the deleter used by enable_shared_from_this was a class with a template conversion operator, which returned the deleter obtained by getting the user's original deleter from the _user_deleter member? I haven't read through the get_deleter code for shared_ptr yet, so I don't know what's really practical.
I'm disturbed by the fact that we now provide an easy (beginner-accessible) way to create dangling pointers. Consistent shared_ptr use is supposed to make this hard.
A sanity check could be added to the destructor of enable_shared_from_this, something like BOOST_ASSERT(owned() || _internal_shared_this.unique()); That would ensure the user either properly passed ownership of the object to a shared_ptr, or at least didn't leave any dangling pointers created by shared_from_this() around.
I'd be interested to hear your ideas about the new precondition of shared_from_this and its new thread safety.
Honestly, I've tried to suppress thoughts of thread-safety so far. I just repeat to myself "it will all work out fine" to make the bad thoughts go away. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH4Cav5vihyNWuA4URAkGCAJ9g5ifnycNr5ffPaQfGVCM8orG7IwCdE84D 9RMoSkMm4p2uMG+YkTQNLvE= =Mh3a -----END PGP SIGNATURE-----
Frank Mori Hess:
Yes, I think it does break get_deleter(), I hadn't considered that. It's done because all the shared_ptrs created by shared_from_this() before the object is owned by the user's shared_ptr need to be made consistent with the shared_count in the user's shared_ptr. The ethical course would have been to take the shared_count from the user's shared_ptr and copy it into any preexisting shared_ptrs created by shared_from_this(). However, there may be many of those, and there was no list of references to them available. So I took the practical course, and went the opposite direction by copying a shared_count from the preexisting shared_ptrs into the user's shared_ptr (of which there is only one, and which has a handy reference available).
My current line of thinking is that we should refrain from introducing regressions. After shared_ptr<X> px( new X, d ); px should contain the deleter d and - in the case when shared_from_this hasn't been called in the constructor - px.use_count() should be 1. Is it really that important to make the shared_ptr instances created in X::X share ownership with px?
On Tuesday 18 March 2008 16:57, Peter Dimov wrote:
My current line of thinking is that we should refrain from introducing regressions. After
shared_ptr<X> px( new X, d );
px should contain the deleter d and - in the case when shared_from_this hasn't been called in the constructor - px.use_count() should be 1.
The use_count() is 1 with the patched version too, there's no regression there. _internal_shared_this is reset when px is constructed, so it doesn't inflate the use_count. By the way, I think I understand now what you were saying before about not seeing the need for 2 shared_ptrs in enable_shared_from_this. Since _internal_shared_this is only used from construction until ownership is transferred to a shared_ptr, and _user_deleter is only used after ownership is transferred, it can probably be optimized so that a single shared_ptr object fulfills both roles.
Is it really that important to make the shared_ptr instances created in X::X share ownership with px?
It is to me, as that is my primary motivation for making this patch. If they didn't share ownership I wouldn't bother with it, since the plain old "this" pointer would do just as well. -- Frank
Frank Mori Hess:
Is it really that important to make the shared_ptr instances created in X::X share ownership with px?
It is to me, as that is my primary motivation for making this patch. If they didn't share ownership I wouldn't bother with it, since the plain old "this" pointer would do just as well.
To clarify, my question was specifically about sharing ownership. Not about making weak pointers created from the constructor behave as expected. After a brief consideration I think I agree with your approach, though. Nothing better comes to mind. But it would be nice if the deleter replacement happens only if shared_from_this has been called. To proceed, we need two new tests. One that tests currently valid code that will continue to work the same way, and another that tests the new behavior.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 19 March 2008 08:39 am, Peter Dimov wrote:
After a brief consideration I think I agree with your approach, though. Nothing better comes to mind. But it would be nice if the deleter replacement happens only if shared_from_this has been called.
I think that could be done, but would probably require a mutex to protect against shared_from_this() getting called concurrently with ownership transfer. Of course, the lazy initialization in init_internal_shared_once() from the patch in trac probably needs a mutex too, or to use call_once from boost.thread.
To proceed, we need two new tests. One that tests currently valid code that will continue to work the same way
Maybe some of the existing test functions can be turned into templates where the template type parameter is the type owned by the shared_ptr? Then the test could be run on a type that does and doesn't inherit from enable_shared_from_this. I haven't looked at most of the tests, but something like that should work fine on get_deleter_test.cpp.
, and another that tests the new behavior.
I can add some stuff to shared_from_this_test.cpp that calls shared_from_this from constructors. Regarding the get_deleter breakage, I think I can modify sp_counted_impl_pd/sp_counted_impl_pda so their get_deleter methods try to unwrap the deleter D if it is of type detail::deleter_wrapper (a class I'm currently using as the deleter for the shared_from_this pointers ). Maybe I should svn copy a little branch somewhere like sandbox-branches/fmhess/enable_shared_from_this/ for the files I've modified, so you can see my code immediately instead of waiting for a patch posted on trac or wherever? - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH4UIZ5vihyNWuA4URAiFMAJsFftsyPiBzUZw/9GKlr0ZZIvp63QCfaIww evgbnVd1CTMYXg4s9xBsfcY= =a/xZ -----END PGP SIGNATURE-----
Frank Mori Hess:
On Wednesday 19 March 2008 08:39 am, Peter Dimov wrote:
After a brief consideration I think I agree with your approach, though. Nothing better comes to mind. But it would be nice if the deleter replacement happens only if shared_from_this has been called.
I think that could be done, but would probably require a mutex to protect against shared_from_this() getting called concurrently with ownership transfer.
I'm not sure. The intended use case is to call shared_from_this from the constructor, which is being called from: shared_ptr<X> px( new X ); I can think of contrived examples where X::X passes the shared_ptr to another thread which converts it to raw pointer and then calls shared_from_this again... but I think that we can postpone handling such cases unless and until they arise.
, and another that tests the new behavior.
I can add some stuff to shared_from_this_test.cpp that calls shared_from_this from constructors.
No, the new functionality should have its own separate test.
Regarding the get_deleter breakage, I think I can modify sp_counted_impl_pd/sp_counted_impl_pda so their get_deleter methods try to unwrap the deleter D if it is of type detail::deleter_wrapper (a class I'm currently using as the deleter for the shared_from_this pointers ).
This, too, can probably wait.
To proceed, we need two new tests. One that tests currently valid code that will continue to work the same way, and another that tests the new behavior.
I've added esft_regtest.cpp to libs/smart_ptr/test that aims to catch regressions in existing code. If you can spot something that I missed please let me know.
Frank Mori Hess:
Here's a proof-of-concept patch to svn trunk for enable_shared_from_this.hpp and shared_ptr.hpp.
+ _internal_shared_this(static_cast<T*>(this), &_internal_shared_this_deleter, detail::ignore_shared_from_this_tag()), Does this pass shared_from_this_test?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 19 March 2008 15:17 pm, Peter Dimov wrote:
Frank Mori Hess:
Here's a proof-of-concept patch to svn trunk for enable_shared_from_this.hpp and shared_ptr.hpp.
+ _internal_shared_this(static_cast<T*>(this), &_internal_shared_this_deleter, detail::ignore_shared_from_this_tag()),
Does this pass shared_from_this_test?
The first version I posted to the mailing list does not. The version I put in trac does (it does a lazy dynamic_cast). I've made some more changes today, and my local version passes the get_deleter_test (modified to run with either the old X class or a new Y class derived from enable_shared_from_this). I'll put an updated patch with what I have now in trac sometime in the next hour. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH4XNH5vihyNWuA4URAhtCAJ451+46OOOgWo7MI2PPOrKCgulENwCgq69E pOD9YcZ2N0/gbhCUBr/FpKc= =JGgd -----END PGP SIGNATURE-----
Frank Mori Hess:
I'll put an updated patch with what I have now in trac sometime in the next hour.
Your _v3 patch looks pretty good. deleter_wrapper should probably be sp_deleter_wrapper. What is the purpose of owned()?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 19 March 2008 17:41 pm, Peter Dimov wrote:
Your _v3 patch looks pretty good. deleter_wrapper should probably be sp_deleter_wrapper. What is the purpose of owned()?
It indicates whether the object has had ownership passed to a shared_ptr yet. I don't have a strong justification on why it needs to be public, maybe it would be useful for debugging/diagnostic purposes. Internally, it's needed to figure out if the lazy initialization of _internal_shared_this has been performed yet, although it could be replaced by something that more directly serves that purpose. Attached is a test for the new functionality. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH4n6d5vihyNWuA4URArUcAKDfpaUBKcfbyNU4pbu1P2IjDdbBGQCfZsHx TYToCExKCckLEX6sKcgF6eo= =YoeM -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
On Wednesday 19 March 2008 17:41 pm, Peter Dimov wrote:
What is the purpose of owned()?
It indicates whether the object has had ownership passed to a shared_ptr yet. I don't have a strong justification on why it needs to be public, maybe it would be useful for debugging/diagnostic purposes.
It's better to omit the owned() accessor unless/until we have a use case for it.
Attached is a test for the new functionality.
Looks good. In: { boost::weak_ptr<X> early_weak_px; { boost::shared_ptr<X> early_px; X x( 0, &early_px ); boost::weak_ptr<X> early_weak_px = early_px; early_px.reset(); BOOST_TEST( early_weak_px.expired() == false ); BOOST_TEST( X::instances == 1 ); } BOOST_TEST( early_weak_px.expired() ); } you probably didn't want to redeclare early_weak_px inside the {} block though. As a minor stylistic issue, I'd prefer !expired() instead of expired() == false. You might want to add tests for ownership sharing, !( early_px < px ) && !( px < early_px ). One final question, in: BOOST_TEST( early_px.use_count() > 0 ); what does use_count() return? 2? Do we want to test against 2 directly, or are we to leave the initial use count unspecified? Do you have SVN write access?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 20 March 2008 11:25 am, Peter Dimov wrote:
It's better to omit the owned() accessor unless/until we have a use case for it.
Ok
As a minor stylistic issue, I'd prefer !expired() instead of expired() == false.
Ok
You might want to add tests for ownership sharing, !( early_px < px ) && !( px < early_px ).
Ah, yes. That's what I was trying to get at with some of the use count checking, but didn't realize there was a better way.
One final question, in:
BOOST_TEST( early_px.use_count() > 0 );
what does use_count() return? 2? Do we want to test against 2 directly, or are we to leave the initial use count unspecified?
Yes, it returns 2. We can test for equality if you prefer. I just left it unspecified because I felt like the 2 was just an implementation detail leaking out.
Do you have SVN write access?
Yes, just tell me what/when is ok to commit and I should be able to do it. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH4ogv5vihyNWuA4URAnYIAJ4gcWMlrZHW0Km2Z8+uMXsMJr5MlACgpZJt QQcApintW/tFGUyWRGLuIVo= =+oLw -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
One final question, in:
BOOST_TEST( early_px.use_count() > 0 );
what does use_count() return? 2? Do we want to test against 2 directly, or are we to leave the initial use count unspecified?
Yes, it returns 2. We can test for equality if you prefer. I just left it unspecified because I felt like the 2 was just an implementation detail leaking out.
Unspecified is fine.
Do you have SVN write access?
Yes, just tell me what/when is ok to commit and I should be able to do it.
I think that your current iteration is ready for the trunk. Thank you again for your work.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 20 March 2008 11:11 am, Frank Mori Hess wrote:
Attached is a test for the new functionality.
And here's a one-line patch to fix a bug I just noticed in the new test. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFH4oLj5vihyNWuA4URAs0zAKDBiGtZ89fa/5zlLm+P464knabgCwCfYGc4 RarFAFIhcKAOfykK0Mwp+jo= =E6IO -----END PGP SIGNATURE-----
participants (3)
-
Frank Mori Hess -
Frank Mori Hess -
Peter Dimov