[Insight-developers] Re: LightObject race condition fix

[kent williams]

Simon's hypothetical also "should" never happen -- if thread A can decrement
the reference count to zero, then there should be no existing references to
the object for B to acquire and call Register() with.  If there is an
existing reference for B to acquire, then A should leave the object with a
reference count of at least 1 when it unlocks, and A won't call 'delete
this'.  

The operative phrase is '... on a LightObject L shared across threads': Each
thread should count as a reference, and call Register() for themselves.  If
they 'share' an object without calling Register() for themselves, then they
should not both call UnRegister(), because it breaks the Register/Unregister
symmetry.

I think Gaetan's patch only accidentally solves Julia Smith's race
condition.  Perhaps this would do a better job:

diff -u -r1.34 itkLightObject.cxx
--- itkLightObject.cxx  4 Apr 2007 20:04:10 -0000       1.34
+++ itkLightObject.cxx  6 Aug 2007 12:07:57 -0000
@@ -150,12 +150,12 @@
::UnRegister() const
{
    m_ReferenceCountLock.Lock();
-  m_ReferenceCount--;
+  int tmpReferenceCount = --m_ReferenceCount;
   m_ReferenceCountLock.Unlock();

   // ReferenceCount in now unlocked.  We may have a race condition
   // to delete the object.
-  if ( m_ReferenceCount <= 0)
+  if ( tmpReferenceCount == 0)
      {
      delete this;
      }

In this case only one thread can get a tmpReferenceCount exactly equal to
zero.

More to the point, there's a problem in Julia Smith's code if two threads
are both decrementing m_ReferenceCount < 0.  Either each thread should call
x->Register() for all shared objects 'x' and x->Unregister() when they're
done with it, or they shouldn't be calling x->Unregister() at all because
they don't own the reference.

If the problem Julia is seeing is inherent somehow to Java's symantics, then
the ITK wrapping for Java is wrong for this problem to arise.

On 8/6/07 1:38 PM, "Gaëtan Lehmann" <gaetan.lehmann at jouy.inra.fr> wrote:

> 
> Le 6 août 07 à 18:29, Simon Warfield a écrit :
> 
>> 
>> So imagine the following sequence of events:
>> Object A in thread 1 calls UnRegister() on a LightObject L shared
>> across threads.
>> 
>> L obtains a lock, and decrements the temporary reference count to
>> be equal to zero, and releases the lock.
>> 
>> Object B in thread 2 , unaware that Object A has asked L to destroy
>> itself, calls Register().
>> It obtains a lock on the reference count variable, and
>> m_ReferenceCount variable goes back up to 1, and then the lock is
>> released.
>> 
>> Light object L then commits suicide by calling delete this, since
>> its temporary reference count variable is 0.
>> 
>> Object B is not expecting this to happen, since it just incremented
>> the reference count.
>> Object B dereferences some pointer  inside L (or a class derived
>> from L) was managing for it, gets random memory contents, and
>> crashes with a segmentation violation.
> 
> Yes, that's a problem, but it is also there currently.
> This patch does not try to fix that - only the problem reported by
> Julia Smith.
> 
> Gaëtan
> 
> 
>>> The changes are:
>>> 
>>> [glehmann at marvin Common]$ cvs diff -u
>>> Index: itkLightObject.cxx
>>> ===================================================================
>>> RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.cxx,v
>>> retrieving revision 1.34
>>> diff -u -r1.34 itkLightObject.cxx
>>> --- itkLightObject.cxx  4 Apr 2007 20:04:10 -0000       1.34
>>> +++ itkLightObject.cxx  6 Aug 2007 12:07:57 -0000
>>> @@ -150,12 +150,12 @@
>>> ::UnRegister() const
>>> {
>>>     m_ReferenceCountLock.Lock();
>>> -  m_ReferenceCount--;
>>> +  int tmpReferenceCount = --m_ReferenceCount;
>>>     m_ReferenceCountLock.Unlock();
>>> 
>>>     // ReferenceCount in now unlocked.  We may have a race condition
>>>     // to delete the object.
>>> -  if ( m_ReferenceCount <= 0)
>>> +  if ( tmpReferenceCount <= 0)
>>>       {
>>>       delete this;
>>>       }
>>> Index: itkLightObject.h
>>> ===================================================================
>>> RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.h,v
>>> retrieving revision 1.33
>>> diff -u -r1.33 itkLightObject.h
>>> --- itkLightObject.h    6 Feb 2006 22:01:56 -0000       1.33
>>> +++ itkLightObject.h    6 Aug 2007 12:07:57 -0000
>>> @@ -114,7 +114,7 @@
>>>     virtual void PrintTrailer(std::ostream& os, Indent indent) const;
>>> 
>>>     /** Number of uses of this object by other objects. */
>>> -  mutable int m_ReferenceCount;
>>> +  volatile mutable int m_ReferenceCount;
>>>     /** Mutex lock to protect modification to the reference count */
>>>     mutable SimpleFastMutexLock m_ReferenceCountLock;
>>> 
>>> Gaëtan
>>> 
>>> 
>> 
>> -- 
>> Simon
>> _______________________________________________
>> Insight-developers mailing list
>> Insight-developers at itk.org
>> http://www.itk.org/mailman/listinfo/insight-developers
> 
> --
> Gaëtan Lehmann
> Biologie du Développement et de la Reproduction
> INRA de Jouy-en-Josas (France)
> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
> http://voxel.jouy.inra.fr
> 
> 
> 
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers