How do I properly handle garbage collection and avoid memory leaks with a collection of custom object pointers...?

Question

So I'm trying to get a firm grasp of how I should code collections of objects and pointers, without causing memory leaks... I want to do something like this...

class StackTest
{
    public:
        StackTest() : m_id(-1), 
           m_name("")
        { };
        StackTest(const int id, 
           const std::string name) : m_id(id), 
           m_name(name)
        { };

        void SomeFunctionThatCanPreformTasksOnTHISStackTest();
    private:
        const int m_id;
        const std::string m_name;
}

class StackTestCollection
{
    public:
        void AddStackTest(const StackTest* test);

        void SomeFunctionThatCanPreformTasksOnAllStackTests();
    private:
        std::vector<StackTest*> m_stack_tests;
}

I then have give the collection the function AddStackTest (and make the functions that do things to the collection and the individual StackTest)...

void StackTestCollection::AddStackTest(const StackTest* test)
{
    m_stack_tests.push_back(test);
}

Finally my main code base...

// This scopes my_collection so that after the '}' my_collection should get deleted...
{
    StackTestCollection my_collection;
    my_collection.AddStackTest(new StackTest(0, "First Stack Test"));
    my_collection.AddStackTest(new StackTest(0, "Second Stack Test"));
    my_collection.AddStackTest(new StackTest(0, "Third Stack Test"));
}

Now I realize that I need to call delete on the pointers to StackTest that are stored in StackTestCollection. I imagine that I would be able to add this to StackTestCollection....

~StackTestCollection()
{
    for(auto &test : m_stack_tests)
    {
        delete test;
    }
}

Now this makes me ask a few questions...

1) Would this cause the program to crash during the loop as we are deleting the data in the heap, or would the vector still be stable as the pointer in the stack still exists, but is a dangling pointer...

2) Would I also need to run a delete on the collections m_stack_tests vector? Like this...

delete m_stack_tests;

3) In my main Code I created the collection (StackTestCollection my_collection;), it's my understanding that I only need to control garbage with pointers.... So since my_collection is not a pointer, I don't need to do something along the lines of this.... Correct?

delete my_collection;

I would like to note that this sample program would be running in a large multithread server, where a user logs in and is assigned their own collection. (So each user has their own collection on login, and on log out the data related to the user should be removed too).

Also, I'm not using C++11, so no unique_ptr's :P.

The most effective method is to not produce garbage in the first place. — juanchopanza, Oct 21 '15 at 14:19
The rule is: is thou allocate it via `new`(resp. `new[]`) thou shall delete it via `delete` (resp `delete[]`). Or you can make use of standard containers and/or smart pointers (`std::unique_ptr`) to do it for you. — Serge Ballesta, Oct 21 '15 at 14:20
Thanks Serge I was thinking that might be a good rule of thumb, but wasn't positive. — Ricky, Oct 21 '15 at 15:18

Mohamad Elghawi · Accepted Answer · 2015-10-21T14:37:52.837

2

1: This will not cause a problem as long as nothing else (another thread for example) tries to access the heap resource after it has been deleted.

2: You only need to delete something if you used new to create it.

3: You're right in saying that you do not need to delete my_collection but wrong in assuming that every pointer must be deleted, consider the following code:

void Foo(int* ptr)
{
    // do something
}

int i = 1234;
Foo(&i); // passing a pointer to i which is a stack based object

I would suggest using smart pointers to manage heap allocations/deallocations and avoid using new and delete altogether. In your case you can use a vector of unique_ptr

edited Oct 21 '15 at 14:37

answered Oct 21 '15 at 14:21

Mohamad Elghawi

2,071
10
14

The program is in a multi threaded environment, although I'm fairly certain that only the one thread access this at a time due to locks. – Ricky Oct 21 '15 at 15:31
Hmmm, I would rather use a map then a vector for the collection... std::map m_stack_tests; If I run through each StackTest and do test.second->Delete(); (Which Delete just calls delete this on the StackTest object)... The memory doesn't get freed... Any idea why? – Ricky Oct 21 '15 at 16:34
How do you know that the memory doesn't get freed? – Mohamad Elghawi Oct 21 '15 at 16:38
I'm viewing the process in task manager. I inserted a loop that made thousands of the same object (makes the program jump to 100k memory usage). The collection objects destructor gets called, and before clearing the map it runs this (auto &test : m_tests) { test.second->Delete(); } Once the destructor finishes, I check the task manager and still says 100k – Ricky Oct 21 '15 at 16:44
I would not trust what Task Manager is telling you. Check out this [post](http://stackoverflow.com/questions/14189759/freeing-memory-from-an-array) – Mohamad Elghawi Oct 21 '15 at 16:47
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/92993/discussion-between-ricky-and-mohamad-elghawi). – Ricky Oct 21 '15 at 16:54

score 0 · Answer 2 · answered Oct 21 '15 at 14:18

0

Defines you vector as being a vector of shared pointers to your type. This will allow the destructor for the shared pointer to destroy your object when the values are removed from the vector.

answered Oct 21 '15 at 14:18

Jon Trauntvein

4,453
6
39
69

score 0 · Answer 3 · answered Oct 21 '15 at 14:20

There are two things you could do:

Make your container std::vector<StackTest> m_stack_tests;. No memory handling necessary. Simply change your function to take a const StackTest& or a StackTest&&.
Make your container std::vector<std::unique_ptr<StackTest>> m_stack_tests;. Now, there is memory handling, but unique_ptr will take care of everything for you, so you don't have to worry about it.

In both cases, you don't have to write delete anywhere, which is great. The way you wrote it, you would have to delete every element in the vector one by one... but you would not have to delete the vector itself. It was not newed.

score 0 · Answer 4 · answered Oct 21 '15 at 16:39

It is not safe, in general, to delete that memory.

The reason is that the pointers are passed in from an external source, and you have no idea where they came from (well, you do, but when you write interfaces you should pretend you don't). It is the caller's responsibility to manage that memory. The is why the std::vector doesn't free the memory for you when that gets destructed.

On the other hand, your class is not safe from other holders of that pointer deleting it behind your back. The solution to both problems is to take a copy of the data in the AddStackTest function, but this is probably inefficient.

Of course, if you say, in the comment above you class,

/* This class takes ownership of the objects passed in, and will delete
   them upon destruction.  */

... now you've changed the rules and callers must take steps to copy the objects if that's not what they want.

There are various ways to implement that destruction. An explicit destructor like yours will work, and so will making your vector use std::unique_ptr.

Incidentally, your AddStackTest takes const StackTest*, but you store StackTest *, which seems wrong (and the compiler should warn you), unless you really are making a copy in there.

How do I properly handle garbage collection and avoid memory leaks with a collection of custom object pointers...?

4 Answers4