Who is responsible for delete?

Question

I was analyzing a code and I am confused on a particular code. I have posted the code/pseudo-code which will convey the same meaning.

Class 1

Class1::Func1()
{
    Collection* cltn;   
    try
    {
        cltn = Class2::get_records_from_db();
    }
    catch(Informix error)
    {}
    catch(DB Error)
    {}
    catch(...)
    { Unknown exception }          //I get this error always once the process processes lot of records   
}

Class 2

Collection* Class2::get_records_from_db()
{
    Collection *clt = new Collection();
    try
    {

        //Query database
        For each row in query result
        Row *row = new row();
        populate(row)
        clt->add(*row)

         ...
        if( Informix error)
        {
            throw Informix error;
        }  
     }

     catch(...)
     {
          delete clt;  //Who will delete row?
          clt = 0;
          throw Db error 
     }

    return clt;  //Who will delete clt?
}

Problem - PART 2

Thanks for the insights on the first problem. Now here is the real problem which is happening.

Class 1 is a C++ process and Class 2 is a library which talks to Informix database. Class2::get_records_from_db() is a function which queries an Informix DB and returns the result-set. I have enhanced the above code which is more similar to the real code.

Collection objects deals with 200k of row objects, which as most of you said is not released properly.
The caller is seeing "Unknown exception" in the general catch block. Can that be because of the huge memory leaks created in Class 2?

I also see some Informix errors 406 (Out of memory error) in the logs. The process core-dumps after spitting out a series of Unknown Exception & SQLERR406

I want to know whether the core dump is a byproduct of the memory leaks.

If the code is actually as what you are showing, there are memory leaks. — David Rodríguez - dribeas, Jun 20 '12 at 12:41
In `get_records_from_db` the `row` variable should definitely be deleted inside the `catch`. Also, since `clt` is returned is should be set to `NULL` inside the `catch` or there will be a dangling pointer. After the function returns, it should be the callers responsibility to delete returned allocated memory in my opinion. However, it must be well documented so the user of the API will know this. — Some programmer dude, Jun 20 '12 at 12:43
-1 asking for deallocation responsibility and **not documenting the return type**. — Cheers and hth. - Alf, Jun 20 '12 at 12:49
This is what I wanted clarity on. If the caller do `delete cltn`, will it release `clt` which was allocated in the callee function? — cppcoder, Jun 20 '12 at 12:56
@Alf - However, the `row` cannot be released by the caller. If I am right, it should be done inside the `Collection` destructor? — cppcoder, Jun 20 '12 at 16:52

score 12 · Accepted Answer · edited May 23 '17 at 12:29

What is the problem with the code you presented?

The code example you present is a very bad and wrong code.

No one deletes either of (row and clt) them. This leads to a memory leak or a Undefined Behavior depending on whether their destructors have trivial or nontrivial implementation.Either way it means very bad things can happen.

If you allocate an object using new you need to explicitly deallocate it by calling delete on the pointer returned by new. Since you do not call delete on either of the pointers, they both are never deallocated at all.

Who should be responsible for delete?

The objects themselves!
The objects should have an inbuilt functionality to de-allocate themselves as soon as their scope({,}) ends. This way no one needs to explicitly deallocate any of the objects, but they get implicitly deleted once they are not needed anymore. This technique is popularly known as Resource Allocation is Initialization(RAII) or Scope Bound Resource Management(SBRM) in C++.

Each of your objects(row and clt) should be using a RAII by writing wrappers over these raw pointers or even better simply by using readily available Smart pointers.

@Nawaz: C++11 3.8 Object lifetime, Para 4: *"....however, if there is no explicit call to the destructor or if a delete-expression (5.3.5) is not used to release the storage, the destructor shall not be implicitly called and any program that depends on the side eﬀects produced by the destructor has undeﬁned behavior."* Hence my answer says *"depending on whether their destructors have trivial or nontrivial implementation."* — Alok Save, Jun 20 '12 at 13:24

score 2 · Answer 2 · answered Jun 20 '12 at 12:46

Smart pointers are what you need. You should put each new Row into std::shared_ptr<Row> row instead of a pointer; those shared_ptrs will be automatically cleaned up when they go out of scope (eg, when the try-catch block exits).

What you should do with 'clt isn't quite so clear cut... I'd be tempted to store it in a std::unique_ptr<Collection> and return that because then it is clear that a) it will be automatically deleted at some point (potentially when your program exits) and b) it is clear to calling code that they now own the value returned by get_records_from_db(), not the Class2 instance (or singleton) that generated it.

Clear ownership semantics are a good thing.

Who is responsible for delete?

2 Answers2