[boost] [smart_ptr] enable_shared_from_this and multiple inheritance solution
Philippe Cayouette
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
>(new Derived());
}
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
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Ansel Sermersheim
> 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]
>
> class Derived : public Base1, public Base2
> {
> public:
> // Factory method
> static boost::shared_ptr< Derived > Create()
> {
> return Generic::SmartPtrBuilder::CreateSharedPtr< Base1, Base2
>>(new Derived());
> }
> 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
Philippe Cayouette
> 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.
Ansel Sermersheim
> 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
Frank Mori Hess
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-----
Ansel Sermersheim
> 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
Philippe Cayouette
>> 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
Philippe Cayouette
>>>> 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).
Did I used your patch erroneously?
Regards,