Reference Counter and destructor being called twice

Question

I am creating a class OpenGLManager, the idea of this class is to manage references to resources allocated in OpenGL (like Vertex Array Objects and Vertex Buffer Objects).

So what I want to do is whenever I allocate a new resource I add the reference into this class, and whenever the object that uses this resource is destroyed (destructor) the reference to this resource is decremented. One problem that I realized is that in some instances the destructor is called twice for an object, and that makes my code break. destructor called twice

What I am basically creating something like a shared_ptr, but instead of pointers I have handlers. Here is the code:

class OpenGLManager
{
public:
    static OpenGLManager& getInstance()
    {
        static OpenGLManager instance; 

        return instance;
    }
    // Increment reference counter
    void incrementVBO(GLuint vbo);
    void incrementVAO(GLuint vao);
    void incrementShaderProgram(GLuint program);
    // Decrement reference counter
    void decrementVAO(GLuint vao);
    void decrementVBO(GLuint vbo);
    void decrementShaderProgram(GLuint program);
private:
    std::unordered_map<GLuint, int> _vbos;
    std::unordered_map<GLuint, int> _vaos;
    std::unordered_map<GLuint, int> _programs;

    OpenGLManager() {};
    OpenGLManager(const OpenGLManager&) = delete;
    void operator=(OpenGLManager const&) = delete;

};
---------------------------------------------------------------
#include "opengl_manager.h"
using std::vector;

void lh::OpenGLManager::incrementShaderProgram(GLuint program)
{
    if (_programs.find(program) == _programs.end())
        _programs[program] = 1;
    else
        _programs[program]++;
}

void lh::OpenGLManager::incrementVAO(GLuint vao)
{
    if (_vaos.find(vao) == _vaos.end())
        _vaos[vao] = 1;
    else
        _vaos[vao]++;

}

void lh::OpenGLManager::incrementVBO(GLuint vbo)
{
    if (_vbos.find(vbo) == _vbos.end())
        _vbos[vbo] = 1;
    else
        _vbos[vbo]++;
}

void lh::OpenGLManager::decrementVAO(GLuint vao)
{
    if (_vaos.count(vao) == 0)
    {
        std::cerr << "Trying to decrement VAO: " << vao << ". Fatal Error." << std::endl;
        exit(EXIT_FAILURE);
    }

    _vaos[vao]--;

    if (_vaos[vao] == 0) // There are no more references
    {
        auto iter = _vaos.find(vao);
        if (iter == _vaos.end())
        {
            std::cerr << "Trying to remove inexistent hashmap (vao) key." << std::endl;
            exit(EXIT_FAILURE);
        }
        _vaos.erase(iter);

        glDeleteVertexArrays(1, &vao);
    }
}

void lh::OpenGLManager::decrementVBO(GLuint vbo)
{
    if (_vbos.count(vbo) == 0)
    {
        std::cerr << "Trying to decrement VBO: " << vbo << ". Fatal Error." << std::endl;
        exit(EXIT_FAILURE);
    }

    _vbos[vbo]--;

    if (_vbos[vbo] == 0) // There are no more references
    {
        auto iter = _vbos.find(vbo);
        if (iter == _vbos.end())
        {
            std::cerr << "Trying to remove inexistent hashmap (vbo) key." << std::endl;
            exit(EXIT_FAILURE);
        }
        _vbos.erase(iter);

        glDeleteBuffers(1, &vbo);
    }
}

void lh::OpenGLManager::decrementShaderProgram(GLuint program)
{
    if (_programs.count(program) == 0)
    {
        std::cerr << "Trying to decrement Program: " << program << ". Fatal Error." << std::endl;
        exit(EXIT_FAILURE);
    }

    _programs[program]--;

    if (_programs[program] == 0) // There are no more references
    {
        auto iter = _programs.find(program);
        if (iter == _programs.end())
        {
            std::cerr << "Trying to remove inexistent hashmap (program) key." << std::endl;
            exit(EXIT_FAILURE);
        }
        _programs.erase(iter);

        glDeleteProgram(program);
    }
}

This seems to work, except when I add objects on a std::vector with push_back, then the destructor is called twice, which decrements the reference by 2. How can I avoid this? Or is this just a bad design?

EDIT

Implementation of class that uses OpenGLManager

lh::RawModel::RawModel(vector<GLfloat> vertices, vector<GLint> indices, GLenum usage) 
: _vao(-1), _indicesCount(indices.size())
{
    std::cout << "CREATING MYSELF" << std::endl;
    createAndBindVAO();
    storeVerticesDataInVBO(0, vertices, 0, usage);
    storeVerticesDataInVBO(1, vertices, VERTEX_SIZE * sizeof(GLfloat), usage);
    storeIndicesDataInEBO(indices, usage);
    unbindVAO();
}

lh::RawModel::~RawModel()
{
    // Decrement reference counters for opengl resources
    std::cout << "DESTROYING MYSELF" << std::endl;
    lh::OpenGLManager::getInstance().decrementVAO(_vao);
    lh::OpenGLManager::getInstance().decrementVBO(_vbo);
    lh::OpenGLManager::getInstance().decrementVBO(_ebo);
}

const int lh::RawModel::VERTEX_SIZE = 3;

void lh::RawModel::storeIndicesDataInEBO(vector<GLint> indices, GLenum usage)
{
    glGenBuffers(1, &_ebo);
    lh::OpenGLManager::getInstance().incrementVBO(_ebo);

    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, _ebo);
    glBufferData(GL_ELEMENT_ARRAY_BUFFER, indices.size() * sizeof(GLint), &indices[0], usage);
}

void lh::RawModel::storeVerticesDataInVBO(int iAttribute, vector<GLfloat> data, int offset, GLenum usage)
{
    glGenBuffers(1, &_vbo);
    lh::OpenGLManager::getInstance().incrementVBO(_vbo);

    glBindBuffer(GL_ARRAY_BUFFER, _vbo);
    glBufferData(GL_ARRAY_BUFFER, data.size() * sizeof(GLfloat), &data[0], usage);
    glVertexAttribPointer(iAttribute, VERTEX_SIZE, GL_FLOAT, GL_FALSE, 2 * (VERTEX_SIZE * sizeof(float)), (GLvoid*)offset);
    glEnableVertexAttribArray(iAttribute);
    glBindBuffer(GL_ARRAY_BUFFER, 0);
}

void lh::RawModel::createAndBindVAO()
{
    glGenVertexArrays(1, &_vao);
    glBindVertexArray(_vao);
    std::cout << "incrementing vao " << _vao << std::endl;
    lh::OpenGLManager::getInstance().incrementVAO(_vao);
}

int lh::RawModel::getNumberOfIndices()
{
    return _indicesCount;
}

GLuint lh::RawModel::getVAO()
{
    return _vao;
}

void lh::RawModel::unbindVAO()
{
    glBindVertexArray(0);
}

Example of a call that breaks:

models_.push_back(lh::RawModel(vertices, indices, GL_STATIC_DRAW));

Show us one of your classes that actually uses the increment and decrement routines. Most likely you need to do something with its copy constructor and assignment operator as per rule-of-three issues. — TheUndeadFish, Jul 20 '15 at 22:36
You should see this [question](https://stackoverflow.com/questions/17161013/raii-wrapper-for-opengl-objects) about RAII with OpenGL. — Jean Pierre Dudey, Jul 20 '15 at 22:57

score 2 · Accepted Answer · answered Jul 20 '15 at 23:10

This is expected behavior.

Your design unfortunately isn't safe. If you create a scoped variable, increment it, copy it to a varible outside the scope, then the scoped variable will be destroyed (call decrement) and then, when you finally destroy the other variable, it will decrement a second time, crashing the program. Something like this:

RawModel rawModel;
{
    RawModel temp;
    temp.increment() //calls increment methods.
    rawModel = temp;
} // destructor called once to clean up temp;
// destroy rawModel. destructor called twice to clean up rawModel;

And this is exactly what happens, will instantiace RawModel and then passes it to vector, which copies your variable inside, now you have two instances to call the destructor and break your program.

Options: 1. (Recommended) Use shared_ptr. 2. Define copy constructor and operator =, copying the content from one RawModel to another AND incrementing the count.

Although the second options sounds easier if you have some free time, go with option 1, learn shared_ptr, it'll help you a lot in future programs.

1.
shared_ptr<RawModel> rawModel = make_shared(new RawModel);
rawModel->increment() //increment stuff
vector.push_back(rawModel);
//now it'll work.

2.
//additional RawModel methods:
RawModel(const RawModel& rawModel) {
    // copies everything and if it is initialized increment
}

const RawModel& operator =(const RawModel& other) {
    //same as copy constructur
}

Hope this helps.

score 0 · Answer 2 · answered Jul 20 '15 at 23:09

push_back stores an object by copying it. Unfortunately your RawModel will just shallow-copy its member and thus there will end up being two object referring to the same VBOs and such. That's why you're having the decrements happen too much.

So you need at the very least an appropriate copy constructor and assignment operator which do the right thing. If using C++11 then you also need a move constructor and move assignment operator. This is Rule of Three / Rule of Five stuff which every C++ developer should learn.

Since it looks like you want multiple objects to be able to share ownership, then what you need to do is:

Implement the copy/assignment operators so that they increment the reference counts as they perform the copy.
Implement the move operators so that they clear out the members of the object where the data is being moved from.

Thus reference counts should be kept up-to-date appropriately.

Reference Counter and destructor being called twice

EDIT

2 Answers2