Re: [boost] Re: Re: [shared_ptr] Interlocked* - possible bug?

From: John Torjo (john.lists_at_[hidden])
Date: 2004-09-25 05:23:40

Peter Dimov wrote:

> John Torjo wrote:
> 
>>Peter Dimov wrote:
>>
>>
>>>John Torjo wrote:
>>>
>>>
>>>>Hi Peter,
>>>>
>>>>I think there is a bug in your Interlocked* implementation of
>>>>shared_ptr (http://www.pdimov.com/cpp/shared_count_x86_exp2.hpp).
>>>>
>>>>In atomic_read you have:
>>>>inline long atomic_read(long volatile const  & value)
>>>>{
>>>>   return value;
>>>>}
>>>>
>>>>I don't believe this is thread-safe.
>>>
>>>
>>>(repost)
>>>
>>>On x86/IA32, I believe that it's as thread safe as you can get. Note
>>>that all count updates go through Interlocked* calls. On IA32, plain
>>>reads can only return a somewhat "surprising" value when interleaved
>>>with plain writes, and even then, the element of surprise is
>>>limited. ;-)
>>
>>I kind of wonder.
>>
>>Here's my test scenario, which I think is quite effective ;)
>>
>>The main thread creates a window. Then it internally has a shared
>>pointer to it.
>>Other threads might want access to this window. Each other thread
>>keeps a weak_pointer to it.
>>
>>Whenever a thread (other than main) wants to access this window, it
>>will query the weak_pointer. The weak_pointer needs to know the LATEST
>>reference count in order to know if the weak pointer is still valid.
>>Thus, atomic_read that simply returns the value is not enough.
> 
> 
> atomic_read is only called by use_count. You shouldn't be using use_count to 
> check whether a weak_ptr is still valid, because this can never be thread 
> safe, regardless of the implementation of atomic_read. Use weak_ptr::lock.
> 
> 

Lets take a look at weak_ptr::lock():

     wnd_shared_ptr<T> lock() const // never throws
     {
#if defined(BOOST_HAS_THREADS)

         // optimization: avoid throw overhead
         if(expired())
         {
             return wnd_shared_ptr<element_type>();
         }

         try
         {
             return wnd_shared_ptr<element_type>(*this);
         }
         catch(bad_wnd_weak_ptr const &)
         {
             // Q: how can we get here?
             // A: another thread may have invalidated r after the 
use_count test above.
             return wnd_shared_ptr<element_type>();
         }

#else

         // optimization: avoid try/catch overhead when single threaded
         return expired()? wnd_shared_ptr<element_type>(): 
wnd_shared_ptr<element_type>(*this);

#endif
     }

And lets look at expired()

     bool expired() const // never throws
     {
         return pn.use_count() == 0;
     }

Now, see what I mean?

Best,
John

-- 
John Torjo
-- john_at_[hidden]
Contributing editor, C/C++ Users Journal
-- "Win32 GUI Generics" -- generics & GUI do mix, after all
-- http://www.torjo.com/win32gui/
-- v1.4 - save_dlg - true binding of your data to UI controls!
    + easily add validation rules (win32gui/examples/smart_dlg)