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

From: John Torjo (john.lists_at_[hidden])
Date: 2004-09-29 01:47:51

Ben Hutchings wrote:
> John Torjo <john.lists_at_[hidden]> wrote:
> 
>>Ben Hutchings wrote:
> 
> <snip>
> 
>>>expired() may read the count as still being 1 and so return false,
>>>but I believe the wnd_shared_ptr constructor will catch the fact
>>>that the pointer really has expired.  Unfortunately I can't yet
>>>see the code to confirm this!
>>
>>By looking at the code, I would think not. But I may be wrong.
>>Anyway,  I've posted the code in my other post.
> 
> 
> Thanks for that.  Supposing that expired() wrongly returns false
> because use_count is really 0 but it gets an old non-zero value.
> Then lock() attempts to construct a shared_ptr:
> 
> calls shared_ptr<T>::shared_ptr<T>(weak_ptr<T> const &)
>       calls detail::shared_count::shared_count(weak_count const &)
>             calls detail::sp_counted_base::add_ref_lock()
>                   calls detail::atomic_conditional_increment(
>                             long volatile &)
>                         either reads the count as 0
>                         or calls _InterlockedCompareExchange
>                                  fails and returns 0
>                            then loops around and reads the count as 0
>                         returns 0
>                   throws bad_weak_ptr
> catches bad_weak_ptr
> returns 0
> 

Yes indeed.
It was my mistake. I failed to see the flow when shared_ptr gets 
constructed from weak_ptr.

So, I was wrong from the beginning. Sorry.

But now, by looking again at atomic_conditional_increment. I'm not a 
threading expert, but it seems pretty costly as it is implemented now. 
Could it not go multiple times (n>=2) thorughout the for?

Current implementation:

inline long atomic_conditional_increment(long volatile & value)
{
     for(;;)
     {
         long tmp = value;
         if(tmp == 0) return 0;
         if(InterlockedCompareExchange(&value, tmp + 1, tmp) == tmp) 
return tmp + 1;
     }
}

Couldn't it become:

inline long atomic_conditional_increment(long volatile & value)
{
     long tmp = value;
     if(tmp == 0) return 0;
     long new_val = InterlockedCompareExchange(&value, tmp + 1, tmp);
     if(new_val == tmp) return tmp + 1;
     else return InterlockedIncrement(&value);
}

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)