2

From the reference I know that std::shared_ptr<T> itself is thread-safe, because the reference counting is typically implemented by some std::atomic with std::memory_order_relaxed.

However, I still don't know how std::shared_ptr ensures thread-safe under concurrent increment and decrement of the reference counter.

I.e.,

Thread 1:
// destructing shared_ptr<T>
// rc.fetch_sub(1)
// Thread 1 got RC == 0 and *thought* that he is the last owner
// ** call T::~T()

Thread 2:
// concurrently copy that shared_ptr<T>
// rc.fetch_add(1)
// Thread 2 got RC == 1 and *thought* that the copied shared_ptr is valid
// dereference the shared_ptr
// ** accessing the destructed T !

This case, although race, is not impossible. Here is an example code (manually construct an extreme case).

std::shared_ptr<T> ptr;
int main()
{
    std::thread t([&ptr] () 
    {
        ptr = std::make_shared<int>();
    } // reference counting decrease here! Call ~T()
    );

    auto ptr2 = ptr; // reference counting increase here! 
    ptr2->some_function(); // access destructed object!

    t.join();
}

My question is:

  • How does C++ reference say about this case?
  • Does std::shared_ptr<T> ensure thread-safe even under concurrent increment and decrement of the reference counter?
Evg
  • 25,259
  • 5
  • 41
  • 83
Bin Yan
  • 117
  • 8
  • I think the resolution is that for this to occur "Thread 2" would need to be copying an existing `shared_ptr`. But the state of "Thread 1" implies that the it is being destroyed (so nobody can be copying it) and is the last instance (due to the counter being seen as 1). There has to be at least one more `shared_ptr` meaning the counter couldn't be 0 in "Thread 1". – François Andrieux Nov 08 '22 at 16:26
  • 1
    Your example contains a race condition on `ptr`. The standard doesn't guarantee that `std::shared_ptr` is atomic, only that the reference counter is. – François Andrieux Nov 08 '22 at 16:28
  • 2
    Please share a [MCVE]. The code shown doesn't compile. What is `T`? Your `shared_ptr` is being assigned a `shared_ptr`. – François Andrieux Nov 08 '22 at 16:29
  • Thanks for your reply! I think the extreme case exists, where Thread 2 is copying the same `shared_ptr` which Thread 1 is concurrently destroying. This situation is possible, because both Thread 1 and Thread 2 can own a reference to the shared_ptr, i.e., `std::shared_ptr&`. – Bin Yan Nov 08 '22 at 16:29
  • 5
    The situation you describe is both a race condition and an object lifetime management error. What you describe is UB even if the type is atomic. You cannot destroy an object and also access it from another object. Edit : You may be confusing destroying a `shared_ptr` with destroying the pointed object, which are different concerns. – François Andrieux Nov 08 '22 at 16:31
  • 1
    Uh. How is `reference counting decrease here! Call ~T()` a place where the reference count decreases? `ptr` is received in the lambda by reference, and assigned; the reference count becomes `1` (thanks to `make_shared` creating it) and is not decremented ever (well, not until globals are destroyed when the program ends). – ShadowRanger Nov 08 '22 at 16:43
  • Just to reiterate what others have already said: your race condition depends on erroneous lifetime management. You must not use an object in another thread before it's constructor finishes or after it's destructor has been invoked. If you follow this, the race condition cannot occur. – Wutz Nov 08 '22 at 16:50
  • 1
    Don't use the same `std::shared_ptr` in more than 1 thread. Access to `ptr` is not thread safe. You should always use a unique copy of a `std::shared_ptr` in each thread. The problem starts here `std::thread t([&ptr] ()...` by using a reference to `ptr` it should instead start the thread with a copy of `ptr`. – Richard Critten Nov 08 '22 at 16:53
  • Change `ptr2->some_function(); // access destructed object!` to `ptr2->some_function(); // possibly access not yet constructed object!` – Eljay Nov 08 '22 at 17:10
  • And `// reference counting decrease here! Call ~T()` is an incorrect comment, since `ptr` is *captured by reference*. – Eljay Nov 08 '22 at 17:11
  • Side-note: The logic where you say `Thread 2 got RC == 1` is incorrect. If `fetch_add` returns `1`, then it was already at reference count `1` (`fetch_add` returns the old value, not the new value), will now be at reference count `2`, and the whole thing was valid from the get-go (it never hit `0`, and therefore would never have been destroyed). If `fetch_add` returns `0`, that's an issue (because it implies `ptr` was *already* invalid, so odds are the control block access to even perform the `fetch_add` was undefined behavior), but that's because you shared the same variable in two threads. – ShadowRanger Nov 08 '22 at 18:00
  • The sharing the same variable in two threads issue is where the race lies, and [François Andrieux's comments on WinterMute's answer](https://stackoverflow.com/questions/74364000/c-race-of-incrementing-and-decrementing-reference-counting-of-stdshared-ptr#comment131282128_74364168) explain why that's an issue (it's a problem unrelated to `shared_ptr` itself; `shared_ptr` can only protect against issues when two threads use the same control block through different `shared_ptr` instances). – ShadowRanger Nov 08 '22 at 18:02

1 Answers1

2

shared_ptr does not use fetch_add etc. internally, instead it'll use compare_exchange. From the GNU implementation:

  template<>
    inline bool
    _Sp_counted_base<_S_atomic>::
    _M_add_ref_lock_nothrow() noexcept
    {
      // Perform lock-free add-if-not-zero operation.
      _Atomic_word __count = _M_get_use_count();
      do
        {
          if (__count == 0)
            return false;
          // Replace the current counter value with the old value + 1, as
          // long as it's not changed meanwhile.
        }
      while (!__atomic_compare_exchange_n(&_M_use_count, &__count, __count + 1,
                                          true, __ATOMIC_ACQ_REL,
                                          __ATOMIC_RELAXED));
      return true;
    }

There is a little boilerplate, but in essence what this does is get the current count, check if it's zero, otherwise do an atomic compare_exchange operation under the expectation that the value hasn't changed since the read. This is a fairly common mechanism in lock-free (but not wait-free) data-structures. You could call it a kind of user-space spinlock, if you squinted a little. This is advantageous when compared to mutexes because it saves a very expensive system call unless contention on the shared_ptr is extremely high.

Your code, by the way, should check whether ptr2 is valid before use since the thread could have ended before the copy in the main thread. This seems unrelated to your actual question, though.

Wintermute
  • 42,983
  • 5
  • 77
  • 80
  • `shared_ptr`'s assignment is not atomic. There is still a race condition on `ptr` in the original code. Checking if `ptr2` is valid is insufficient. The scenario described in the question is not possible without already being in UB and indicates that there is an underlying confusion. – François Andrieux Nov 08 '22 at 16:38
  • Are you sure? I thought there were guarantees for `shared_ptr`, just not the object it references. – Wintermute Nov 08 '22 at 16:41
  • 3
    Only the control block is synchronized. That means if you have two threads, each with their own `shared_ptr` to the same object and using the same reference counter, it is safe for each to reassign or reset their own `shared_ptr`. But it doesn't allow two threads that have a reference to the same pointer to avoid a data race if they do the same to that `shared_ptr`. – François Andrieux Nov 08 '22 at 16:42
  • 1
    See https://en.cppreference.com/w/cpp/memory/shared_ptr : All member functions (including copy constructor and copy assignment) can be called by multiple threads on **different instances** of shared_ptr without additional synchronization even if these instances are copies and share ownership of the same object. If multiple threads of execution access the **same instance** of shared_ptr without synchronization and any of those accesses uses a non-const member function of shared_ptr then a data race will occur. – François Andrieux Nov 08 '22 at 16:43
  • Sure, they shouldn't both use the referenced object at the same time unless the object has its own thread-safety guarantees, but they don't in the example. Either the child thread doesn't destroy the object because the father has increased the refctr, or the father thread doesn't get a pointer to it because the refctr is already zero when it tries to take a copy of the shared_ptr. – Wintermute Nov 08 '22 at 16:45
  • 5
    The problem is simply that `ptr = /*something*/;` in one thread and `/*something/* = ptr;` in another thread, without synchronization and using the same object `ptr` is UB. `shared_ptr` doesn't provide an exception to that. – François Andrieux Nov 08 '22 at 16:46
  • Oh, I think I see what you mean, they're both using `ptr.` That could be unsafe, yes. I admit my language lawyering isn't instantly up to par there. – Wintermute Nov 08 '22 at 16:46
  • After looking into it for a few minutes, I believe you are correct, particularly because the child thread modifies `ptr.` Also now that I look at it again, if `ptr` is a global object, then it is not destroyed when the child thread ends and the refctr doesn't become zero. So I agree the code in the question is a little confused. – Wintermute Nov 08 '22 at 17:02