smart pointer / constructor design problem
Andrew Tomazos
follows:
class Handleable { int refs; }
template<T> // T is subclass of Handleable
class Handle<T>
{
T* m_p;
Handle(T* p) : m_p(p) { m_p->refs++; }
Handle(Handle<T>& that) : m_p(that.m_p) { m_p->refs++; }
~Handle() { if (m_p->refs-- == 0) delete this; }
}
(Handle also can deal with null pointers and other things but you get
the idea)
The problem is that if a Handle is created and destroyed during
construction of a Handleable object, than the ref count is reduced to
0 and "delete this" fires:
void f(Handle<C> c)
{
...
}
class C : public Handleable
{
C()
{
f(this);
}
}
Handle<C> c = new C(); // crash, f's Handle<C> parameter causes
destruction of C instance before c's constructor can increment ref
count.
I'm trying to think of a nice way around this. Ive considered having
a seperate initializing routine, but that would lose most of the
benefits of constructors. Also considered adding something to
Handleable's constuctor to help avoid this situation, but it looks
messy.
Adding the first Handle<T> to Handleable's constructor is an option...
template <class T>
Handleable::Handleable(Handle<T>& firstHandle)
{
firstHandle = this;
}
This way the first handle would be set before the construction of the
subclass and it would change the syntax as follows:
Handle<C> c; new C(c);
Again this seems messy and slow.
Any ideas?
Thanks.
Andrew.
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
ta0...@yahoo.com
> void f(Handle<C> c)
> {
> ...
>
> }
>
> class C : public Handleable
> {
> C()
> {
> f(this);
> }
>
> }
Why exactly do you do 'f(this);'? That seems flawed, mostly because
'f' doesn't retain the pointer; if it did, 'f' returning wouldn't
cause destruction because the reference count would stay > 0. That
sounds like 'f' doesn't need to be taking a 'Handle' in the first
place; it should just take a pointer. You probably need to think more
about making the cause go away rather than how to live with it.
Kevin P. Barry
Chris
> void f(Handle<C> c)
> {
> ...
>
> }
>
> class C : public Handleable
> {
> C()
> {
> f(this);
> }
>
> }
> Handle<C> c = new C(); // crash, f's Handle<C> parameter causes
> destruction of C instance before c's constructor can increment ref
> count.
>
> I'm trying to think of a nice way around this.
How about documenting this problem and leaving it up to the
programmer to do his job? I think this problem should not be
solved. Why? What they're doing is undefined behavior and
likely to crash anyway. Since the object lifetime doesn't begin
until the constructor successfully completes, if they pass the
partially constructed object to another function, they're passing
GARBAGE.
If your users write code with undefined behavior as your code
demonstrates, there is nothing you can do to recover for them.
The best thing for your user is to detect this severe flaw in
their code, and your opportunity to help is realized by crashing.
If you don't allow this crash, user code might continue to
run (for a while) in a corrupted state, making debugging much,
much harder once the bug's symptoms start showing up.
IMHO, early detection is better than late, so don't "fix" this,
as doing so only facilitates your users to write faulty,
harder-to-debug code.
Chris
Andrew Tomazos
> On Apr 17, 4:12 pm, Andrew Tomazos <and...@tomazos.com> wrote:
>
> > void f(Handle<C> c)
> > {
> > ...
>
> > }
>
> > class C : public Handleable
> > {
> > C()
> > {
> > f(this);
> > }
>
> > }
>
> Why exactly do you do 'f(this);'? That seems flawed, mostly because
> 'f' doesn't retain the pointer; if it did, 'f' returning wouldn't
> cause destruction because the reference count would stay > 0. That
> sounds like 'f' doesn't need to be taking a 'Handle' in the first
> place; it should just take a pointer. You probably need to think more
> about making the cause go away rather than how to live with it.
This is just a simple example to illustrate the mechanics of the
problem. The actual system is far more complex. For the sake of
argument suppose that f passes the Handle to a different thread
context that destroys the Handle during one of three distinct
timeframes depending on the runtime environment. Either (1) before
the constructor returns; (2) after the constructor returns and before
the assigned handle is destroyed; or (3) after the assigned handle is
destroyed. The problem occurs in case 1.
-Andrew.
Thomas J. Gritzan
> I have a standard reference counted smart pointer class abridged as
> follows:
>
> class Handleable { int refs; }
>
> template<T> // T is subclass of Handleable
> class Handle<T>
> {
> T* m_p;
> Handle(T* p) : m_p(p) { m_p->refs++; }
> Handle(Handle<T>& that) : m_p(that.m_p) { m_p->refs++; }
Handle(Handle<T> const& that) /*...*/
...unless you don't want to initialize a Handle from a temporary.
> ~Handle() { if (m_p->refs-- == 0) delete this; }
I hope that this is a typo and you wanted to delete m_p here.
Also, it should be prefix decrement:
if (--m_p->refs == 0) delete m_p;
> }
>
> (Handle also can deal with null pointers and other things but you get
> the idea)
>
> The problem is that if a Handle is created and destroyed during
> construction of a Handleable object, than the ref count is reduced to
> 0 and "delete this" fires:
>
> void f(Handle<C> c)
> {
> ...
> }
>
> class C : public Handleable
> {
> C()
> {
> f(this);
> }
> }
>
> Handle<C> c = new C(); // crash, f's Handle<C> parameter causes
> destruction of C instance before c's constructor can increment ref
> count.
That's because a class' constructor shouldn't pass around smart pointers
of itself. The constructor's job is to initialize the class instance,
and not to start a thread or to pass the instance to a function that
uses it.
If you'ld write a class D that inherits from C, C's constructor would
pass the instance to a function even before D is fully constructed.
The creator, the one who does "new C", should be responsible for passing
the instance around:
Handle<C> c = new C;
f(c);
You can put this in a factory-like function and make the constructor
protected, if you want to enforce a call to f() for every instance of C.
--
Thomas
Andrew Tomazos
> Handle(Handle<T> const& that) /*...*/
> ...unless you don't want to initialize a Handle from a temporary.
> Also, it should be prefix decrement:
Here is the real code for your review. I hope you can find as many
bugs in it. :)
#define DeclHandleable(T) class T; typedef Handle<T> H##T
class Handleable
{
public:
inline Handleable();
inline Handleable(const Handleable& that);
inline Handleable& operator=(const Handleable& that);
inline virtual ~Handleable();
inline bool isHandled();
inline void addHandle();
inline void removeHandle();
private:
volatile LONG m_iNumHandles;
};
inline Handleable::Handleable()
: m_iNumHandles(0)
{
}
inline Handleable::Handleable(const Handleable& that)
: m_iNumHandles(0)
{
}
inline Handleable& Handleable::operator=(const Handleable& that)
{
return (*this);
}
inline Handleable::~Handleable()
{
}
inline bool Handleable::isHandled()
{
return m_iNumHandles > 0;
}
inline void
Handleable::addHandle()
{
InterlockedIncrement(&m_iNumHandles);
}
inline void
Handleable::removeHandle()
{
if (InterlockedDecrement(&m_iNumHandles) == 0)
delete this;
}
template<class T>
class Handle
{
public:
Handle();
Handle(T* pTarget);
Handle(const Handle<T>& that);
template<class S>
Handle(const Handle<S>& that);
template<class S>
Handle<S> cast();
template<class S> bool operator == (const Handle<S>&) const;
template<class S> bool operator != (const Handle<S>& that) const;
template<class S> bool operator < (const Handle<S>&) const;
template<class S> bool operator > (const Handle<S>& that) const;
template<class S> bool operator <= (const Handle<S>&) const;
template<class S> bool operator >= (const Handle<S>& that) const;
~Handle();
T* ptr() const { return m_pTarget; }
T& operator*() const;
T* operator->() const;
Handle<T>& operator=(const Handle<T>&);
bool isSet() const;
bool isNull() const;
void setNull();
private:
T* m_pTarget;
};
template<class T>
Handle<T>::Handle()
: m_pTarget(NULL)
{
}
template<class T>
Handle<T>::Handle(T* pTarget)
: m_pTarget(pTarget)
{
if (m_pTarget)
m_pTarget->addHandle();
}
template<class T>
Handle<T>::Handle(const Handle<T>& that)
: m_pTarget(that.m_pTarget)
{
if (m_pTarget)
m_pTarget->addHandle();
}
template<class T>
template<class S>
Handle<T>::Handle(const Handle<S>& that)
: m_pTarget(that.ptr())
{
if (m_pTarget)
m_pTarget->addHandle();
}
template<class T>
template<class S>
Handle<S> Handle<T>::cast()
{
return Handle<S>(dynamic_cast<S*>(m_pTarget));
}
template<class T>
template<class S>
bool Handle<T>::operator == (const Handle<S>& that) const
{
return m_pTarget == that.ptr();
}
template<class T>
template<class S>
bool Handle<T>::operator != (const Handle<S>& that) const
{
return m_pTarget != that.ptr();
}
template<class T>
template<class S>
bool Handle<T>::operator < (const Handle<S>& that) const
{
return m_pTarget < that.ptr();
}
template<class T>
template<class S>
bool Handle<T>::operator > (const Handle<S>& that) const
{
return m_pTarget > that.ptr();
}
template<class T>
template<class S>
bool Handle<T>::operator <= (const Handle<S>& that) const
{
return m_pTarget <= that.ptr();
}
template<class T>
template<class S>
bool Handle<T>::operator >= (const Handle<S>& that) const
{
return m_pTarget >= that.ptr();
}
template<class T>
Handle<T>::~Handle()
{
if (m_pTarget)
m_pTarget->removeHandle();
}
template<class T>
T&
Handle<T>::operator*() const
{
return *m_pTarget;
}
template<class T>
T*
Handle<T>::operator->() const
{
return m_pTarget;
}
template<class T>
Handle<T>&
Handle<T>::operator=(const Handle<T>& that)
{
if (m_pTarget)
m_pTarget->removeHandle();
m_pTarget = that.m_pTarget;
if (m_pTarget)
m_pTarget->addHandle();
return (*this);
}
template<class T>
bool
Handle<T>::isSet() const
{
return (m_pTarget != NULL);
}
template<class T>
bool
Handle<T>::isNull() const
{
return (m_pTarget == NULL);
}
template<class T>
void
Handle<T>::setNull()
{
if (m_pTarget)
{
m_pTarget->removeHandle();
m_pTarget = NULL;
}
}
--
ta0...@yahoo.com
> This is just a simple example to illustrate the mechanics of the
> problem. The actual system is far more complex. For the sake of
> argument suppose that f passes the Handle to a different thread
> context that destroys the Handle during one of three distinct
> timeframes depending on the runtime environment. Either (1) before
> the constructor returns; (2) after the constructor returns and before
> the assigned handle is destroyed; or (3) after the assigned handle is
> destroyed. The problem occurs in case 1.
> -Andrew.
That doesn't make it any less flawed; it just demonstrates that it's
flawed in a complex manner.
Kevin P. Barry
Andrew Tomazos
> On Apr 18, 6:40 pm, Andrew Tomazos <and...@tomazos.com> wrote:
>
> > This is just a simple example to illustrate the mechanics of the
> > problem. The actual system is far more complex. For the sake of
> > argument suppose that f passes the Handle to a different thread
> > context that destroys the Handle during one of three distinct
> > timeframes depending on the runtime environment. Either (1) before
> > the constructor returns; (2) after the constructor returns and before
> > the assigned handle is destroyed; or (3) after the assigned handle is
> > destroyed. The problem occurs in case 1.
>
> flawed in a complex manner.
It is perfectly reasonable that one cannot know at compile time which
of the three cases I mentioned will account for the lifetime of the
passed this pointer. In fact, in the real system that is one the
cases. The lifetime of the smart pointer depends on the how long an
internal concurrent caching activity takes to complete, which in turn
depends on the constructor parameters and runtime environment.
Therefore I disagree with your assessment that this "smart pointer use
case" is inherently flawed.
In any case, my original question wasn't "should", it was "how". :)
-Andrew.
Frank Birbacher
Andrew Tomazos schrieb:
> Therefore I disagree with your assessment that this "smart pointer use
> case" is inherently flawed.
I agree that giving away a smart pointer to "this" from the constructor
_is_ flawed. A smart pointer relies on the object to exists (begin of
lifetime). The lifetime of an object does not begin until contruction is
finished.
> In any case, my original question wasn't "should", it was "how". :)
I suggest the same approach as Thomas already posted. You answered to
the typo issue only but not on his suggestion of a factory method.
Handle<C> makeC()
{
Handle<C> newInstance( new C );
f(newInstance); //give away after construction of C
return newInstance;
}
Frank
Andrew Tomazos
> I suggest the same approach as Thomas already posted. You answered to
> the typo issue only but not on his suggestion of a factory method.
>
> Handle<C> makeC()
> {
> Handle<C> newInstance( new C );
> f(newInstance); //give away after construction of C
> return newInstance;
>
> }
This approach is superior to a separate initializer function as the
class author can protect the constructor from being called. You still
lose the chaining of constructors, in the case some class D inherits
from C. Each subclass will have to have its own factory method and
make sure they are chained with the base classes factory.
The approach I am contemplating is to initialize the ref count of
Handleable to 1 instead of 0, and then create a factory method in
Handle, in lieu of the public Handle(Handleable*) constructor. After
the constructor of the Handleable subclass returns and is assigned to
a Handle, the ref count is then 2. The Handle factory method then
decrements it by 1 to the correct amount, before it returns the
Handle.
Roughly speaking:
class Handleable
{
Handleable() : refs(1) {};
}
class Handle
{
public:
static Handle new_() { Handle h = new Handleable(); h->refs--; }
private:
Handle(Handleable*);
}
Of course the type parameter of the Handleable subclass to be
constructed and the constructor parameters, (and implicitly their
types as type parameters), must all be passed to the Handle::new_
function in the same way as a functor-style class is implemented. But
you get the idea.
That reminds me, when do we get template<var_class T1..TN> or
similiar? It's annoying typing out one template for each number of
functor parameter types and similiar functions. We have a perl script
to automatically generate the code based on a "metatemplate", but it
is messy to maintain.
-Andrew.