[smart_ptr] enable_shared_from_this and multiple inheritance solution
(new Derived()); }
Fellow boosters, A little while ago I found a pernicious problem with enable_shared_from_this when used with multiple inheritance. Here is an example: #include <boost/smart_ptr.hpp> #include <boost/enable_shared_from_this.hpp> class Base1 : public boost::enable_shared_from_this< Base1 > { public: Base1() {} virtual ~Base1() {} boost::shared_ptr< Base1 > GetBase1() { return shared_from_this(); } }; class Base2 : public boost::enable_shared_from_this< Base2 > { public: Base2() {} virtual ~Base2() {} boost::shared_ptr< Base2 > GetBase2() { return shared_from_this(); } }; class Derived : public Base1, public Base2 { }; int main() { boost::shared_ptr< Derived > wDerived(new Derived()); // Either of the following lines crashes with GCC, but the first one // passes on VS2005 boost::shared_ptr< Base1 > wBase1 = wDerived->GetBase1(); boost::shared_ptr< Base2 > wBase2 = wDerived->GetBase2(); return 0; } Since Derived inherits from two bases that each inherits from enable_shared_from_this, it has two internal weak_ptr. These weak_ptr are usualy initialised in the constructor of the shared_ptr and I have observed two different behavior with the above code depending if I build it with VS2005 or GCC. With VS2005, the first base class in the inheritance list of Derived (here Base1) get its internal weak_ptr initialized, but not the second one, thus it crashes when we try to access the Base2 weak_ptr via shared_from_this. On GCC, neither base classes get their internal weak_ptr initialized and both base classes access to their internal weak_ptr provokes a crash. To prevent that kind of problem, I created a SmartPtrBuilder and a (very) slightly modified EnabledSharedFromThis (only one assert was removed). Here is the same program with the modifications: #include <boost/smart_ptr.hpp> #include "EnableSharedFromThis.h" #include "SmartPtrBuilder.h" class Base1 : public Generic::EnableSharedFromThis< Base1 > { public: Base1() {} virtual ~Base1() {} boost::shared_ptr< Base1 > GetBase1() { return SharedFromThis(); } }; class Base2 : public Generic::EnableSharedFromThis< Base2 > { public: Base2() {} virtual ~Base2() {} boost::shared_ptr< Base2 > GetBase2() { return SharedFromThis(); } }; class Derived : public Base1, public Base2 { public: // Factory method static boost::shared_ptr< Derived > Create() { return Generic::SmartPtrBuilder::CreateSharedPtr< Base1, Base2 private: Derived() {} }; int main() { boost::shared_ptr< Derived > wDerived = Derived::Create(); // No more crashes boost::shared_ptr< Base1 > wBase1 = wDerived->GetBase1(); boost::shared_ptr< Base2 > wBase2 = wDerived->GetBase2(); return 0; } As you can see, Derived has now a factory method that uses the SmartPtrBuilder to create the shared_ptr. Two template arguments are used on CreateSharedPtr, allowing it to initialize the internal weak_ptr of each base classes. The SmartPtrBuilder I did can support up to 10 base classes as template arguments and I also did a variadic template version which has no limit. By using the factory method, the users of Derived don't need to know which of its base classes need its internal weak_ptr need to be initialized. Source code for the SmartPtrBuilder and the modified EnableSharedFromThis: http://spoluck.ca:8000/boost/EnableSharedFromThis.h http://spoluck.ca:8000/boost/SmartPtrBuilder.h That builder might be incorporated to the boost smart pointer library so that anyone could use its facilities to prevent the problem mentioned earlier. No modification to the mighty shared_ptr class required, only the addition of a builder. What do you think? Philippe
Philippe Cayouette <pcayouette@spoluck.ca> writes:
Fellow boosters,
A little while ago I found a pernicious problem with enable_shared_from_this when used with multiple inheritance.
[snip detailed example]
Since Derived inherits from two bases that each inherits from enable_shared_from_this, it has two internal weak_ptr. These weak_ptr are usualy initialised in the constructor of the shared_ptr and I have observed two different behavior with the above code depending if I build it with VS2005 or GCC. With VS2005, the first base class in the inheritance list of Derived (here Base1) get its internal weak_ptr initialized, but not the second one, thus it crashes when we try to access the Base2 weak_ptr via shared_from_this. On GCC, neither base classes get their internal weak_ptr initialized and both base classes access to their internal weak_ptr provokes a crash.
I have also run into this problem. In my particular example, I was able to avoid the problem by moving the inheritance from enable_shared_from_this up the class hierarchy. However, the experience lead me to consider alternate ways to resolve this issue. I was revisiting this work the other day, and I thought I would take this opportunity to share my solution as well.
To prevent that kind of problem, I created a SmartPtrBuilder and a (very) slightly modified EnabledSharedFromThis (only one assert was removed).
Here is the same program with the modifications:
[snip]
(new Derived()); }
class Derived : public Base1, public Base2 { public: // Factory method static boost::shared_ptr< Derived > Create() { return Generic::SmartPtrBuilder::CreateSharedPtr< Base1, Base2 private: Derived() {} };
int main() { boost::shared_ptr< Derived > wDerived = Derived::Create(); // No more crashes boost::shared_ptr< Base1 > wBase1 = wDerived->GetBase1(); boost::shared_ptr< Base2 > wBase2 = wDerived->GetBase2(); return 0; }
As you can see, Derived has now a factory method that uses the SmartPtrBuilder to create the shared_ptr. Two template arguments are used on CreateSharedPtr, allowing it to initialize the internal weak_ptr of each base classes. The SmartPtrBuilder I did can support up to 10 base classes as template arguments and I also did a variadic template version which has no limit.
This is a really neat idea, and I never considered using template arguments to resolve this issue of multiple inheritance.
By using the factory method, the users of Derived don't need to know which of its base classes need its internal weak_ptr need to be initialized.
I took a slightly different tack, not wanting to force users to make use of a factory method. I don't mind factory methods, but I hesitate to impose the use of a factory for no other reason than implementation details. In my solution, my enable_shared_from_polymorphic<T> class is just a shim that inherits virtually from enable_shared_from_polymorphic_base, which is not a template type. The base class holds a weak pointer, which can be dynamic_pointer_cast by the child class when needed. This allows your example to work as expected by merely inheriting from enable_shared_from_polymorphic<T> rather than enable_shared_from_this<T>. If anyone is interested in the code, it can be found at: http://208.106.110.44/~ansel/boost/enable_shared_from_polymorphic.patch This patch is against the current SVN version of boost. The pro of this method is that the class can be used identically to the way that enable_shared_from_this is used. A downside is that any class deriving from enable_shared_from_polymorphic<T> becomes polymorphic and incurs all the cost thereof. This cost could potentially be mitigated if these classes used static_pointer_cast instead of dynamic_pointer_cast. However, I am not familiar enough with the appropriate areas of the C++ standard to be sure that would work in cases of multiple inheritance, precisely when these classes are useful. Thanks for reading, Ansel
On 10-11-28 03:02 PM, Ansel Sermersheim wrote:
In my solution, my enable_shared_from_polymorphic<T> class is just a shim that inherits virtually from enable_shared_from_polymorphic_base, which is not a template type. The base class holds a weak pointer, which can be dynamic_pointer_cast by the child class when needed.
Interesting idea, you resolve the problem of multiple inheritance with virtual inheritance. Having a single internal weak_ptr is surely a better idea than having a lot of them.
This allows your example to work as expected by merely inheriting from enable_shared_from_polymorphic<T> rather than enable_shared_from_this<T>.
Did you tried your code on GCC? I just applied your patch and I think I am encountering the same problem as my example: on GCC, the function sp_enable_shared_from_this that is called is the one with the variable arguments (...), since the compiler cannot be sure of the value of T in the expression enable_shared_from_polymorphic< T > (it might be Base1 or Base2). On VS2005, the compiler chooses Base1 (in the original example) and thus, the pe->_internal_accept_owner function is called on the unique enable_shared_from_polymorphic_base and everything works (I guess, I didn't try). However, you might want to change the signature of sp_enable_shared_from_this that takes a enable_shared_from_polymorphic_base instead of a enable_shared_from_polymorphic< T >. I tried to just change it but it didn't compile, you might want rearrange your stuff to make it compile and give it a try.
If anyone is interested in the code, it can be found at:
http://208.106.110.44/~ansel/boost/enable_shared_from_polymorphic.patch
This patch is against the current SVN version of boost.
The pro of this method is that the class can be used identically to the way that enable_shared_from_this is used. A downside is that any class deriving from enable_shared_from_polymorphic<T> becomes polymorphic and incurs all the cost thereof.
IMHO, I think the biggest downside of your method is the usage of dynamic_pointer_cast, not the polymorphic costs. In fact, dynamic casts are non-deterministic and need RTTI, which is not always enabled depending on the environment (e.g. VxWorks 653 with cert). Real-time applications often disable RTTI to make sure they stay deterministic.
This cost could potentially be mitigated if these classes used static_pointer_cast instead of dynamic_pointer_cast. However, I am not familiar enough with the appropriate areas of the C++ standard to be sure that would work in cases of multiple inheritance, precisely when these classes are useful.
If you manage to make this work with static_pointer_cast instead of dynamic_pointer_cast, it would be the perfect solution to me... but I am not sure it is possible.
Philippe Cayouette <pcayouette@spoluck.ca> writes:
On 10-11-28 03:02 PM, Ansel Sermersheim wrote:
In my solution, my enable_shared_from_polymorphic<T> class is just a shim that inherits virtually from enable_shared_from_polymorphic_base, which is not a template type. The base class holds a weak pointer, which can be dynamic_pointer_cast by the child class when needed.
Interesting idea, you resolve the problem of multiple inheritance with virtual inheritance. Having a single internal weak_ptr is surely a better idea than having a lot of them.
This allows your example to work as expected by merely inheriting from enable_shared_from_polymorphic<T> rather than enable_shared_from_this<T>.
Did you tried your code on GCC? I just applied your patch and I think I am encountering the same problem as my example: on GCC, the function sp_enable_shared_from_this that is called is the one with the variable arguments (...), since the compiler cannot be sure of the value of T in the expression enable_shared_from_polymorphic< T > (it might be Base1 or Base2). On VS2005, the compiler chooses Base1 (in the original example) and thus, the pe->_internal_accept_owner function is called on the unique enable_shared_from_polymorphic_base and everything works (I guess, I didn't try).
This is rather puzzling. I do develop on GCC, I tested with: bash$ g++ --version g++ (Debian 4.4.5-6) 4.4.5 Copyright (C) 2010 Free Software Foundation, Inc. However, your idea of having the _internal_accept_owner function take an enable_shared_from_polymorphic base makes perfect sense, and I have made that change. Hopefully that builds more happily for you.
If anyone is interested in the code, it can be found at:
http://208.106.110.44/~ansel/boost/enable_shared_from_polymorphic.patch
This patch is against the current SVN version of boost.
The pro of this method is that the class can be used identically to the way that enable_shared_from_this is used. A downside is that any class deriving from enable_shared_from_polymorphic<T> becomes polymorphic and incurs all the cost thereof.
IMHO, I think the biggest downside of your method is the usage of dynamic_pointer_cast, not the polymorphic costs. In fact, dynamic casts are non-deterministic and need RTTI, which is not always enabled depending on the environment (e.g. VxWorks 653 with cert). Real-time applications often disable RTTI to make sure they stay deterministic.
Very good point, and one I often miss since most projects I work on use RTTI extensively.
This cost could potentially be mitigated if these classes used static_pointer_cast instead of dynamic_pointer_cast. However, I am not familiar enough with the appropriate areas of the C++ standard to be sure that would work in cases of multiple inheritance, precisely when these classes are useful.
If you manage to make this work with static_pointer_cast instead of dynamic_pointer_cast, it would be the perfect solution to me... but I am not sure it is possible.
After mulling this over for some time, I _think_ I have come up with a solution that mitigates all these problems. I have a new patch up, at the same URL. In order to make this mechanism work, I had to add a new constructor to boost::shared_ptr<T>, which I am loath to do. Given that downside the patch appears to work properly. It also adds a free function boost::shared_from<T>(T*) which returns a boost::shared_ptr<T>. The only cast that occurs in this code is a static_cast that downcasts from enable_shared_from_polymorphic<T> to T. I have read the relevant portions of the standard several times and I beleive this is valid, but I would definitely be curious if wiser heads agree with my reading. I would be very curious to hear if this new patch compiles successfully on your system. Thanks for your feedback, Ansel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 29 November 2010, Ansel Sermersheim wrote:
After mulling this over for some time, I _think_ I have come up with a solution that mitigates all these problems.
I have a new patch up, at the same URL. In order to make this mechanism work, I had to add a new constructor to boost::shared_ptr<T>, which I am loath to do. Given that downside the patch appears to work properly. It also adds a free function boost::shared_from<T>(T*) which returns a boost::shared_ptr<T>.
The only cast that occurs in this code is a static_cast that downcasts from enable_shared_from_polymorphic<T> to T. I have read the relevant portions of the standard several times and I beleive this is valid, but I would definitely be curious if wiser heads agree with my reading.
If you use a free function only (see enable_shared_from_raw.hpp in trunk), the enabling base class doesn't have to use the CRTP. Why did you need another shared_ptr constructor (sorry, I don't have time to actually read the code)? It's hard to imagine, given the presence of the shared_ptr aliasing constructor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkz1BtAACgkQ5vihyNWuA4Uz3gCbBSoU+V0ifaE19k6bX9IzxD4l WRcAmwXXO3eFlzKN8i/85Js3ho15TH6y =zZIO -----END PGP SIGNATURE-----
Frank Mori Hess <frank.hess@nist.gov> writes:
On Monday 29 November 2010, Ansel Sermersheim wrote:
After mulling this over for some time, I _think_ I have come up with a solution that mitigates all these problems.
I have a new patch up, at the same URL. In order to make this mechanism work, I had to add a new constructor to boost::shared_ptr<T>, which I am loath to do. Given that downside the patch appears to work properly. It also adds a free function boost::shared_from<T>(T*) which returns a boost::shared_ptr<T>.
The only cast that occurs in this code is a static_cast that downcasts from enable_shared_from_polymorphic<T> to T. I have read the relevant portions of the standard several times and I beleive this is valid, but I would definitely be curious if wiser heads agree with my reading.
If you use a free function only (see enable_shared_from_raw.hpp in trunk), the enabling base class doesn't have to use the CRTP.
That is true. However, part of my goal was to create a drop-in replacement for enable_shared_from_this which will work even when multiple classes in the inheritance heirarchy inherit from it. Thus, shared_from_this() needs to know what type to return. It certainly would be possible to have another type that inherits from enable_shared_from_polymorphic_base, which would then allow use of the free function. Or the free function may not be the most desirable interface. An earlier version had shared_from as a method, so that one could only call it as shared_from(this). I haven't really thought through the pros and cons of free function vs method at this point.
Why did you need another shared_ptr constructor (sorry, I don't have time to actually read the code)? It's hard to imagine, given the presence of the shared_ptr aliasing constructor.
Now why on earth didn't I see that the last time I looked at the available shared_ptr constructors? That is exactly what was needed. This removes one of my major dislikes about this approach. I will respin a new patch later today and clean up this area. Thank you very much for the critique, Ansel
On 2010-11-29 22:56, Ansel Sermersheim wrote:
Did you tried your code on GCC? I just applied your patch and I think I am encountering the same problem as my example: on GCC, the function sp_enable_shared_from_this that is called is the one with the variable arguments (...), since the compiler cannot be sure of the value of T in the expression enable_shared_from_polymorphic< T> (it might be Base1 or Base2). On VS2005, the compiler chooses Base1 (in the original example) and thus, the pe->_internal_accept_owner function is called on the unique enable_shared_from_polymorphic_base and everything works (I guess, I didn't try).
This is rather puzzling. I do develop on GCC, I tested with:
bash$ g++ --version g++ (Debian 4.4.5-6) 4.4.5 Copyright (C) 2010 Free Software Foundation, Inc.
Sorry for the confusion, your original patch did actually build on my system, however, the compiler does not know which Base to take in sp_enable_shared_from_this, so it decides to go with the (...) function instead (please read again my explanation above for details, you might also want to step into the code to see for yourself). So at runtime, the internal weak_ptr never get initialized.
However, your idea of having the _internal_accept_owner function take an enable_shared_from_polymorphic base makes perfect sense, and I have made that change. Hopefully that builds more happily for you.
This should solve the problem.
If anyone is interested in the code, it can be found at:
http://208.106.110.44/~ansel/boost/enable_shared_from_polymorphic.patch
This patch is against the current SVN version of boost.
The only cast that occurs in this code is a static_cast that downcasts from enable_shared_from_polymorphic<T> to T. I have read the relevant portions of the standard several times and I beleive this is valid, but I would definitely be curious if wiser heads agree with my reading.
To my understanding, you cannot downcast statically from a virtual base, so I wonder how you can make it without dynamic cast, I will try your latest patch and get back to you. The passage from enable_shared_from_polymorphic<T> to T is no brainer, it is the passage from enable_shared_from_polymorphic_base to enable_shared_from_polymorphic<T> that I think is unknown at compile time... Philippe
On 2010-11-30 13:23, Philippe Cayouette wrote:
If anyone is interested in the code, it can be found at:
http://208.106.110.44/~ansel/boost/enable_shared_from_polymorphic.patch
This patch is against the current SVN version of boost.
The only cast that occurs in this code is a static_cast that downcasts from enable_shared_from_polymorphic<T> to T. I have read the relevant portions of the standard several times and I beleive this is valid, but I would definitely be curious if wiser heads agree with my reading.
I finally took the time to test your latest patch from the URL you provided. I am using VS2005 right now and it does not work, in fact, the function sp_enable_shared_from_this in your file never got called, resulting in an uninitialized internal weak_ptr. It builds successfully and even give the illusion that it works! I will try to explain the problem. The function shared_from_this of the class enable_shared_from_polymorphic recreates a shared_ptr using "this", the weak_ptr you pass to the shared_ptr constructor is NULL and then constructs a shared_ptr with px = this and pn.pi_ = NULL. Now if you copy the shared_from_this into a second shared_ptr while the original shared_ptr is still alive, the copy will work (this is the illusion I was talking about). However, when the original shared_ptr goes out of context, it is destroyed and if you use the copy shared_ptr (still in context), you will work on deleted memory... A good example is worth a million words. I have put a test file over there: http://spoluck.ca:8000/boost/SharedFromThisTest.cpp Debug step in it and you will see. You can set USING_VARIADIC_AND_FACTORY_METHOD to 1 to fall back to my original solution. You will need to have my original support file for that (still at the same URL). http://spoluck.ca:8000/boost/EnableSharedFromThis.h http://spoluck.ca:8000/boost/SmartPtrBuilder.h Did I used your patch erroneously? Regards, Philippe
participants (3)
-
Ansel Sermersheim -
Frank Mori Hess -
Philippe Cayouette