Problem with destructor not being executed

I'm having a weird problem with regard to a destructor which fails to execute when I call delete on the instance.

I have a typedef:

typedef struct STITCHREQUEST
{
  CImage* Image;
  CImageCollection* Collection;

  STITCHREQUEST(CImage* a_pImage, CImageCollection* a_pCollection)
  {
     Image = a_pImage;       // I now own this
     Collection = pCollection; // I do not own this
  };

  ~STITCHREQUEST()
  {
    delete Image;
    Image = NULL;
    Collection      = NULL;
  };
} STITCHREQUESTTTYPE;


Basically I have a process which is capturing frames, and it creates these "stitching requests" which is passed into a worker thread, which saves the image and attaches it to the collection. The process that launches the stitching thread instantiates the CImage but the instance is to be destroyed by the ~STITCHREQUEST() destructor so that it is not killed off before stitching is complete.

I set it up like this:

CImage* pImage = new CImage(hbmFrame);
STITCHREQUEST* pRequest = new STITCHREQUEST(pImage, m_pCollection);  

Anyway -  the stitching and such all works fine. The problem is I have a memory leak, due to ~CImage() never being executed, inspite of the fact 'delete Image;' within ~STITCHREQUEST() is being executed. I have put breakpoints in both destructors, and ~STITCHREQUEST() is executed, both pointers are set to null,  but I am very certain that ~CImage() is never run.

Any idea why the CImage destructor isn't called inspite of the fact that: delete Image; is executed??? The CImage instancing and destruction work fine elsewhere in my app.

thanks!
PMH4514Asked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

itsmeandnobodyelseCommented:
Can you post the header where class CImage gets defined?
0
itsmeandnobodyelseCommented:
Note, in C++ you shouldn't define struct by using a typedef

Simply define

struct STITCHREQUEST
{
};

A 'typedef struct' is only sinful if you need to use the struct both for C and C++. However in C you can't specify a destructor.

Regards, Alex
0
PMH4514Author Commented:
sure:

// Image.h: interface for the CImage class.
//
//////////////////////////////////////////////////////////////////////

#if !defined(AFX_IMAGE_H__8DC3367A_4774_11D4_9F57_00500418792F__INCLUDED_)
#define AFX_IMAGE_H__8DC3367A_4774_11D4_9F57_00500418792F__INCLUDED_

#if _MSC_VER > 1000
#pragma once
#endif // _MSC_VER > 1000

#include <afxdisp.h>
#include "..\\VS2000UI\\FramePacket.h"
#include "DataManager.h"

// MODELS THE FIELDS IN THE [IMAGE] TABLE.
typedef struct DBIMAGE
{
      signed long        id;
      signed long      session_id;
      CString            path;
      signed long      zpos;
      signed long      ypos;
      signed long      xpos;
      CString            comments;
      BOOL            deleted;
      double            brightness;
      double            contrast;
      double            exposure;
      double            gain;
      double            gainred;
      double            gainblue;
      double            gamma;

      DBIMAGE()      // default constructor
      {
            id                  = 0;
            session_id            = 0;
            comments            = _T("");
            deleted                  = FALSE;
            path                  = _T("");
            xpos                  = 0;
            ypos                  = 0;
            zpos                  = 0;
            brightness            = 0.0;
            contrast            = 0.0;
            exposure            = 0.0;
            gain                  = 0.0;
            gainred                  = 0.0;
            gainblue            = 0.0;
            gamma                  = 0.0;
      }      

      ~DBIMAGE()      // default constructor
      {
            path.Empty();
            comments.Empty();
      }

} DBIMAGETYPE;

class CImage
{
public:
      CImage();
      CImage(auto_ref_ptr<CFramePacket> pPacket);
      CImage(HBITMAP a_hbm);

      virtual ~CImage();

      DBIMAGETYPE* Data() { return m_pData;};
      void Path(CString* a_sReturn);      

      HBITMAP GetBitmap(){ return m_hbmImage;};

protected:      

      BOOL Save();      
      BOOL SaveTo(CString a_sPath, BOOL a_bReadOnly);

      DBIMAGETYPE* m_pData;


private:
      void PopulateFromBitmap(HBITMAP a_hbm); // setup dimensions based on bitmap

      HBITMAP m_hbmImage;

friend class CFramePacket;
friend class CDataManager;
friend class CImageCollection;            // so it can create a new image for the collection element

};

#endif // !defined(AFX_IMAGE_H__8DC3367A_4774_11D4_9F57_00500418792F__INCLUDED_)
0
Cloud Class® Course: Certified Penetration Testing

This CPTE Certified Penetration Testing Engineer course covers everything you need to know about becoming a Certified Penetration Testing Engineer. Career Path: Professional roles include Ethical Hackers, Security Consultants, System Administrators, and Chief Security Officers.

PMH4514Author Commented:
>>Simply define
>>struct STITCHREQUEST

but when I don't include typedef, I get a compile error:

error C2512: 'STITCHREQUEST' : no appropriate default constructor available
0
AxterCommented:
Hi PMH4514,
> but I am very certain that ~CImage() is never run.

What makes you so certain of this?
I would verify by putting debug logging code in ~CImage

David Maisonave (Axter)
Cheers!
0
AxterCommented:
PMH4514,
> but when I don't include typedef, I get a compile error:
Please post the code as is when you get this compile error.


David Maisonave (Axter)
0
AxterCommented:
Are you trying to create an array of STITCHREQUEST?

If so, you're going to need a default constructor.

If you don't want this type to have a default constructor, than I recommend you use std::vector<STITCHREQUEST> to create the array.
0
PMH4514Author Commented:
>>What makes you so certain of this?
>>I would verify by putting debug logging code in ~CImage

I put debug logging AND a breakpoint in ~CImage, and it is never called. I can also watch my virtual memory in the task manager, and I can see it go up by 1MB for each new image, and it keeps going up and up, as the bitmap is cleaned up in ~CImage. The memory leak was the sign, I traced it to the ~CImage not being called.

The compile error, well, that's only to the aside that Alex mentioend. Look at the typedef struct STITCHREQUEST definition in my initial post. The compile error was when I removed "typedef" and left everything else the same.
0
itsmeandnobodyelseCommented:
Where do you know that the leak is a CImage object?

Maybe it is only resources that you explicitly need to release?

>>>> call CImage::ReleaseGDIPlus to explicitly release resources used by GDI+.

I found that statement at http://msdn2.microsoft.com/en-us/library/bwea7by5(VS.80).aspx. Maybe 'delete' will not free resources.

Regards, Alex

0
PMH4514Author Commented:
Oh, right, the default constructor. Duh, I knew that too. I think though the typedef vs. no typedef is another topic of discussion?

Anyway -
I'm not creating an array of stitchrequest, I create a STITCHREQUEST* and then use that to launch a thread:

CImage* pImage = new CImage(hbmFrame);
STITCHREQUEST* pRequest = new STITCHREQUEST(pImage, m_pCollection);

DWORD dwStitchThreadId;
HANDLE hStitchThread = CreateThread(NULL, 0, StitchThread, (LPDWORD*)pRequest, 0, &dwStitchThreadId);      


the thought being that the thread would take care of stitching the image into the collection and then be responsible for destroying the CImage when the thread was ready to exit.
0
PMH4514Author Commented:
Alex -

CImage is my own class, not a part of the Microsoft API.

0
PMH4514Author Commented:
I create these CImage instances elsewhere throughout my app numerous times, and their destructors are called properly when I delete them, as expected.

Sorry for the confusion about the nomenclature "CImage" -  again, this is my own class (header a few responses above) that models a table in a database called IMAGE, and happens to carry with it a HBITMAP as well.

It just seems calling delete pImage does not result in the destructor being executed, when the instance is deleted within the destructor of my STITCHREQUEST.. Or perhaps it has something to do with the CImage instance being created in one thread, but the attempt to delete in another??
0
itsmeandnobodyelseCommented:
>>>> CImage is my own class

Maybe you should consider a different class name to avoid misunderstandings or future conflicts.

Did you set a breakpoint to the destructor implementation of CImage?

>>>> but when I don't include typedef, I get a compile error

It seems that by using the typedef struct the compiler ignores the destructor and handles the struct as it would handle a C struct. However, I never experienced that before (and would have contradicted the issue). Nevertheless, if you want to use the struct same as a C++ class you should provide the default constructor (where both member pointers should be initialized to NULL). Most likely the destructor will be called after you've done the changes.

Regards, Alex
 
0
itsmeandnobodyelseCommented:
>>>> CImage instance being created in one thread, but the attempt to delete in another

Are the two threads running in different executables (dlls) ???

Generally, it's bad code if you free objects in different modules than they were created. If the modules were in different threads it is worse. Note, if you pass a pointer from a main thread to a child thread the main thread should delete the pointer after the child thread terminated successfully. If there isn't any hierarchy between the threads, any thread should create (and free) its own objects. If you need to pass objects from one thread to another you should make a copy after passing or use container objects that were managed in the main thread, e. g. in global or static containers like std::vector.

Regards, Alex
0
AxterCommented:
>>It just seems calling delete pImage does not result in the destructor being executed
Are you sure when delete is being called, that the pointer is not already set to a NULL value.

If the pointer is already set to a NULL value, than calling delete results in a nop.
0
AxterCommented:
You can save yourself a lot of time, and make your code safer, if you would use smart pointers instead of using dummy raw pointers.

Consider using the following smart pointer:
http://axter.com/smartptr
0
PMH4514Author Commented:
re: the name change, that is something I will consider. I'll check the MS API from now on when I name my own classes :)

>>Did you set a breakpoint to the destructor implementation of CImage?

yes, breakpoints and debug logging. I am quite sure it never gets executed.

The same breakpoints and debug logging show me that elsewhere in the app, this works:
CImage* pImage = new CImage();
//
delete pImage;  // breakpoints and logging show me the destructor is fully executed.

>>It seems that by using the typedef struct the compiler ignores the
>>destructor and handles the struct as it would handle a C struct.
>>...
>>Most likely the destructor will be called after you've done the changes.

I think you misunderstood, the ~STITCHREQUEST() destructor DOES get executed as is. I can set breakpoints within it, and step right over the line:

delete Image;

and when I do that, the breakpoint I also have set in ~CImage() is never hit (nor is my debug logging done) which tells me that the ~CImage() never ran. Again, ~STITCHREQUEST() is executed.

Anyway, I just went and placed a default constructor in STITCHTREQUEST and the behavior has not changed. The ~STITCHREQUEST() continues to run, but "delete" on the CImage does not execute ~CImage();

I'm going to go try creating a new class CStitchRequest and use that in place of the struct, see if that has any ramifications.

0
AxterCommented:
>>Anyway, I just went and placed a default constructor in STITCHTREQUEST and the behavior has not changed. The ~STITCHREQUEST() continues to run,
>>but "delete" on the CImage does not execute ~CImage();

Did you check to see if the pointer is NULL?

If the pointer is NULL, calling delete will not result in a destructor call.
0
itsmeandnobodyelseCommented:
>>>> The ~STITCHREQUEST() continues to run, but "delete" on the CImage does not execute ~CImage();

You should check the pointer value at delete is equal to the pointer value returned by new. You also should make a 'Rebuild All' cause ignored function calls might be caused by a build error or PCH files (precompiled header files) out-of-date.

Regards, Alex

0
itsmeandnobodyelseCommented:
>>>> I'm going to go try creating a new class CStitchRequest and use that in place of the struct

I personally do not use a capital 'C' as prefix to my classes. Thus,  I am minimizing potential conflicts with MFC or ATL class names.

Regards, Alex
0
PMH4514Author Commented:
David - yes, I checked the pointer, it is not null. I can put it in the watcher at that point and see all the initialized values contained within.

Alex - I have, checked the pointer value at delete is equal to the pointer value at new.


Anyway - I have found a workaround that seems to work. Though I don't like not understanding the reason for it.  I  went ahead and wrote a new class CStitchRequest, a standard class and put it in place of my struct for the same purposes. Code below:

// StitchRequest.h: interface for the CStitchRequest class.
//
//////////////////////////////////////////////////////////////////////

#if !defined(AFX_STITCHREQUEST_H__CBC6BA84_D603_4D20_B5B3_8FF5DB3EBB2A__INCLUDED_)
#define AFX_STITCHREQUEST_H__CBC6BA84_D603_4D20_B5B3_8FF5DB3EBB2A__INCLUDED_

#if _MSC_VER > 1000
#pragma once
#endif // _MSC_VER > 1000

// forward reference
class CImage;
class CImageCollection;


// StitchRequest.h: interface for the CStitchRequest class.
//
//////////////////////////////////////////////////////////////////////

#if !defined(AFX_STITCHREQUEST_H__CBC6BA84_D603_4D20_B5B3_8FF5DB3EBB2A__INCLUDED_)
#define AFX_STITCHREQUEST_H__CBC6BA84_D603_4D20_B5B3_8FF5DB3EBB2A__INCLUDED_

#if _MSC_VER > 1000
#pragma once
#endif // _MSC_VER > 1000

// forward reference
class CImage;
class CImageCollection;


class CStitchRequest  
{
public:
      CStitchRequest()
      {
            m_pImage = NULL;
            m_pImageCollection = NULL;
            m_iIndex = 0;
      };

      CStitchRequest(CImage* a_pImage, CImageCollection* a_pCollection, int a_iIndex)
      {
            m_pImage = a_pImage;
            m_pImageCollection = a_pCollection;
            m_iIndex = a_iIndex;
      };


      virtual ~CStitchRequest()
      {
            delete m_pImage;
            m_pImage = NULL;
            m_pImageCollection = NULL;
      };

      int Index(){ return m_iIndex;};
      CImage*      Image(){ return m_pImage;};
      CImageCollection* Collection(){ return m_pImageCollection;};

private:
      CImage*                  m_pImage;
      CImageCollection*   m_pImageCollection;
      int                       m_iIndex;
};

#endif // !defined(AFX_STITCHREQUEST_H__CBC6BA84_D603_4D20_B5B3_8FF5DB3EBB2A__INCLUDED_)


In this case, when ~CStitchRequest() executes,  calling: delete m_pImage results in the ~CImage() destructor being executed properly.


So, having found my own solution, the points go to whomever can explain to me why. :-)

Thanks guys!
0
PMH4514Author Commented:
(whoops, seems I double pasted a portion of the header above, I'm sure the extent of the code is clear to you both ;-)
0
AxterCommented:
>>I personally do not use a capital 'C' as prefix to my classes. Thus,  I am minimizing potential conflicts with MFC or ATL class names.

I also do the same.  IMHO, the practice of adding C prefix adds very little value to the source code.

In every work site I go to, I always try to convince the local developers to stop this practice.

It also makes it harder for you to find the source code, since VC++ IDE does not create a file with the C prefix.
0
itsmeandnobodyelseCommented:
>>>> So, having found my own solution ...

I am pretty sure that if you would now change back to your old code, it would call the destructor properly. I often experienced that virtual calls were not made cause the projects simply were not up-to-date. Unfortunatedly Visual Studio sometimes didn't handle dependencies very accurate, especially if different dlls or precompiled header files were involved.

>>>> , the points go to whomever can explain to me why. :-)

You could split points if you think you got help by more than one expert. You also could ask for a refund if you think you didn't get proper help.

Regards, Alex
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
PMH4514Author Commented:
Sorry Alex and David, I missed some of your comments earlier:


Alex:
>>Are the two threads running in different executables (dlls) ???
No.


Alex:
>>Generally, it's bad code if you free objects in
>>different modules than they were created.

Agreed. This is a single executable app and the only place I'm doing it is in this case where I wanted to ensure that the CImage wasn't deleted until the stitch thread were done with it. The instance was created only for the stitch thread and if the thread work were simply put in the calling function, the instance would have been deleted when the work was done, so it seamed as long as I documented this, it was a suitable approach.



Alex:
>>Note, if you pass a pointer from a main thread to a child
>>thread the main thread should delete the pointer after the child thread terminated successfully.

In most every case, this is how I've handled things.

Alex:
>>If there isn't any hierarchy between the threads,
>>any thread should create (and free) its own objects.

That makes sense as well. I could rework it perhaps so that rather than creating and pushing a CImage pointer into the worker thread, I could pass the thread the simple data type values it needs to create its own instance so that it could be responsible for both creating and destroying it.


Alex:
>>If you need to pass objects from one thread to another you should make a copy after passing or
>>use container objects that were managed in the main thread, e. g. in global or static containers like std::vector.

Good points. Sounds like most of what I'm doing is correct.

David:
>>You can save yourself a lot of time, and make your code safer,
>>if you would use smart pointers instead of using dummy raw pointers.

In fact I am using that very code elsewhere in my app. It's quite nice.

Alex:
>>I personally do not use a capital 'C' as prefix to my classes.
>>Thus,  I am minimizing potential conflicts with MFC or ATL class names."

good point. I was using "C" for class names long before I started with C++. It's a force of habit more than anything else. I'll keep that in mind in the future.


Alex:
>>I am pretty sure that if you would now change back to your old code,
>>it would call the destructor properly. I often experienced that virtual
>>calls were not made cause the projects simply were not up-to-date.

Nope, still does the same thing.

I'll just split the points between both of you.

Thanks!
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Editors IDEs

From novice to tech pro — start learning today.