We had a head-scratcher bug the other day. Our system is written in C++, and has a custom reference counting system for managing memory (see Greenspun's Law). We were seeing an assertion that we had thought was long debugged and solid. Here's what we found. (Warning: lots of arcane C++ stuff ahead.)

Our reference counting implementation looks kind of like this:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
/**
 * Reference-counted object.  When its reference count goes to
 * zero, it destroys itself.
 */
class CRefCountedObject
{
public:
    CRefCountedObject()
    {
        m_nRefCount = 0;
    }

    virtual ~CRefCountedObject()
    {
        // CRefCountedObject's can be created on the stack,
        // where they will be destroyed by falling out of scope.
        // This assert checks the implicit assumption that no one
        // is still interested in the object when it is destroyed.
        ASSERT(m_nRefCount == 0);
    }

    /** Declare your interest in this object: ups the refcount.
     */
    void Retain()
    {
        m_nRefCount++;
    }

    /** Declare your lack of interest in this object: if you were
     *  the last interested party, the object is destroyed.
     */
    void Release()
    {
        m_nRefCount--;
        if (m_nRefCount== 0) {
            delete this;
        }
    }

private:
    int m_nRefCount;
};

/**
 * Smart pointer, templated to a particular object class.
 */
template <class TObject>
class CPtr
{
public:
    // .. lots of stuff omitted ..

    /** Assignment operator.  Manage the refcounts on the old and
     *  new objects.
     */
    TObject *operator=(TObject *p)
    {
        if (m_p != p) {
            if (p != NULL) {
                p->Retain();
            }
            if (m_p != NULL) {
                m_p->Release();
            }
            m_p = p;
        }
        return m_p;
    }

    //.. lots of stuff omitted ..
};

(Note: this is a simplified version of our actual code, and some of the simplifications mean that it will not work properly, but not in ways that affect the narrative here. For example, don't try this code with multiple threads, or with multiple inheritance. I'm taking some expository license. Don't bug me about it!)

The assert that fired is on line 19: it's designed to protect against allocating reference counted objects on the stack. The problem with a stack-allocated refcounted object is that the object will be destroyed when it falls out of scope, regardless of the reference count. For our reference counting to work right, the only thing that should destroy an object is the delete on line 36. With a stack allocated object, the compiler destroys the object for us, so smart pointers can exist which point to freed memory:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
{
    CPtr<CRefCountedObject> pObj;

    //.. blah blah ..

    if (blah blah) {
        CRefCountedObject stackobj;
        pObj = stackobj;
    }

    // pObj is invalid at this point.
}

The object stackobj is created at line 106, and then destroyed at line 108 (when it falls out of scope), but pObj still has a pointer to it. When stackobj is destroyed, its reference count is 1, so the assert in the CRefCountedObject destructor will fire. All is well.

So when that assertion fired the other day, we thought we understood the problem. Just look up the stack, find the stack allocated refcounted object, and change it to a heap allocated object. But when we looked into it, there was no stack allocated object. So who was destroying the refcounted object before its time? How does a heap allocated object get destroyed other than using delete on it?

Digging deeper, we rediscovered a C++ detail that we had forgotten. Turns out we had some code that boiled down to this:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
class CBadObject: public CRefCountedObject
{
    //.. blah blah ..
};

CPtr<CBadObject> pGlobalBad;

CBadObject::CBadObject()
{
    pGlobalBad = this;

    DoSomethingWhichThrowsAnException();
}

This object's constructor assigned itself to a smart pointer outside itself, then threw an exception. C++ semantics state that if a constructor of a heap allocated object throws an exception, that the object's destructor is called. Since CBadObject derives from CRefCountedObject, the CRefCountedObject destructor is called. It checks the reference count, sees that it is not zero (it's one, because pGlobalBad has called Retain, but hasn't called Release), and fires the assertion.

Updated May 2005: Actually, the correct statement of C++ semantics is that if a constructor throws an exception, a destructor is called for all the base classes that had been successfully constructed. In this case, the CBadObject destructor is not called (because the CBadObject constructor didn't finish), but the CRefCountedObject destructor is called, because its constructor completed. It's the CRefCountedObject destructor which causes the trouble here. More about this at More C++ constructor trivia.

So the lesson is: try not to throw exceptions from constructors, or at least understand that they will destroy the object for you.

Updated May 2005: Again, this is not correct. The real lesson is that throwing exceptions from constructors is perfectly legal, and C++ is precisely engineered to specify exactly what will happen. But you may be surprised by what happens. In particular, you may have objects in "impossible" states.

It just goes to show you:

You learn something new every day, no matter how hard you try.

By the way: there were two things we did to fix the problem. Do you know what they were? The first was to move most of the code out of the constructor, so that hard stuff happens in regular methods. The second was to add to the comment in CRefCountedObject's destructor:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
virtual ~CRefCountedObject()
    {
        // CRefCountedObject's can be created on the stack,
        // where they will be destroyed by falling out of scope.
        // This assert checks the implicit assumption that no one
        // is still interested in the object when it is destroyed.
        // The other way this can happen is if a constructor
        // fails, but it has already assigned itself to a CPtr
        // someplace.  When a C++ constructor fails, the compiler
        // automatically destroys the object.
        ASSERT(m_nRefCount == 0);
    }

That way, future generations might be spared the head-scratching.

tagged: , , » 8 reactions

Comments

[gravatar]
Alan Hohn 9:03 AM on 2 Dec 2004

I would take issue with a constructor for a reference counted object that makes the refcount increase in its constructor. Usually, the code that creates the object should be the one refcounting it. If you really need a global instance of this object, shouldn't you use the singleton pattern and have a service manager maintain the (single) refcount?


Also, your global variable will contain the last CBadObject created. Only an implicit constraint on the system (only one location in the code where a CBadObject is created) will ensure that pGlobalBad continues pointing to the same instance.

[gravatar]
Ned Batchelder 9:44 AM on 2 Dec 2004

Sorry, the example isn't quite clear. The point of the global pointer wasn't to hold a singleton instance. In the real code, the constructor is registering itself with a facility which holds pointers to a number of objects. In trying to simplify the code for the sake of explanation, I made it harder to understand!

You are right: singletons should be done differently.

[gravatar]
christopher baus 10:07 PM on 2 Dec 2004

Ned,

This is off topic, but what do you use to format you C++ into HTML? It looks really nice.

[gravatar]
Ned Batchelder 6:23 AM on 3 Dec 2004

Thanks, I use SilverCity, with some local tweaks for line numbering.

[gravatar]
Chui 2:55 PM on 3 Dec 2004

Here is an equivalent python clanger:


l = Lock()
class ScopedLock:
"Lock released with it goes out of scope"
def __init__:
l.acquire()
def __del__:
l.release()

def accessLockedResource():

locallock = ScopedLock()
.... lots of complicated stuff here ...


The problem we encountered was when accessLockedResource threw an exception, the lock was never released. It wasn't evident until we realized that sys.traceback holds all the value of each frame, and each frame includes all the local variables. Therefore, the lock wasn't getting released because the locallock hasn't been destroyed.

[gravatar]
Joe Ganley 10:17 AM on 3 May 2005

"C++ semantics state that if a constructor of a heap allocated object throws an exception."

Where did you find this fact? Stroustrup, other sources (e.g. this), and my own experience (VC++ under Windows) indicates just the opposite: If the constructor throws an exception, the destructor is not called.

[gravatar]
Ned Batchelder 9:35 PM on 12 May 2005

Joe, you are absolutely right! I've added an update above, with a link to a new entry I wrote about it.

[gravatar]
George Philip, Elevatta 1:02 PM on 1 Mar 2007

I dont think the destructor is called If the constructor throws an exception
http://www.georgepsblogs.blogspot.com

Add a comment:

name
email
Ignore this:
not displayed and no spam.
Leave this empty:
www
not searched.
 
Name and either email or www are required.
Don't put anything here:
Leave this empty:
URLs auto-link and some tags are allowed: <a><b><i><p><br><pre>.