[Insight-developers] LightObject race condition fix

Wed Jul 11 09:33:29 EDT 2007

Thanks Gaetan:

On 7/10/07, Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr> wrote:
>
>
> Le 10 juil. 07 à 21:59, Luis Ibanez a écrit :
>
> >
> > Hi Gaetan,
> >
> > Wasnt' the right solution to move the code:
> >
> >   if ( m_ReferenceCount <= 0)
> >    {
> >    delete this;
> >    }
> >
> > inside the mutex locked section ?
> >
>
> Can the mutex be unlocked once destroyed by the delete call ?

I agree with you here.  "delete this" invalidates non-static member vars. So
I doubt if the following code snippet will work.

UnRegister()
{
  m_ReferenceCountLock.Lock();
  m_ReferenceCount--;
  if ( m_ReferenceCount <= 0) { delete this;  }
  m_ReferenceCountLock.Unlock();
}

However I agree with Steve/Luis that access of the resource m_ReferenceCount
be thread safe. One solution would be to use a scoped lock (unlocking is
implemented in its destructor, so it automatically does that when it goes
out of scope).

The code would look like

UnRegister()
{
  ScopedLock lock( m_ReferenceCountLock )
  m_ReferenceCount--;
  if ( m_ReferenceCount <= 0) { delete this;  }
}

--------------------------------

Would a scoped lock class that constructs itself from an
itk::SimpleFastMutexLock be useful ?

Boost uses such a scoped lock in its implementation of shared_ptr.

http://boost.cvs.sourceforge.net/boost/boost/boost/detail/shared_ptr_nmt.hpp?revision=1.5&view=markup
http://boost.cvs.sourceforge.net/boost/boost/boost/detail/atomic_count_pthreads.hpp?revision=1.3&view=markup

Thanks
--k

What
> happens in that case ?
> In the current code (as well as with the proposed patch), delete is
> the last thing ran, which seem to be a quite safe behavior.
>
> Here is the description by Julia of the race condition: "Two threads
> could be un-registering at the same time. The decrement is all fine,
> but as soon as you release the lock, one thread is going to win the
> delete race, and sometime later the second thread is going to examine
> the memory and will attempt delete again because the reference count
> is still zero or < 0. Saving the value before releasing the lock will
> guarantee the value of the reference count."
>
> About the Steve's example, a light object should never get a Register
> () call when m_ReferenceCount is 0, because at that point, the object
> is, or will be, deleted, so it is likely to show a bug in the
> program. The behavior with the patch in that example does not seem
> worth than without the patch - and it doesn't seem better though. In
> that case, I think it's up to the user to keep a smart pointer in a
> safe place so it can be safely referenced in a thread.
>
> The changes made are as small as possible to avoid breaking anything,
> but there are surely some others changes to be made.
>
>
> >
> > --
> >
> > About "volatile" its purpose is to let the compiler
> > know that the variable may change its value for reasons
> > that can not be deduced from the current code context.
> >
> > Its purpose is to prevent the compiler from making
> > optimization decision based on the assumption that
> > the variable is not directly modified by assignment
> > code in the current code context.
> >
> >
> >
> >    Luis
> >
> >
> > ---------------------------
> > Steve M. Robbins wrote:
> >> Hi,
> >> I'm not sure what race conditions Julia Smith has uncovered, but the
> >> first change looks like it creates a worse race.
> >> On Tue, Jul 10, 2007 at 08:51:21PM +0200, Gaëtan Lehmann wrote:
> >>> Index: Code/Common/itkLightObject.cxx
> >>> ===================================================================
> >>> RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.cxx,v
> >>> retrieving revision 1.34
> >>> diff -u -r1.34 itkLightObject.cxx
> >>> --- Code/Common/itkLightObject.cxx      4 Apr 2007 20:04:10
> >>> -0000       1.34
> >>> +++ Code/Common/itkLightObject.cxx      10 Jul 2007 18:41:08 -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;
> >>>     }
> >> Imagine that thread 1 has a reference to a LightObject L with
> >> m_ReferenceCount = 1 and calls L->Unregister().  The test for zero
> >> references happens outside the critical section, so imagine what
> >> happens when thread 2 calls L->Register() while thread 1 is at line
> >> [***].  Thread 2 thinks it has obtained a reference and is going
> >> to be
> >> very unhappy after thread 1 deletes L.
> >> I'm not clear on the semantics of "volatile", so it's not clear to me
> >> whether the other change (marking m_ReferenceCount as volatile) is
> >> sufficient to cure this race in the original code.
> >> If volatile isn't sufficient, why not just "delete this" while
> >> holding
> >> the lock?  You'd also need to worry about threads blocking in
> >> Register()
> >> while the object is deleted.  Presumably SimpleFastMutexLock.Lock()
> >> would have to throw if the lock was destructed.  Or something.
> >> My head hurts.
> >> -Steve
> >> ---------------------------------------------------------------------
> >> ---
> >> _______________________________________________
> >> 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
>
>
>

-- 
Karthik Krishnan
R&D Engineer,
Kitware Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.itk.org/mailman/private/insight-developers/attachments/20070711/3627e6e9/attachment.htm