4

Here is a code sample:

class A {
  boost::mutex a_mutex;
  boost::shared_ptr<int> a;

  boost::shared_ptr<int> clone_a(void) {
    boost::lock_guard<boost::mutex> lock(a_mutex);
    return a;
  }
};

The suggestion is that the boost::shared_ptr copy constructor call on A::a will precede the boost::lock_guard destructor call despite of the compiler optimizations. So, is it safe to call A::clone_a() ?

eudoxos
  • 18,545
  • 10
  • 61
  • 110
abyss.7
  • 13,882
  • 11
  • 56
  • 100

3 Answers3

5

If by "safe" you mean you won't get data races on a, then yes. It is exactly as you say.

However, it won't protect further accesses to *a (or *clone_a()), as you probably know. I'm not sure, why is the method called "clone", as it doesn't clone anything.

jpalecek
  • 47,058
  • 7
  • 102
  • 144
2

Yes, this code is safe. If you refer to shread_ptr Thread-Safety you can see that threaded write access to thread-local shared_ptr objects is just fine.

In your code above the access to the member shared_ptr requires the lock, since it could be accessed by multiple threads. The copy to the return temporary is done within the lock, thus you're safe there. That temporary cannot be seen by other threads so at that point you are safe to copy it to other shared_ptr's.

Now perhaps the choice of clone_a as a function name is wrong. You aren't cloning the underlying object, you are simply getting a copy of the shared_ptr. So I am assuming you intend on sharing the same underlying "int".

edA-qa mort-ora-y
  • 30,295
  • 39
  • 137
  • 267
-1

Not if you use the return value. The return value itself is a temporary, whose lifetime extends beyond the end of the function; it will be destructed at the end of the full expression which calls A::clone_a. So if you write something like:

shared_ptr<int> newA = object->clone_a();

, the formal semantics will be for the temporary returned by object->clone_a() to be copied into newA, in the context of the caller (and so unprotected by the mutex). In this particular case, you may get away with it because of RVO, but that won't necessarily be the case, and there are other cases where RVO can't intervene.

If all you're worried about is the copy of the pointer, I'm pretty sure that if you set the right compiler options (-D somthing), boost::shared_ptr will behave atomically. In this case, you don't need the mutex at all.

James Kanze
  • 150,581
  • 18
  • 184
  • 329
  • Something's missing here. The first copy will be done inside the `clone_a` function within the locked segment. This is the copy from the member variable to the return. The returned temporary will then be copied to the new variable. I don't see any problems here (of course I don't know what clone_a is actually supposed to do). – edA-qa mort-ora-y Oct 07 '11 at 07:39
  • @edA-qamort-ora-y The first copy, from the member variable to where ever the compiler puts return values, takes place in the function, and is protected. The second copy, from where ever the compiler puts return values to where the value is supposed to go, takes place at the call site, and is not protected. – James Kanze Oct 07 '11 at 07:44
  • 1
    But that's the question: protected from what? It is completely safe to pass and copy shared_ptr's, even temporaries, each one properly maintains the reference count. That is, your `newA` is guaranteed to point to the same object referenced from within the locked segment. – edA-qa mort-ora-y Oct 07 '11 at 07:50
  • The `A::a` need to be protected from being changed (i.e. underlying pointer changes) in another thread while copy construction. As I understand, since the first copying from `A::a` to return value is protected, futher copyings from the return value are thread-specific. – abyss.7 Oct 07 '11 at 08:07
  • @edA-qamort-ora-y As I pointed out, `boost::shared_ptr` has options to make it fully thread safe, and no locks are needed, even in the `clone_a` function. Otherwise, `shared_ptr` modifies memory when being copied, and this needs to be protected. – James Kanze Oct 07 '11 at 08:24
  • 1
    @edA-qamort-ora-y The `A::clone_a()` is supposed to create a thread-specific shared pointer so it can't be changed by another thread. – abyss.7 Oct 07 '11 at 08:27
  • 1
    @James, abyss is correct, the shared_ptr being copied outside of the lock is local to the calling thread and need not be protected. Thus this code is safe as shown. – edA-qa mort-ora-y Oct 07 '11 at 08:54
  • @edA-qamort-ora-y You don't seem to understand the logic behind `shared_ptr`s. Different pointers **share** what they point to. Including the reference count, which will be updated every time the pointer is copied. – James Kanze Oct 07 '11 at 09:18
  • 1
    I understand, and the underlying reference counting is always thread-safe. The wrapping shared_ptr is not, and thus needs to be protected by a mutex. So once you've copied to a thread local shared_ptr you're safe since no other thread can refer to this wrapper. – edA-qa mort-ora-y Oct 07 '11 at 09:24
  • Refer to: http://www.boost.org/doc/libs/1_47_0/libs/smart_ptr/shared_ptr.htm#ThreadSafety – edA-qa mort-ora-y Oct 07 '11 at 09:30
  • @edA-qamort-ora-y James is also correct that the `boost::shared_ptr` has a "common" reference count variable, but it's ok for me till a reference count is greater than zero - and it will be, since I have at least one (thread-specific) shared pointer variable. – abyss.7 Oct 07 '11 at 09:38
  • @edA-qamort-ora-y The underlying reference counting is thread safe **if** you've compiled it with the correct options for it to be thread safe. As I've said from the beginning: if you're using `boost::shared_ptr`, and compiled it with the thread safe options, then there is no need of a lock anywhere in the sample code. And if the pointer hasn't been compiled as thread safe, the proposed locks aren't sufficient. – James Kanze Oct 07 '11 at 10:15
  • 1
    @James, no, that's incorrect. There is an option to make it non-thread-safe, but in the normal mode it is thread-safe. And I see no option to make it super-safe, the mutex is always required in the above function (presuming the code calling it is multi-threaded). – edA-qa mort-ora-y Oct 07 '11 at 10:45
  • @abyss.7 "_The A::clone_a() is supposed to create a thread-specific shared pointer_" so it should probably be called something like `get_shared` – curiousguy Oct 07 '11 at 11:32
  • @edA-qamort-ora-y An option implies two possibilities. In this case, one is thread safe, and one isn't. And the code in the given function does exactly the same thing as the code at the call site; if a mutex is needed for one, it is needed for both, and if it isn't needed for one, it isn't needed for the other. – James Kanze Oct 07 '11 at 15:53
  • 1
    @JamesKanze, that logic doesn't make sense. One variable is shared between threads, the other is not. It's a very standard race condition on a shared variable. I'm not sure what else I can say, the document I linked to states the same thing but much clearer than I can paraphrase here. – edA-qa mort-ora-y Oct 07 '11 at 20:41
  • @JamesKanze You are missing the point of `shared_ptr`: it should be usable the same way an ordinary pointer is. It does **not** need to have stronger thread safety than a scalar. So you are wrong, the mutex is necessary, the code is correct as written. – curiousguy Oct 08 '11 at 05:54
  • @curiousguy There's nothing in the posted code which would require the mutex. If other member functions will be called from a different thread, and these may modify the member variable `a`, then a mutex may be necessary in `clone_a`, but my understanding of the code is that the object of type `A` is only used by one thread. If `shared_ptr` offers the same guarantees as a raw pointer, and there are no other threads modifying `A::a`, then the mutex is not necessary. – James Kanze Oct 10 '11 at 07:47
  • @edA-qamort-ora-y The only data which is modified in the code we're given is the counter in `shared_ptr`. Either `shared_ptr` protects this data, and no mutex is necessary, or `shared_ptr` doesn't, and the mutex isn't sufficient. The only time the mutex would be necessary is if `A::a` were both modified **and** accessed from another thread. Which is a different problem than the one described. – James Kanze Oct 10 '11 at 07:51