7
std::shared_ptr<int> int_ptr;

int main() {
    int_ptr = std::make_shared<int>(1);
    std::thread th{[&]() {
        std::weak_ptr int_ptr_weak = int_ptr;
        auto int_ptr_local = int_ptr_weak.lock();
        if (int_ptr_local) {
            cout << "Value in the shared_ptr is " << *int_ptr_local << endl;
        }
    });

    int_ptr.reset(nullptr);
    th.join();
    return 0;
}

Is the code above thread safe? I read this answer About thread-safety of weak_ptr but just wanted to make sure that the above code is thread safe.

The reason I ask this is that if the code above is indeed thread safe, I cannot understand how the std::weak_ptr and std::shared_ptr interfaces make the following operation atomic expired() ? shared_ptr<T>() : shared_ptr<T>(*this). It just seems to me that making two logical lines of code like above cannot be made synchronous without using some sort of mutex or spinlock.

I understand how atomic increments work with different instances of shared pointers and I understand that shared_ptrs themselves are not thread safe, but if the above is indeed thread safe it is very much like a thread safe shared_ptr and I don't understand how two lines of code like in the conditional above can be made atomic without locks.

Community
  • 1
  • 1
Curious
  • 20,870
  • 8
  • 61
  • 146
  • *"it is very much like a thread safe `shared_ptr`"* - Is using a shared-`shared_ptr` to construct a `weak_ptr` really thread-safe? – Holt Jan 06 '17 at 07:54
  • There's nothing stopping an implementation from using a mutex to make `lock` atomic, except that it would not be very efficient. – Oktalist Jan 06 '17 at 14:47

4 Answers4

5

Is the code above thread safe?

I believe it's not, as int_ptr.reset(nullptr); is racing against std::weak_ptr int_ptr_weak = int_ptr;

I cannot understand how the std::weak_ptr and std::shared_ptr interfaces make the following operation atomic expired() ? shared_ptr<T>() : shared_ptr<T>(*this)

Such an operation is not atomic, as expired() may return false, yet by the time you act upon that value, it may no longer be accurate. On the other hand if it returns true, that's guaranteed to remain accurate, as long as no one modified this particular instance of shared_ptr since then. That is, operations on other copies of a given shared_ptr can't cause it to unexpire.

The weak_ptr::lock() implementation is not going to be using expired(). It will probably do something like atomic compare-exchange, where an extra strong reference is added only if the current number of strong references is greater than zero.

Joseph Artsimovich
  • 1,499
  • 10
  • 13
  • I asked because cppreference says that line of code is atomic. http://en.cppreference.com/w/cpp/memory/weak_ptr/lock Did I interpret that wrong? – Curious Jan 06 '17 at 10:14
  • 1
    The wording on cppreference.com is as follows: _Effectively returns expired() ? shared_ptr() : shared_ptr(*this), executed atomically_. That should be interpreted as "does something to that effect, but atomically". – Joseph Artsimovich Jan 06 '17 at 10:21
  • 3
    Your usage of `weak_ptr::lock()` is indeed thread safe, however there is an unrelated race condition between constructing a weak pointer from a strong one and resetting that strong pointer. – Joseph Artsimovich Jan 06 '17 at 10:31
  • 1
    @Curious: Yes, that's why the last two words are "executed atomically". The code snippet only describes the outcome, not the actual process to get there. – MSalters Jan 06 '17 at 10:32
  • @Tulon why then does it say that the lock() operation is thread safe? what is it safe with respect to? – Curious Jan 06 '17 at 10:33
  • 2
    @Curious: The `weak_ptr::lock()` operation is safe with respect to concurrent operations on other instances of `shared_ptr` and `weak_ptr` that share the state with the `weak_ptr` you are calling `lock()` on. Such operations also include the destruction of another instance. – Joseph Artsimovich Jan 06 '17 at 10:37
  • @Curious: BTW, to make your code really thread safe you need to initialize your `weak_ptr` in the main thread and then capture it into the thread by value rather than by reference. – Joseph Artsimovich Jan 06 '17 at 10:48
  • @Tulon could you explain what the answer was trying to convey in the post I linked to? – Curious Jan 06 '17 at 10:55
  • @Curious: Do you mean [this answer](http://stackoverflow.com/a/30085310/7359606)? It just re-iterates that concurrent access to different instances of `shared_ptr` or `weak_ptr` that share the same state is fine, while concurrent access to the same instance (unless it's read-only access) is not. – Joseph Artsimovich Jan 06 '17 at 11:08
  • But it says "You can use weak_ptr::lock() to get a shared_ptr from other threads without further synchronization." Doesnt that mean that you can get a `weak_ptr` from `shared_ptr`s contained in other threads without any locks? – Curious Jan 06 '17 at 11:39
  • @Curious: `weak_ptr::lock()` is a const method, so that counts as a read-only access. You can call const methods on the same `weak_ptr` instance concurrently from multiple threads as long as no other thread calls non-const methods on that instance. Still, it's a good practice to keep a private copy of `weak_ptr` that a thread will be accessing exclusively. Note that const methods being thread-safe doesn't apply to just any code, but it does apply to the code in the standard library. – Joseph Artsimovich Jan 06 '17 at 12:17
  • @Curious A thread doesn't "contain" objects. Do you mean automatic variable of functions running in other threads? – curiousguy Aug 05 '18 at 07:45
4

This question has two parts:

Thread-safety

The code is NOT threadsafe, but this has nothing to do with lock():
The race exists between int_ptr.reset(); and std::weak_ptr int_ptr_weak = int_ptr;. Because one thread is modifying the non-atomic variable int_ptr while the other reads it, which is - by definition - a data race.

So this would be OK:

int main() {
    auto int_ptr = std::make_shared<int>(1);
    std::weak_ptr<int> int_ptr_weak = int_ptr;  //create the weak pointer in the original thread
    std::thread th( [&]() {
        auto int_ptr_local = int_ptr_weak.lock();
        if (int_ptr_local) {
            std::cout << "Value in the shared_ptr is " << *int_ptr_local << std::endl;
        }
    });

    int_ptr.reset();
    th.join();
}

Atomic version of the example code expired() ? shared_ptr<T>() : shared_ptr<T>(*this)

Of course the whole process can't be atomic. The actually important part is that the strong ref count is only incremented if it is already greater than zero and that the check and the increment happen in an atomic fashion. I don't know if there are any system/architecture specific primitives available for this, but one way to implement it in c++11 would be: 

std::shared_ptr<T> lock() {
    if (!isInitialized) {
        return std::shared_ptr<T>();
    }
    std::atomic<int>& strong_ref_cnt = get_strong_ref_cnt_var_from_control_block();
    int old_cnt = strong_ref_cnt.load();
    while (old_cnt && !strong_ref_cnt.compare_exchange_weak(old_cnt, old_cnt + 1)) {
        ;
    }
    if (old_cnt > 0) {
        // create shared_ptr without touching the control block any further
    } else {
        // create empty shared_ptr
    }
}
MikeMB
  • 20,029
  • 9
  • 57
  • 102
  • Thanks for the answer! The only part that I was confused about here was that the way to do the `lock()` involves a lock (in your example a spin lock) I was under the assumption that the standard said that the `lock()` function would be done in a lock free manner. Is that not true? – Curious Jan 07 '17 at 14:55
  • @Curious: Where did you get the impression that the operation must be lock-free? The standard only says that it has to happen atomically, which is not the same as lock-free. Actually, the operations on `std::atomic_flag` are the only operations that the standard requires to be lock-free. Also I'm not sure, if I'd call this a spin-lock. as the code is not acquiring, releasing or waiting for anything here – MikeMB Jan 08 '17 at 01:19
  • @Curious "_in your example a spin lock_" There is no spin lock. – curiousguy Aug 05 '18 at 07:54
3

No, your code is not thread-safe. There is a data race between the int_ptr.reset() operation in the main thread (which is a write operation) and the initialization of int_weak_ptr from int_ptr in th (which is a read operation).

Wil Evers
  • 67
  • 1
1

" how the std::weak_ptr and std::shared_ptr interfaces make the following operation atomic expired() ? shared_ptr<T>() : shared_ptr<T>(*this)"

The interfaces don't. It's internal to the implementation. Exactly how it's done will differ between implementations.

MSalters
  • 173,980
  • 10
  • 155
  • 350
  • I just didnt understand how that line of code can be atomic without locks. Could you give an example of any one such implementation? – Curious Jan 06 '17 at 10:14
  • 1
    @Curious: That line of code _isn't_ atomic. Implementations will have other code. Since it's a template, you can just find an example in your implementation. – MSalters Jan 06 '17 at 10:28