Problem with destructor not being executed

Can you post the header where class CImage gets defined?

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

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_)

>>Simply define
>>struct STITCHREQUEST

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

error C2512: 'STITCHREQUEST' : no appropriate default constructor available

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!

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)

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.

>>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.

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

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.

Alex -

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

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??

>>>> 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
 

>>>> 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

>>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.

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.

>>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.

>>>> 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

>>>> 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

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!

(whoops, seems I double pasted a portion of the header above, I'm sure the extent of the code is clear to you both ;-)

>>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.

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!