Game Object Factory: Fixing Memory Leaks

Dear all, this is going to be tough: I have created a game object factory that generates objects of my wish. However, I get memory leaks which I can not fix.

Memory leaks are generated by return new Object(); in the bottom part of the code sample.

static BaseObject * CreateObjectFunc()
{
    return new Object();
}

How and where to delete the pointers? I wrote bool ReleaseClassType(). Despite the factory works well, ReleaseClassType() does not fix memory leaks.

bool ReleaseClassTypes()
{
    unsigned int nRecordCount = vFactories.size();
    for (unsigned int nLoop = 0; nLoop < nRecordCount; nLoop++ )
    {
        // if the object exists in the containe开发者_如何转开发r and is valid, then render it
        if( vFactories[nLoop] != NULL) 
            delete vFactories[nLoop]();
    }
    return true;
}

Before taking a look at the code below, let me help you in that my CGameObjectFactory creates pointers to functions creating particular object type. The pointers are stored within vFactories vector container.

I have chosen this way because I parse an object map file. I have object type IDs (integer values) which I need to translate them into real objects. Because I have over 100 different object data types, I wished to avoid continuously traversing very long Switch() statement.

Therefore, to create an object, I call vFactories'['nEnumObjectTypeID']'() via CGameObjectFactory::create() to call stored function that generates desired object.

The position of the appropriate function in the vFactories is identical to the nObjectTypeID, so I can use indexing to access the function.

So the question remains, how to proceed with garbage collection and avoid reported memory leaks?

#ifndef GAMEOBJECTFACTORY_H_UNIPIXELS
#define GAMEOBJECTFACTORY_H_UNIPIXELS

//#include "MemoryManager.h"
#include <vector>


template <typename BaseObject>
class CGameObjectFactory
{
public:
    // cleanup and release registered object data types
    bool ReleaseClassTypes()
    {
        unsigned int nRecordCount = vFactories.size();
        for (unsigned int nLoop = 0; nLoop < nRecordCount; nLoop++ )
        {
            // if the object exists in the container and is valid, then render it
            if( vFactories[nLoop] != NULL) 
                delete vFactories[nLoop]();
        }
        return true;
    }

    // register new object data type
    template <typename Object>
    bool RegisterClassType(unsigned int nObjectIDParam )
    {
        if(vFactories.size() < nObjectIDParam) vFactories.resize(nObjectIDParam);

        vFactories[nObjectIDParam] = &CreateObjectFunc<Object>;
        return true;
    }


    // create new object by calling the pointer to the appropriate type function
    BaseObject* create(unsigned int nObjectIDParam) const
    {
        return vFactories[nObjectIDParam]();
    }


    // resize the vector array containing pointers to function calls
    bool resize(unsigned int nSizeParam)
    {
        vFactories.resize(nSizeParam);
        return true;
    }

private:
    //DECLARE_HEAP;

    template <typename Object>
    static BaseObject * CreateObjectFunc()
    {
        return new Object();
    }


    typedef BaseObject*(*factory)();
    std::vector<factory> vFactories;
};


//DEFINE_HEAP_T(CGameObjectFactory, "Game Object Factory");

#endif // GAMEOBJECTFACTORY_H_UNIPIXELS

So the question remains, how to proceed with garbage collection and avoid reported memory leaks?

Consider using std::shared_ptr or boost::shared_ptr to manage your BaseObject pointer ownership.

This line:

delete vFactories[nLoop]();

Calls new, and then promptly deletes the object. It won't delete other objects that have been created by the factory. Does your leak detection tool give you stack trace of the allocation that wasn't deleted? If not, get one that does.

You can start by using std::shared_ptr or std::tr1::shared_ptr or boost::shared_ptr depending on your compiler.

You would use it like this:

typedef std::shared_ptr<BaseObject> BaseObjectPtr;
static BaseObjectPtr CreateObjectFunc()
{
    return BaseObjectPtr(new Object());
}

You won't need to release the created resources. They will do automatic reference counting and deallocate themselves when there are no strong references pointing to it.

So in your second code example:

#ifndef GAMEOBJECTFACTORY_H_UNIPIXELS
#define GAMEOBJECTFACTORY_H_UNIPIXELS

//#include "MemoryManager.h"
#include <vector>
#include <memory>

template <typename BaseObject>
class CGameObjectFactory
{
public:
    typedef std::shared_ptr<BaseObject> BaseObjectPtr;

    // cleanup and release registered object data types
    bool ReleaseClassTypes()
    {
        unsigned int nRecordCount = vFactories.size();
        for (unsigned int nLoop = 0; nLoop < nRecordCount; nLoop++ )
        {
            // if the object exists in the container and is valid, then render it
            //if( vFactories[nLoop] != NULL) 
            //    delete vFactories[nLoop]();
            // The above code would create something then immediately delete it.
            // You could keep a container of pointers to the objects you created
            // and loop through that instead, or use shared_ptr.
            // If you want to unregister the creator functions just NULL the pointer.
            vFactories[nLoop] = NULL;
        }
        return true;
    }

    // register new object data type
    template <typename Object>
    bool RegisterClassType(unsigned int nObjectIDParam )
    {
        if(vFactories.size() < nObjectIDParam) vFactories.resize(nObjectIDParam);

        // Store a pointer to the creation function
        vFactories[nObjectIDParam] = &CreateObjectFunc<Object>;
        return true;
    }


    // create new object by calling the pointer to the appropriate type function
    BaseObjectPtr create(unsigned int nObjectIDParam) const
    {
        return vFactories[nObjectIDParam]();
    }


    // resize the vector array containing pointers to function calls
    bool resize(unsigned int nSizeParam)
    {
        vFactories.resize(nSizeParam);
        return true;
    }

private:
    //DECLARE_HEAP;

    template <typename Object>
    static BaseObjectPtr CreateObjectFunc()
    {
        return BaseObjectPtr(new Object());
    }


    typedef BaseObjectPtr(*factory)();
    std::vector<factory> vFactories;
};


//DEFINE_HEAP_T(CGameObjectFactory, "Game Object Factory");

#endif // GAMEOBJECTFACTORY_H_UNIPIXELS

See if that helps.

The ReleaseClassTypes method is flawed:

delete vFactories[nLoop]();

is basically saying:

delete new Object();

You are deleting the object you just created, not all the objects created by calling CGameObjectFactory::create(). That said, you'll need another vector to store all the created objects so you can dump them all at once.

As others have said, look into various implementations of shared_ptr.

But if you really want to do what you think your code is doing, your create and release methods should look more like this (you'll also need a vector to store the created BaseObject*s [called vObjects below] as your current code stores only the factories, not the created objects):

public:
BaseObject* create(unsigned int nObjectIDParam)
{
    BaseObject *obj = vFactories[nObjectIDParam]();
    //I'm assuming you have error handling/detection already in code that calls this create function

    vObjects.push_back(obj);
    return obj;
}

bool ReleaseClassTypes()
{
    for (typename vector<BaseObject*>::iterator iter = vObjects.begin(); iter != vObjects.end(); ++iter) {
        if (*iter) {
            delete *iter;
            *iter = NULL; //not strictly needed, but doesn't hurt
        }
    }
    vObjects.erase();
    return true; //you might as well just convert the return type to void
}

But then you should probably code a destructor to call ReleaseClassTypes:

public:
~CGameObjectFactory() {
    ReleaseClassTypes();
}

And in a slight deviation to the Rule of Three, you'll probably want to make the copy constructor and assignment operator private to disallow copies (or you could properly define them to acquire new resources and release the old ones, but I'm not sure why you would need to copy a factory).

private:
CGameObjectFactory(const CGameObjectFactory& cgoFact) { }
CGameObjectFactory& operator=(const CGameObjectFactory& cgoFact) { return *this; }

Proposed Solution no1: Using Internal Vector for Generated Pointers

- Not Yet Working

The advices above recommend two solutions. First of them is to create another internal vector to record all the pointers generated by: return new Object();

template <typename Object>
static BaseObject* CreateObjectFunc()
{   
     return new Object();
}

The vector should be then manually deleted like in this function:

bool ReleaseClassTypes()
{
    unsigned int nRecordCount = vObjects.size();
    for (unsigned int nLoop = 0; nLoop < nRecordCount; nLoop++ )
    {
        if( vObjects[nLoop] != NULL) 
            delete vObjects[nLoop];
    }
    return true;
}

I did according to advices, and tired many other combinations as well. However, I get compile errors:

d:\source\satori\satori\gameobjectfactory.h(48): error C2663: 'std::vector<_Ty>::push_back' : 2 overloads have no legal conversion for 'this' pointer
1>          with
1>          [
1>              _Ty=CGameObject *
1>          ]
1>          d:\source\satori\satori\gameobjectfactory.h(45) : while compiling class template member function 'CGameObject *CGameObjectFactory<BaseObject>::create(unsigned int) const'
1>          with
1>          [
1>              BaseObject=CGameObject
1>          ]
1>          d:\source\satori\satori\resourcemanager.h(99) : see reference to class template instantiation 'CGameObjectFactory<BaseObject>' being compiled
1>          with
1>          [
1>              BaseObject=CGameObject
1>          ]

Here is the modified CGameObjectFactory, resulting in the compile error. Any good tip on where is the problem now, please?

#ifndef GAMEOBJECTFACTORY_H_UNIPIXELS
#define GAMEOBJECTFACTORY_H_UNIPIXELS

#include "GameObject.h"
#include <vector>


template <typename BaseObject>
class CGameObjectFactory
{
public:
    //typedef std::shared_ptr<BaseObject> BaseObjectPtr;
    // cleanup and release registered object data types
    bool ReleaseClassTypes()
    {
        //unsigned int nRecordCount = vFactories.size();
        unsigned int nRecordCount = vObjects.size();
        for (unsigned int nLoop = 0; nLoop < nRecordCount; nLoop++ )
        {
            if( vObjects[nLoop] != NULL) 
                delete vObjects[nLoop];
        }
        return true;
    }


    // register new object data type
    template <typename Object>
    bool RegisterClassType(unsigned int nObjectIDParam )
    {
        if(vFactories.size() < nObjectIDParam) vFactories.resize(nObjectIDParam);
        vFactories[nObjectIDParam] = &CreateObjectFunc<Object>;

        return true;
    }


    // create new object by calling the pointer to the appropriate type function
    BaseObject* create(unsigned int nObjectIDParam) const
    {
        BaseObject* pObject = vFactories[nObjectIDParam]();
        vObjects.push_back(pObject);
        return pObject;
    }


    // resize the vector array containing pointers to function calls
    bool resize(unsigned int nSizeParam)
    {
        vFactories.resize(nSizeParam);
        return true;
    }

private:
    //DECLARE_HEAP;

    template <typename Object>
    static BaseObject* CreateObjectFunc()
    {   
        return new Object();
    }



    typedef  BaseObject* (*factory)();
    std::vector<factory> vFactories;
    std::vector<BaseObject*> vObjects;

};


//DEFINE_HEAP_T(CGameObjectFactory, "Game Object Factory");

#endif // GAMEOBJECTFACTORY_H_UNIPIXELS

Proposed Solution no2: Using Smart Pointers

- Not Yet Working

The advices above recommend two solutions. The second solution recommends using smart pointers. I have replaced the key pointers with std::smart_ptr as the code below displays:

#ifndef GAMEOBJECTFACTORY_H_UNIPIXELS
#define GAMEOBJECTFACTORY_H_UNIPIXELS

#include "GameObject.h"
#include <vector>
#include <memory>


template <typename BaseObject>
class CGameObjectFactory
{
public:
    typedef std::shared_ptr<BaseObject> BaseObjectPtr;

    // cleanup and release registered object data types
    bool ReleaseClassTypes()
    {
        unsigned int nRecordCount = vFactories.size();
        for (unsigned int nLoop = 0; nLoop < nRecordCount; nLoop++ )
        {
            // if the object exists in the container, then delete it
            if( vFactories[nLoop] != NULL) vFactories[nLoop] = NULL;
        }
        return true;
    }


    // register new object data type
    template <typename Object>
    bool RegisterClassType(unsigned int nObjectIDParam )
    {
        if(vFactories.size() < nObjectIDParam) vFactories.resize(nObjectIDParam);
        vFactories[nObjectIDParam] = &CreateObjectFunc<Object>;

        return true;
    }


    // create new object by calling the pointer to the appropriate type function
    BaseObjectPtr create(unsigned int nObjectIDParam) const
    {
        return vFactories[nObjectIDParam]();        
    }


    // resize the vector array containing pointers to function calls
    bool resize(unsigned int nSizeParam)
    {
        vFactories.resize(nSizeParam);
        return true;
    }

private:
    //DECLARE_HEAP;

    template <typename Object>
    static BaseObjectPtr CreateObjectFunc()
    {   
        return BaseObjectPtr(new Object());
    }



    typedef  BaseObjectPtr(*factory)();
    std::vector<factory> vFactories;

};


//DEFINE_HEAP_T(CGameObjectFactory, "Game Object Factory");

#endif // GAMEOBJECTFACTORY_H_UNIPIXELS

The code gets compiled fine. However, I get error saying:

Debug Error!

HEAP CORRUPTION DETECTED: after Normal block(#949) at 0x04DC7E68.

CRT detected that the application wrote to memory after end of the heap buffer.

Memory allocated at: filepath to line in CGameObjectFactory: return BaseObjectPtr(new Object());*

I have three options in the Popup window: Abort, Retry, Ignore

If I repeadetly press Ignore, the vector container of allocated game pointers gets apparently deleted, because, I'll end up with NO MEMORY LEAKS. On the other hand, if I press Abort, I end up with my memory leaks again.

Any idea what that might indicate? I gues, I haven't done any specialties in my code, so I completely do not understand this behavior.