Trying to avoid double-destruction and inadvertently triggering it


We saw some time ago the importance of artificially bumping an object's reference count during destruction to avoid double-destruction. However, one person's attempt to avoid this problem ended up triggering it.

ULONG MyObject::Release()
{
 LONG cRef = InterlockedDecrement(&m_cRef);
 if (cRef > 0) return cRef;
 m_cRef = MAXLONG; // avoid double-destruction
 delete this;
 return 0;
}

The explanation for the line m_cRef = MAXLONG was that it was done to avoid the double-destruction problem if the object receives a temporary AddRef/Release during destruction.

While it's true that you should set the reference count to an artificial non-zero value, choosing MAXLONG has its own problem: integer overflow.

Suppose that during the object's destruction, the reference count is temporarily incremented twice and decremented twice.

Action m_cRef
Just before call to Release() 1
InterlockedDecrement 0
m_cRef = MAXLONG 2147483647
destructor does temporary AddRef() −2147483648 (integer overflow)
destructor does temporary AddRef() −2147483647
destructor does temporary Release() −2147483648
since m_cRef < 0, we re-destruct

Sure, choosing a huge DESTRUCTOR_REFCOUNT means that you have absolutely no chance of decrementing the reference count back to zero prematurely. However, if you choose a value too high, you introduce the risk of incrementing the reference count so high that it overflows.

That's why the most typical values for DESTRUCTOR_REFCOUNT are 1, 42, and 1000. The value 1 is really all you need to avoid double-destruction. Some people choose 42 because it's cute, and other people choose 1000 because it's higher than any "normal" refcount, so it makes it easier to spot during debugging. But even then, the "high" value of 1000 still leaves room for over two billion AddRef()s before overflowing the reference count.

On the other hand, if you choose a value like MAXLONG or MAXDWORD, then you're taking something that previously never happened (reference count integer overflow) and turning it into an almost certainty.

Comments (34)
  1. kog999 says:

    i set my DESTRUCTOR_REFCOUNT’s to 1337 because its cooler then 42

  2. porter says:

    #define DESTRUCTOR_REFCOUNT 911

    I would suggest (MAXLONG>>2)

  3. someone else says:

    To find something in the debugger (or worse, a hex dump), “words” appear more practical. Like DEADBEEF, F001 or B00B.

  4. Alexander Grigoriev says:

    “While it’s true that you should set the reference count to an artificial non-zero value”

    Isn’t this one of those false truths? Your desructor code should never do AddRef/Release. The destructor should never pass the object pointer away, this is why AddRef should never be called. Put an ASSERT(ref_cnt != 0) to AddRef.

    [In other words, an object which has persistence shouldn’t try to save itself when destructed, but rather should save itself whenever a property changes? -Raymond]
  5. Curly’s Law to the rescue…

    Instead of overloading the m_cRef variable, create a boolean m_bBeingDestroyed variable.  Initialize it to false, and set it to true when the reference count first drops to 0.

  6. John says:

    I think it would be easier if the interface for the object exposed a Save method which you had to explicitly call in order to save it.  If you insist on doing stuff in your destructor which could result in calls to AddRef and Release (I try to avoid this whenever possible), then I think it would be easier to use a separate flag rather than trying to do tricks with the ref count:

    ULONG MyObject::AddRef()

    {

    if (m_fDestroying) return 1;

    assert(m_cRef != 0);

    return InterlockedIncrement(&m_cRef);

    }

    ULONG MyObject::Release()

    {

    if (m_fDestroying) return 1;

    LONG cRef = InterlockedDecrement(&m_cRef);

    if (cRef > 0) return cRef;

    m_fDestroying = TRUE; // avoid double-destruction

    delete this;

    return 0;

    }

  7. Alexandre Grigoriev says:

    [In other words, an object which has persistence shouldn’t try to save itself when destructed, but rather should save itself whenever a property changes? -Raymond]

    Raymond,

    It all comes from wrong use of AddRef. You should not call it if you don’t need to save the object for later use. When your object calls its own methods, such as Save, it should not do AddRef.

    At destructor time, you should not be passing your object around. For all purposes, it’s gone, it’s behind an event horizon. You should not pass its interface pointer to other functions. You can pass its data pointers around, for saving, because you still keep their references inside your soon-to-die object. But all of your object’s interfaces are out of this world now. QueryInterface should also ASSERT on zero refcount.

    [“You should not pass its interface pointer to other functions.” Translation: Correct, an object should not attempt to auto-save. Require clients to call an explicit Save method. (And if there’s more than one client, they need to coordinate among themselves who is the one to call Save.) Feel free to use this design pattern in your code. -Raymond]
  8. porter says:

    Seeing as COM objects generally do not restore their persistence in IClassFactory::CreateInstance(), because it had no idea where it’s persistence was, then similarly persistence should not be dependent on the last arbitrary IUnknown::Release().

    If you then had a client using a garbage collected language (hmm, I wonder if MS encourages the use of a lanuage with garbage collection and COM interoperability) then your last release and hence persistence is dependent on the whim of a foreign garbage collector.

  9. What John said but I’d code it a little differently:

    ULONG MyObject::AddRef()

    {

    ____ LONG cRef = InterlockedIncrement(&m_cRef);

    ____ assert(cRef >= (m_fDestroying ? 1 : 2));

    ____ return cRef;

    }

    ULONG MyObject::Release()

    {

    ____ LONG cRef = InterlockedDecrement(&m_cRef);

    ____ assert(cRef >= 0);

    ____ if (cRef == 0 && !m_fDestroying) {

    ____ ____ delete this; // calls destructor

    ____ }

    ____ return cRef;

    }

    MyObject::~MyObject()

    {

    ____ m_fDestroying = TRUE;

    ____ // do what you like here

    ____ // including AddRef() and Release()

    }

  10. Oh, and tack on an "assert(m_cRef == 0);" on the end of that destructor.

    This begs the question… if you’re giving a pointer to an object away in its destructor, what do you do if one of the things you gave a pointer to in the destructor holds on to it?  do { Sleep(…); } while (m_cRef > 0); ?

  11. Adam says:

    Let’s hope future programming languages will be able to avoid this COLLECTION of GARBAGE.

  12. Adam says:

    That code is ugly and barely readable. Let’s hope future programming languages will find away to avoid this COLLECTION of GARBAGE.

  13. porter says:

    > That code is ugly and barely readable.

    If that code is ugly and barely readable then I hope that you are not a programmer!

    There are two commonly used solutions to ugly code: hide it in a library or in a base class.

  14. ulric says:

    I’m thinking that all of these uses of InterlockedDecrement (in the article and the comments) are moot if you aren’t going to bother making the entire function threadsafe. am I right

    I never use Interlock* if I haven’t actually thought the threadsafety through – don’t want to give a false impression.

  15. porter says:

    > I’m thinking that all of these uses of InterlockedDecrement (in the article and the comments) are moot if you aren’t going to bother making the entire function threadsafe. am I right

    If you are only using the interlocked functions correctly for your usage count then they will ensure that only one thread gets the case of InterlockedDecrement return zero, so that single thread can safely call the destructor. That means you don’t need a mutex around your constructor or destructor if you only hand out a reference to the object after construction and the only single place that delete is called is in the IUnknown::Release when InterlockedDecrement has returned zero. In comparison to many examples on the net, I also have my constructor set the usage to 1 in the first place, so the life cycle sequence is "new" then "Release", not "new", then "AddRef" then "Release".

  16. Alexander Grigoriev says:

    RC: [Translation: Correct, an object should not attempt to auto-save.]

    I don’t quite get why you bring auto-save. To auto-save, the destructor can call whatever external function it needs to write the object’s persistent state out. This doesn’t mean the destructor needs to call some external function that needs to take the object’s pointer. If you mean the auto-save function should be external to the object, it’s wrong design. The object should know how to save itself, without exposing it to the outside. The external functions only serve to save its members – strings, numbers, etc.

    Again, auto-save function doesn’t need to pass the object’s pointer away; it should be solely an internal business. Then the problem of AddRef at destruction time won’t byte your behind.

    [It means that any function called in the destructor may need to have two versions, one that calls AddRef and Release when it copies the pointer (for use during normal conditions), and one that doesn’t (for use during destruction). The no-AddRef/Release version can’t use smart pointers and it can’t call QI. I bring up auto-save because it is not unusual for Save() to hand out the pointer temporarily, for example for use as a site, or to call QI (e.g. to QI for IPersist). -Raymond]
  17. porter says:

    > [It means that any function called in the destructor may need to have two versions, one that calls AddRef and Release when it copies the pointer (for use during normal conditions), and one that doesn’t (for use during destruction).

    What is wrong with doing

    ULONG Release()

    {

        if (!InterlockedDecrement(&usage))

        {

             usage=REFCOUNT_DESTRUCT;

             Save();

             assert(usage==REFCOUNT_DESTRUCT);

             delete this;

        }

        return 0;

    }

  18. Neil says:

    According to the mantra, every programming problem can be solved by an extra layer of indirection. So the solution is obviously to create a new object whose job it is to save the original object but without passing the original object around (so the new object implements IPersist or whatever interfaces are necessary). Once the new object has done its job it can then delete the original object in its destructor.

  19. Médinoc says:

    @Alexander, @Porter: The problem is, your save method can do this:

    http://blogs.msdn.com/oldnewthing/archive/2005/09/27/474384.aspx

    SetSite() calls AddRef(), and some other methods can call QueryInterface() (which does an AddRef()) on your object…

  20. GWO says:

    Good grief.  I can’t believe good programmers would recommend such a hideous kludge as The Way to prevent double destruction.  Anything that requires this kind of workaround needs a redesign (or, I suspect,  lacked a initial design).

  21. !GWO says:

    @GWO:

    Very funny! Ha ha ! "I don’t understand the problem or the solution, but its wrong ! wrong wrong wrong ! And I cant think of a better design! That makes me smart ! Whee !"

  22. Joseph Koss says:

    porter:

    In regards to MS recommending a language with COM Interop and garbage collection…

    The first MS language I used that fit that description was Visual Basic 6.0, and VBA is still going strong.

  23. PhilW says:

    Actually 42 isn’t just cool, it’s the answer to the ultimate question of life the universe and everything. That’s why it has snuck into the geek vocabulary, as any Douglas Adams reader would grok (sneaking in another SF reference).

  24. porter says:

    > (And if there’s more than one client, they need to coordinate among themselves who is the one to call Save.) Feel free to use this design pattern in your code. -Raymond

    So lets guess why Microsofts distributed COM transaction server requires stateless objects.

  25. porter says:

    @Médinoc

    If your Save *may* do that then you have end up with…

    Release()

    {

       while (!InterlockedDecrement(&usage))

       {

          if (fDirty)

          {

              AddRef();

              Save();

          }

          else

          {

              delete this;

              break;

          }

       }

    }

    So what to do if Save fails and fDirty is still true? Funnily enough that is one of the fundamental problems of attempting to Save on Release().

  26. bmm6o says:

    Joseph Koss: porter was suggesting C#/VB.Net, I believe.  VB 6 calls Release as soon as each local reference is set to nothing, resulting in generally deterministic behavior.

  27. grok (sneaking in another SF reference)

    … and a stranger one.

  28. Médinoc says:

    @GWO: So, it would need a version of QueryInterface that does not add a reference?

    …Good point, actually.

  29. Daniel says:

    PhilW: yes, and you can be sure that the odds of someone who visits this blog getting the reference would be *quite* high. Raymond just didn’t bother to point out the obvious. Many geeks enjoy subtlety, and as one of them myself, I appreciate the way he described the number.

  30. avek says:

    Sorry, but something seems wrong with all this to me.

    COM objects are, in effect, garbage collected. (Yeah, I know, we don’t call reference counting GC these days. But it’s still one of the GC methods, and it’s mentioned among GC methods in any theoretical talk). As result of that, COM objects don’t have any destructors, when we look at them as COM objects, universal, re-usable and callable from any language. They have Release, of course, but it’s not a destructor.

    Now, implementation of a COM object may have a… let’s call it finalizer – private method that Release calls when object actually becomes dead (again, that’s not a destructor). And when implementing COM objects in C++, the finalizer usually has to invoke a true C++ destructor at some point, because C++ objects die that way. Without a call to C++ destructor, there’s just no good way to clean object internals up, so let it be – the more, the merrier.

    But if the finalizer does something to the object as to a COM object, like giving its interfaces to other objects, then THE OBJECT MUST STILL BE A LIVE ONE when it goes outside of Release/finalizer. And if it’s not a live one, it must be made live by force. That’s similar to what .NET does with its finalizers: if one needs to be called, the object is not collected and turned back into a first-class live one instead, then it’s given to its finalizer, and the object lifetime prolongs until the next collection. On the next collection, the object may again turn up dead and be collected. Or it may become used again and get stuck in the heap, thus the finalizer will in fact have no "finalizing" effect on it.

    Every C++ destructor is always one-shot and irreversible, unlike .NET finalizers; it always makes the object unusuable when called. So, it can’t do the job of a GC finalizer. Ergo, when Release revives the object it may NOT call the C++ destructor (and no operator delete, too). Period. Some special finalizer may be invoked instead, but not a destructor.

    It may seem that I’m nitpicking. But, you see, it’s all actually very important because there are many abstractions here, and they tend to mix, and can begin to leak if not properly separated from each other. So unconditionally calling the C++ destructor from Release at zero refcount, when some complex COM-based finalization is due – that’s just wrong.

    The proper algorithm of Release should be something like this (very sketchy):

    Release()

    {

     TRefCount newRefCount = DecrementRefCount();

     if (newRefCount != 0) return newRefCount;

     if (objectStillHasSomethingToDo)

     {

       AddRef();

       DoSomething();

       return Release();

     }

     delete this;

     return newRefCount;

    }

    DoSomething() is Save() or anything else that can be done with object at finalization time (we can play some music there, for example ;) DoSomething MUST be wrapped by AddRef() and return Release(), AddRef revives object back to working state and Release undoes that, any attempts to save some CPU ticks by removing those is a hack. The recursion can be optimized away, it will give something like porter’s version above. But from high-level point of view, all of those steps must be done. As they used to say here in Russia, "step to the left, step to the right is an escape attempt…" :)

    It all may seem excess. Technically, you can always squeeze non-destructing finalizer, DoSomething, and destructor into a single entity (which can only be the C++ destructor, of course).

    But, you see, such a mixture is actually a hack that mixes two very different abstractions – GC finalizers and C++ destructors (that aren’t simply interchangeable). And setting reference count directly is even more a hack. You should never set it directly. You should not touch the reference count field at all except when in constructor, AddRef or Release (and the finalizer or destructor code doesn’t count as "in Release"). Because if you set it to, say, 42, it’s not a reference count anymore, it’s "reference count except when the object runs its destructor, in that case it’s reference count + 41" (wait, or + 42? or +43? can you really tell?). You’ve lost your abstraction for an optimization. And is there really any benefit from this optimization?

    What’s worse, this optimization is not correct in general case (again, broken abstraction come back to haunt you). Because, you see, DoSomething could use the object after it returns. Yeah, we can define contract for DoSomething so it doesn’t do such nasty things. But, wait, how can we? If it does something beyond trivial, like saving the object to persistent storage or playing some music dependent on object’s data, then it could be given to us as a ready-to use black box. Interfaces in, interfaces out, ready to use implementation, no modifications accepted. That’s, actually, more a norm of component-based development than an exception, AFAIK.

    With proper reviving, such a more complex scheme will just work. The only thing needed is correct reference counting in the other component: it takes reference to the revived object, it holds it until it’s done with the object, and voila, the C++ destructor call is properly delayed.

    With the dirty hack by calling destructor always and setting refcount to 42 there (or 1000, or 1) the C++ destructor of our object will ignore the reference count and kill the object instantly. While another component still works on it somehow. Craaashhh.

  31. GWO says:

    @!GWO: “Very funny! Ha ha ! “I don’t understand the problem or the solution, but its wrong ! wrong wrong wrong ! And I cant think of a better design! That makes me smart ! Whee !””

    I do know the solution.  The solution is properly designed ownership semantics, and programmers who understand notions of ownership.  

    By the time the destructor runs, there should be no need to ever add an additional reference to this object.  If you don’t understand why this is the case, you don’t understand what a destructor is for.  No destructor should call a function that believes it owns the thing it receives.  To do so is very bad design, and no matter how you lie to the reference count, it *will* come back and bite you.

    [You’re missing the forest for the trees. Fine, we stop doing scary stuff in the destructor by rewriting Release like this:
    ULONG MyObject::Release()
    {
     LONG cRef = InterlockedDecrement(&m_cRef);
     if (cRef > 0) return cRef;
     Finalize(); // disconnect handlers, write out unsaved changes, etc.
     delete this;
     return 0;
    }
    

    All the problems we had in the destructor are now problems in Finalize(). -Raymond

    ]

  32. Alexandre Grigoriev says:

    @avek,

    That’s the most reasonable thing.

    Even then, suppose you called the finaliser, and some of its code path (like Save to remote HTTP host) will need to hold the object ref). You then need to make sure all interfaces are OK after the finaliser returned. And god forbid the finaliser fails. Then you just have to accept that "when axing a forest, the chips get scattered around".

  33. Thinking about this, it seemed kind of unfair that the last person to release the object got stuck waiting around while the object did its finalization.

    So I came up with the following idea.  When you’ve detected that your last reference is being released, schedule a work item to finalize the object, including deleting it, and return 0.

    That work item can assume that no other threads have a reference to the object when it starts.  It still gets complicated if one of the things that gets a reference to the object during cleanup decides to hold on to the reference, though.

  34. 640k says:

    No it’s not "unfair". It’s how refcount works and how library users expect it to work. By changing this behaviour you just make the inner of your library more obscure.

Comments are closed.

Skip to main content