RE: [boost] Re: [shared_ptr] Interlocked* - possible bug?
John Torjo <john.lists@torjo.com> wrote:
Alexander Terekhov wrote:
John Torjo wrote: [...]
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.
However, the weak_ptr doesn't just use atomic_read. If atomic_read returns 0 that means the use count has dropped to 0 [*] and can never increase again, so it must be the latest version. Otherwise weak_ptr makes a second test that is properly protected. So it can never use an old value. [*] I think this is right but I'm not certain that it can't return a 0 that the processor read during creation of an object that uses enable_shared_from_this. Having said all that, I feel the naming of functions may give a false sense of generality and encapsulation when they actually only work in the way they are being used currently. <snip>
Here's my scenario: (remember - we're talking about: http://www.pdimov.com/cpp/shared_count_x86_exp2.hpp)
This is still unaccessible. why don't you mail the code to Alexander and me (or to the list, in case anyone else is interested)?
class test { ... };
[thread1] shared_ptr<test> p1( new test); // refcount = 1
---------------- thread switch
Switches between threads running on a single processor can't cause problems here, because the use of "volatile" ensures that the compiler won't reoder access to the use count and the processor will only reorder memory access in a way that is invisible to the code it's running. There might conceivably be problems that occur when multiple threads run concurrently on multiple processors, in which case reordering by one processor can affect another. So I'll ignore the "thread switches".
[thread2] shared_ptr<test> p2(p1); // refcount = 2 weak_ptr<test> weak(p2); p2 = shared_ptr<test>(); // refcount = 1
------------------thread switch [thread1] p1 = shared_ptr<test>(); // refcount = 0 - object gets destroyed ---------------- thread switch [thread2]
// here - you need InterlockedCompareExchange // to realize that refcount is 0 (zero) shared_ptr<test> p3 = weak.lock();
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!
Also, see my other post to Peter Dimov.
Seen it. Ben.
Ben Hutchings wrote: [...]
If atomic_read returns 0 that means the use count has dropped to 0 [*] and can never increase again, so it must be the latest version. Otherwise weak_ptr makes a second test that is properly protected. So it can never use an old value.
[*] I think this is right but I'm not certain that it can't return a 0 that the processor read during creation of an object that uses enable_shared_from_this.
What makes you uncertain?
Having said all that, I feel the naming of functions may give a false sense of generality and encapsulation when they actually only work in the way they are being used currently.
You mean that bool expired() { return lock(); } is better? regards, alexander.
Ben Hutchings wrote:
John Torjo <john.lists@torjo.com> wrote:
Alexander Terekhov wrote:
John Torjo wrote: [...]
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.
However, the weak_ptr doesn't just use atomic_read. If atomic_read returns 0 that means the use count has dropped to 0 [*] and can never increase again, so it must be the latest version. Otherwise weak_ptr makes a second test that is properly protected. So it can never use an old value.
[*] I think this is right but I'm not certain that it can't return a 0 that the processor read during creation of an object that uses enable_shared_from_this.
Having said all that, I feel the naming of functions may give a false sense of generality and encapsulation when they actually only work in the way they are being used currently.
I'm not sure which functions do you have in mind here.
<snip>
Here's my scenario: (remember - we're talking about: http://www.pdimov.com/cpp/shared_count_x86_exp2.hpp)
This is still unaccessible. why don't you mail the code to Alexander and me (or to the list, in case anyone else is interested)?
http://66.102.9.104/search?q=cache:1ZQ1uXtFUEIJ:www.pdimov.com/cpp/shared_co...
Ben Hutchings wrote:
John Torjo <john.lists@torjo.com> wrote:
Alexander Terekhov wrote:
John Torjo wrote: [...]
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.
However, the weak_ptr doesn't just use atomic_read. If atomic_read returns 0 that means the use count has dropped to 0 [*] and can never increase again, so it must be the latest version. Otherwise weak_ptr makes a second test that is properly protected. So it can never use an old value.
yes, that is quite right.
This is still unaccessible. why don't you mail the code to Alexander and me (or to the list, in case anyone else is interested)?
done that. See my other post.
class test { ... };
[thread1] shared_ptr<test> p1( new test); // refcount = 1
---------------- thread switch
Switches between threads running on a single processor can't cause problems here, because the use of "volatile" ensures that the compiler won't reoder access to the use count and the processor will only reorder memory access in a way that is invisible to the code it's running. There might conceivably be problems that occur when multiple threads run concurrently on multiple processors, in which
as a side note, my PC is already a dual-processor ;)
case reordering by one processor can affect another. So I'll ignore the "thread switches".
I just used the "thread switch" to make a point ;)
[thread2] shared_ptr<test> p2(p1); // refcount = 2 weak_ptr<test> weak(p2); p2 = shared_ptr<test>(); // refcount = 1
------------------thread switch [thread1] p1 = shared_ptr<test>(); // refcount = 0 - object gets destroyed ---------------- thread switch [thread2]
// here - you need InterlockedCompareExchange // to realize that refcount is 0 (zero) shared_ptr<test> p3 = weak.lock();
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. Best, John -- John Torjo -- john@torjo.com 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)
John Torjo wrote:
Ben Hutchings wrote:
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.
As the comment says, the if( expired() ) test is only an optimization. Remove it if it makes you feel better. ;-) The main idea is that a weak_ptr can never transition by itself from an expired() state back to a !expired() state unless you explicitly assign to it. Because you are in weak_ptr::lock at the moment - a read access - you know that no other threads assign to *this, because this would be a violation of the "basic thread safety" requirements. Therefore, if expired() returns true, it will continue returning true for the foreseeable future, and you can skip the rest of lock().
participants (4)
-
Alexander Terekhov -
Ben Hutchings -
John Torjo -
Peter Dimov