[Insight-developers] Re: LightObject race condition fix

[Steve M. Robbins]

On Tue, Aug 14, 2007 at 07:27:55AM +1000, Dan Mueller wrote:

> From my understanding, because m_PointerToNative is a _native_ pointer
> and not a _smart_ pointer, when it goes out of scope it is deleted.

No, you've got them reversed.  When a native pointer goes out of
scope, *nothing* happens.  The smart pointer, by contrast, adds a
reference to the pointee at construction time, and removes that
reference at destruction time.


> Here is a small program to demonstrate what I mean:
> #define NativePointer 1
> 
> #include "itkImage.h"
> 
> const unsigned int Dimension = 3;
> typedef unsigned char PixelType;
> typedef itk::Image< PixelType, Dimension > ImageType;
> 
> #if NativePointer
> ImageType* GetImage(void)
> {
>     ImageType* image = (ImageType*)ImageType::New();
>     return image;
> }

OK, now I see what smart pointer you were talking about.  Let's
rewrite this to better see what's going on:

ImageType* GetImage(void)
{
    ImageType::Pointer ptr = ImageType::New();
    return ptr.GetPointer();
}

So in the first line, the image is created with ref count = 1, and the
smart pointer returned by New() is assigned to ptr.  At the end of the
function, ptr goes out of scope, decrementing the reference count to
zero and destroying the image.  Thus, the (native) pointer returned is
invalid.

You can fix this by explicitly adding a reference:

ImageType* GetImage(void)
{
    ImageType::Pointer ptr = ImageType::New();
    ptr->Register();
    return ptr.GetPointer();
}

With this version, the image created will have reference count of 1 at
the end of the function, so the returned pointer *is* valid.  The
caller, however, must know to call UnRegister() when done with it.


> Because the wrapper object only keeps a _native_ pointer to the ITK
> object, when a call is made to a wrapper method the reference goes out
> of scope and the object is freed (similar to the example above). To
> combat this, I call Register() in every wrapper method, for example:
> 
> void WrappedSomeMethod()
> {
>     m_PointerToNative->SomeMethod();
>     m_PointerToNative->Register();
> }

You didn't mention how m_PointerToNative was initialized.  

If it is done by something like your GetImage() method, above, the
pointer will be invalid before you even get to use it.  No amount of
extra Register() calls will change things.

If, on the other hand, you stuck in a Register() call -- as in my
version of GetImage() -- then m_PointerToNative is correctly
initialized with reference count of 1.  If so, WrappedSomeMethod()
should *not* be calling Register(), but simply
m_PointerToNative->SomeMethod() and the destructor should do a single
m_PointerToNative->UnRegister().


In summary: the wrapper class should be written as follows, without
the need for SetReferenceCount().

public ref class itkImage_D2 : itkImageBase
{
private:
    // NativeType
    typedef itk::Image< double,2 > NativeType;
    NativeType* m_PointerToNative; //NOTE: NativeType::Pointer is not allowed!

public:
    itkImage_D2() {
        m_PointerToNative = ImageType::New();
	m_PointerToNative->Register();
    }

    ~itkImage_D2() {
	m_PointerToNative->UnRegister();
    }

    void WrappedSomeMethod()
    {
        m_PointerToNative->SomeMethod();
    }


    //...
}


Cheers,
-Steve
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://www.itk.org/mailman/private/insight-developers/attachments/20070813/f505d27c/attachment.pgp