1

I am currently looking at the most popular smart Ptr implementations such as boost shared and weak pointers aswell as loki Smart and Strong pointer since I want to implement my own and from what I understand Loki Strong pointer looks unsafe to me but I rather think that I understand it wrong so I'd like to discuss whether it's safe or not. The reason why I think it's not safe is that as far as I can tell it does not treat weak Pointers (that is a StrongPtr, where false indicates its weak) with enough care:

for instance the dereferencing functions:

PointerType operator -> ()
{
KP::OnDereference( GetPointer() ); //this only asserts by default as far as i know
//could be invalidated right here
return GetPointer();
}

In a multithreaded environment a weak pointer could be invalidated at any time, so that this function might return an invalidated Ptr.

As far as my understanding goes you would either have to create a strongPtr instance of the ptr you are dereferencing to ensure that it does not get invalidated half way through. I think thats also the reason why boost does not allow you to dereference a weak_ptr without creating a shared_ptr instance first. Lokis StrongPtr Constructor suffers from the same problem I think.

Is this a problem or am I reading the src wrong?

markalex
  • 8,623
  • 2
  • 7
  • 32
moka
  • 4,353
  • 2
  • 37
  • 63
  • It does seem unsafe, especially since without creating a strong pointer the object could be delete'd not only before you returned it but also while the user is using it (ouch). Perhaps a good example that Multithreading is hard! – Matthieu M. May 03 '11 at 09:51
  • Well, what you are pointing out, it isn't **thread** safe. I have no idea whether it ever promised to be thread safe (it doesn't look like it from the snippet you show). (_unrelated: your question title is utterly rhetoric and selfreferential_) – sehe May 03 '11 at 09:55
  • I think you are asking a bit too much of the class. Without locking, there is no way you can guarantee that no evil things happen with something you don't own. No matter what better checks you could supposedly add in operator-> to make the actual dereference perfectly safe, it could still be destroyed after you've dereferenced the pointer and are still in a member function call. That's just how it is. – Damon May 03 '11 at 09:57
  • well your points are certainly true, but on one hand loki strong pointer comes with a multithread aware reference count, which simply does not solve the problems I was pointing out. Actually my question was not really rhetoric since as stated I was rather confused that on the one hand it implies to be threadsafe when using MT aware ref cound while it obviously is not. I though I was missing something :) – moka May 03 '11 at 10:36

2 Answers2

3

Regarding the use of assert, it's a programming error to use operator-> on an empty StrongPtr<> instance; i.e., it is the caller's responsibility to ensure that the StrongPtr<> instance is non-empty before dereferencing it. Why should anything more than an assert be needed? That said, if you deem some other behavior more appropriate than assert, then that's exactly what the policy is for.

This is a fundamental difference between preconditions and postconditions; here's a long but very good thread on the subject: comp.lang.c++.moderated: Exceptions. Read in particular the posts by D. Abrahams, as he explains in detail what I'm stating as understood fact. ;-]

Regarding the thread-safety of StrongPtr<>, I suspect most of Loki predates any serious thread-safety concerns; on the other hand, boost::shared_ptr<> and std::shared_ptr<> are explicitly guaranteed to be thread-safe, so I'm sure their implementations make for a "better" (though much more complicated) basis for study.

ildjarn
  • 62,044
  • 9
  • 127
  • 211
  • I disagree, as you can see, the assert is simply not enough to ensure anything in the situation above. Also checking the reference count before does not change anything. As stated the only thing you can do as far as I can tell in a multithreaded environment is to retain the referencecount either manually or by making a Strong copy before dereferencing a weak ptr. I am pretty certain that you could not safely solve the problem with a checking policy since you would have to return some sort of proxy object that stores a Strong pointer internally to retain the reference count. – moka May 03 '11 at 10:45
  • @user300713 : The point is, the `assert` *isn't there to ensure anything* -- it's there to *remind the programmer* to ensure something (that the `StrongPtr<>` is non-empty). Again, it is a programming error to use `operator->` on an empty `StrongPtr<>` instance, and no mechanism exists that can prevent programming errors. The best thing you can do is use something that simply lets said programmer know that they have a programming error, preferably in a manner that avoids degrading runtime performance -- i.e., `assert`. – ildjarn May 03 '11 at 11:04
  • well okay, I never said assert was there to ensure anything. I am just saying in the case I just outlined it might happen that it does not even let the programer know anything. – moka May 03 '11 at 11:08
  • And where is it a programing error if another thread eliminates the last StrongPtr while I am using it? It is certainly a programing error in a single threaded design but I don't see any responsibility in the multithreaded case since as I outlined the way StrongPtr is designed (maybe I am wrong though), you as the programer using it can't do anything about it breaking. – moka May 03 '11 at 11:14
  • @user300713 : That's true, but we've deviated from the actual point: `StrongPtr<>` is **not** unsafe, as it is the programmer's responsibility to ensure that a `StrongPtr<>` instance is non-empty before dereferencing it. Consequently, dereferencing an *empty* `StrongPtr<>` invokes undefined behavior -- you may get an assert, you may get memory corruption, who knows? That's the nature of UB. If your argument is then that `assert` may be an insufficient mechanism to report programming errors due to possible race conditions in a multi-threaded context, then I agree. ;-] – ildjarn May 03 '11 at 11:17
  • @user300713 : I don't see anything about `operator->` that would lead you to believe that it's possible for another thread to eliminate the last `StrongPtr<>` while you're using it. Whether or not that's possible depends entirely on the implementation of `GetPointer()`. – ildjarn May 03 '11 at 11:23
  • yes you are certainly right about that, but by default that function simply returns the raw ptr. as I stated some sort of proxy object retaining the reference count and thus preventing the pointer from being released will most likely solve that issue. – moka May 03 '11 at 11:31
  • @user300713 : By default, `StrongPtr<>` uses the `TwoRefCounts` ownership policy, which is specifically documented: "*It is not thread safe, and is intended for single-threaded environments.*" If you want multi-threading support, use the `LockableTwoRefCounts` ownership policy instead, which is documented: "*It behaves very similarly to `TwoRefCounts`, except that it provides thread-safety.*" – ildjarn May 03 '11 at 11:42
  • well, but I can't see how it can be threadsafe in the following scenario. The object pointed to by StrongPtr gets deleted as soon as Strong RefCount reaches 0. Now imagine the following case where I am dereferencing a WeakPtr (StronPtr where Strong is set to false). If another Thread releases the last StrongPtr while I am doing so, things will break and I will endup with a dangling Ptr. StrongPtr does not provide any mechanism to prevent that from happening as far as I can see. LockableTwoRefCounts makes sure that the Ref Count is correct across multiple threads but does not handle that case. – moka May 03 '11 at 11:51
  • @user300713 : This is a [documented aspect of `StrongPtr<>`](http://loki-lib.sourceforge.net/html/a00639.html): "*Strong pointer : A pointer that claims ownership of a shared object. **When the last strong copointer dies, the object is destroyed even if there are weak copointers**.*" I don't see how this is specific to thread-safety; if this isn't behavior you like, then use a different smart_ptr. (I suspect this is behavior *most* people dislike; there's a reason Boost has completely overshadowed Loki in the last 6 years...) – ildjarn May 03 '11 at 12:08
  • well okay, then it seems to be expected undefined behaviour :) – moka May 03 '11 at 12:11
  • @user300713 : Like I said, Loki predates any serious thread-safety conscientiousness in the C++ community, so this design probably seemed acceptable at the time it was written and an attempt at thread-safety was tacked on later. I suspect that if Alexandrescu were to do it all over again now, it would probably come out looking much more like `std::shared_ptr<>` with policies. – ildjarn May 03 '11 at 12:14
  • well the StrongPtr class is written by Rich Sposato in 2006 so that argument does not really count :) I agree about the second part though! – moka May 03 '11 at 12:25
0

After reading carefully, I think I saw the rationale.

StrongPtr objects are dual in that they represent both Strong and Weak references.

The mechanism of assert works great on a Strong version. On a Weak version, it is the caller's responsability to ensure that the object referenced will live long enough. This can be achieved either:

  • automatically, if you know that you have a Strong version
  • manually, by creating a Strong instance

The benefit wrt std::shared_ptr is that you can avoid creating a new object when you already know that the item will outlive your use. It's an arguable design decision, but works great for experts (of which Alexandrescu undoubtebly is). It may not have been targetted at regular users (us) for which enforcing that a Strong version be taken would have been much better imho.

One could also argue that it's always easier to criticize with the benefit of hindsight. Loki, for all its greatness, is old.

Matthieu M.
  • 287,565
  • 48
  • 449
  • 722
  • well the problem is, that with the design of StrongPtr (which by the way is written by Rich Sposato in 2006) is that you can't even safely create a strong version from a weak one. – moka May 03 '11 at 12:27