[Insight-developers] Re: LightObject race condition fix
Gaëtan Lehmann
gaetan.lehmann at jouy.inra.fr
Mon Aug 6 17:11:31 EDT 2007
Le 6 août 07 à 22:26, kent williams a écrit :
> 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.
I agree with that, and haven't done it only to minimize the changes.
However, for Julia Smith's problem, the original patch would also
work. In her case, there are 2 pointers which are destroyed at the
same time in 2 different threads. The reference count can be
decremented to 0 before the 2 threads comes to the if
( m_ReferenceCount <= 0), and then, the object may be deleted twice.
With the patch, each thread do the test with its own
tmpReferenceCount, and only one of the two tmpReferenceCount can be 0
(the other one is 1).
>
> More to the point, there's a problem in Julia Smith's code if two
> threads
> are both decrementing m_ReferenceCount < 0.
they are not - see the example above
> 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
>
> _______________________________________________
> 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
More information about the Insight-developers
mailing list