[smart_ptr] questions about some regression test cases
My questions result from the observation that in the virtual dtor of enable_shared_from_this, I tried BOOST_ASSERT( _shared_count.empty() ), but this makes 3 tests fail. Looking at these tests, I don't see why they should be legal: In esft_constructor_test.cpp, line 143, X initializes early_px by calling shared_from_this, but X is never passed to a shared_ptr which then owns it. From what I understand, this should be illegal. The next problem is esft_constructor_test.cpp, line 160. y.shared_from_this() returns a shared_ptr, but again, no shared_ptr takes ownership of Y later. While shared_from_this() is not called from the ctor, I wonder if this is yet another use case or just an accident. The test case in shared_from_this_test.cpp, line 135 is similar. Regards, Daniel
On Sunday 27 April 2008 15:02, Daniel Frey wrote:
My questions result from the observation that in the virtual dtor of enable_shared_from_this, I tried BOOST_ASSERT( _shared_count.empty() ),
Are you aware that will abort the program in the case of a derived constructor calling shared_from_this() then throwing? It might be worth considering making it a requirement that no constructor can throw after calling shared_from_this though, see the first half of an earlier post: http://lists.boost.org/Archives/boost/2008/04/135372.php
but this makes 3 tests fail. Looking at these tests, I don't see why they should be legal:
In esft_constructor_test.cpp, line 143, X initializes early_px by calling shared_from_this, but X is never passed to a shared_ptr which then owns it. From what I understand, this should be illegal.
The next problem is esft_constructor_test.cpp, line 160. y.shared_from_this() returns a shared_ptr, but again, no shared_ptr takes ownership of Y later. While shared_from_this() is not called from the ctor, I wonder if this is yet another use case or just an accident.
The test case in shared_from_this_test.cpp, line 135 is similar.
-- Frank
On Sun, 2008-04-27 at 20:27 -0400, Frank Mori Hess wrote:
On Sunday 27 April 2008 15:02, Daniel Frey wrote:
My questions result from the observation that in the virtual dtor of enable_shared_from_this, I tried BOOST_ASSERT( _shared_count.empty() ),
Are you aware that will abort the program in the case of a derived constructor calling shared_from_this() then throwing? It might be worth considering making it a requirement that no constructor can throw after calling shared_from_this though, see the first half of an earlier post:
Consider: class Object { static map< string, shared_ptr< Object > > cache; Object() { cache.insert( make_pair( "foo", shared_from_this() ) ); throw 42; } }; With the above BOOST_ASSERT(_shared_count.empty()), ::abort() seems to most sensible thing we can do, otherwise it would lead to very dangerous situations where cache contains a shared_ptr to a deleted object. Regards, Daniel
Daniel Frey:
Consider:
class Object { static map< string, shared_ptr< Object > > cache;
Object() { cache.insert( make_pair( "foo", shared_from_this() ) ); throw 42; } };
With the above BOOST_ASSERT(_shared_count.empty()), ::abort() seems to most sensible thing we can do, otherwise it would lead to very dangerous situations where cache contains a shared_ptr to a deleted object.
The original assert catches this case, does it not?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 28 April 2008 09:27 am, Peter Dimov wrote:
Daniel Frey:
Consider:
class Object { static map< string, shared_ptr< Object > > cache;
Object() { cache.insert( make_pair( "foo", shared_from_this() ) ); throw 42; } };
With the above BOOST_ASSERT(_shared_count.empty()), ::abort() seems to most sensible thing we can do, otherwise it would lead to very dangerous situations where cache contains a shared_ptr to a deleted object.
The original assert catches this case, does it not?
Yes. The difference would be if cache was: static map< string, weak_ptr< Object > > cache; Then the current assert would allow it (and it would probably be safe at least in a single-threaded context), but BOOST_ASSERT(_shared_count.empty()) would abort. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIFfdY5vihyNWuA4URAvhbAJ9MzZO+arh/mrbr6i0KCRXF6QM4zQCfS/EA 1rReoupBhiB/cStdvJQJeU8= =RWWF -----END PGP SIGNATURE-----
On Mon, 2008-04-28 at 12:11 -0400, Frank Mori Hess wrote:
On Monday 28 April 2008 09:27 am, Peter Dimov wrote:
The original assert catches this case, does it not?
Yes. The difference would be if cache was:
You are both right, my example was a bad one.
static map< string, weak_ptr< Object > > cache;
Then the current assert would allow it (and it would probably be safe at least in a single-threaded context), but BOOST_ASSERT(_shared_count.empty()) would abort.
The main point of BOOST_ASSERT(_shared_count.empty()) is to catch more cases than the current test. It breaks three test cases, so once again: Are these tests correct and by design or do they pass by accident? Regards, Daniel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 28 April 2008 12:32 pm, Daniel Frey wrote:
The main point of BOOST_ASSERT(_shared_count.empty()) is to catch more cases than the current test. It breaks three test cases, so once again: Are these tests correct and by design or do they pass by accident?
They are correct, in that I was testing that my implementation behaved the way I expected it to. So they are by design, but only my own design. The details of what the esft destructor should choke on are not something that have been discussed in depth before. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIFgb75vihyNWuA4URAn3tAKDdpvL5/DC8LQFTMDSJpGMiuQPWnwCgldyB 3cy3lGaQusSFTf+AwP4NqIs= =JUbJ -----END PGP SIGNATURE-----
On Mon, 2008-04-28 at 13:18 -0400, Frank Mori Hess wrote:
On Monday 28 April 2008 12:32 pm, Daniel Frey wrote:
The main point of BOOST_ASSERT(_shared_count.empty()) is to catch more cases than the current test. It breaks three test cases, so once again: Are these tests correct and by design or do they pass by accident?
They are correct, in that I was testing that my implementation behaved the way I expected it to. So they are by design, but only my own design. The details of what the esft destructor should choke on are not something that have been discussed in depth before.
I hope it's OK if we discuss it now. So far, I was under the impression that calling shared_from_this() requires that some other shared_ptr takes ownership later. BOOST_ASSERT( _shared_count.empty() ) tests exactly that. It's an easy to understand rule and I can see the use cases. How would you describe/document the behavior you had in mind? What use case would it serve? Note that for me it is crucial to understand what needs to be supported in order to be able to continue optimizing it (if that's possible at all :) Regards, Daniel
On Monday 28 April 2008 13:38, Daniel Frey wrote:
I hope it's OK if we discuss it now. So far, I was under the impression that calling shared_from_this() requires that some other shared_ptr takes ownership later. BOOST_ASSERT( _shared_count.empty() ) tests exactly that. It's an easy to understand rule and I can see the use cases.
How would you describe/document the behavior you had in mind? What use case would it serve?
It asserts that no dangling shared_ptrs exist, or that a shared_ptr has taken ownership (in which case it is assumed the user knows what they are doing). It permits, for example, weak_ptrs to exist which might be used to track the object's lifetime whether or not it is actually managed by a shared_ptr. To be more concrete, you might have an object which uses Qt signals/slots and whose lifetime is managed by the Qt framework. You could use shared_from_this() to get a weak_ptr that can be used to track the object's lifetime by thread_safe_signals, for automatic connection management of additional signals/slots from thread_safe_signals. It would work equivalently to boost::trackable from the current boost.signals. On the other hand, I can't think of any situation where aborting on the "no owner and no dangling shared_ptr" case would actually provide any additional safety over allowing it. -- Frank
On Mon, 2008-04-28 at 22:24 -0400, Frank Mori Hess wrote:
On Monday 28 April 2008 13:38, Daniel Frey wrote:
I hope it's OK if we discuss it now. So far, I was under the impression that calling shared_from_this() requires that some other shared_ptr takes ownership later. BOOST_ASSERT( _shared_count.empty() ) tests exactly that. It's an easy to understand rule and I can see the use cases.
How would you describe/document the behavior you had in mind? What use case would it serve?
It asserts that no dangling shared_ptrs exist, or that a shared_ptr has taken ownership (in which case it is assumed the user knows what they are doing). It permits, for example, weak_ptrs to exist which might be used to track the object's lifetime whether or not it is actually managed by a shared_ptr. To be more concrete, you might have an object which uses Qt signals/slots and whose lifetime is managed by the Qt framework. You could use shared_from_this() to get a weak_ptr that can be used to track the object's lifetime by thread_safe_signals, for automatic connection management of additional signals/slots from thread_safe_signals. It would work equivalently to boost::trackable from the current boost.signals.
OK, thanks.
On the other hand, I can't think of any situation where aborting on the "no owner and no dangling shared_ptr" case would actually provide any additional safety over allowing it.
Well, the failing test cases are an example - it just depends on whether you want to allow them or if you consider this usage pattern an error. Please don't get me wrong, I'm not against it. I am just trying to clarify the intentions of the code. And I took the freedom to fix the comment that IMHO was misleading me to think that the test cases were unintentionally passing. It also means that sp_deleter_wrapper needs to make sure that it is a no-op when set_deleter has never been called, which I have to keep in mind when I try to optimize this stuff further. Thanks for clarifying. Regards, Daniel
I think there are cases not caught by the current assert condition: struct A : boost::enable_shared_from_this<A> {}; int main() { boost::shared_ptr<A> pa; { A a; pa = a.shared_from_this(); boost::shared_ptr<A> pa2( &a ); // here, first 'pa2' and then 'a' goes out of scope! } // now 'pa' still exists and points to a deleted object } What do you think of the following condition: BOOST_ASSERT( _weak_count.use_count() == 0 || _shared_count.use_count() == 1 ); It passes all regression tests and catches the above case. Do you see any problem with it or shall I commit it? Regards, Daniel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 29 April 2008 02:18 am, Daniel Frey wrote:
I think there are cases not caught by the current assert condition:
struct A : boost::enable_shared_from_this<A> {};
int main() { boost::shared_ptr<A> pa; { A a; pa = a.shared_from_this(); boost::shared_ptr<A> pa2( &a ); // here, first 'pa2' and then 'a' goes out of scope! } // now 'pa' still exists and points to a deleted object }
You've already created undefined behavior just by passing ownership of a stack object to a shared_ptr with the default deleter, which is in no way specific to enable_shared_from_this.
What do you think of the following condition:
BOOST_ASSERT( _weak_count.use_count() == 0 || _shared_count.use_count() == 1 );
There is no shared_ptr requirement that a user is forbidden to create dangling shared_ptr if they choose to. I don't think owning an enable_shared_from_this derived object should impose any additional constraints on shared_ptr usage. Adding additional checks to enforce shared_ptr best practices seems like an independent concern from esft. It could be done in an independent class, and might make decent example for sp_accept_owner usage. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIFyXs5vihyNWuA4URAn3RAKCT385xl5617h1uE+OmqrsItL6TdgCdGPW1 Tg6I9VXEsRVxQrLVFYQuCIM= =7whB -----END PGP SIGNATURE-----
participants (4)
-
Daniel Frey -
Frank Mori Hess -
Frank Mori Hess -
Peter Dimov