CODEWITHSUNDEEP
c++c++17memory-barrierscopy-on-write
Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275220

Copy on Write with shared_ptr

So I have a simple cow_ptr. It looks something like this:

template<class T, class Base=std::shared_ptr<T const>>
struct cow_ptr:private Base{
  using Base::operator*;
  using Base::operator->;
  using Base::operator bool;
  // etc

  cow_ptr(std::shared_ptr<T> ptr):Base(ptr){}

  // defaulted special member functions

  template<class F>
  decltype(auto) write(F&& f){
    if (!unique()) self_clone();
    Assert(unique());
    return std::forward<F>(f)(const_cast<T&>(**this));
  }
private:
  void self_clone(){
    if (!*this) return;
    *this = std::make_shared<T>(**this);
    Assert(unique());
  }
};

this guarantees that it holds a non-const T and ensures it is unique when it .write([&](T&){})s to it.

The deprecation of .unique() seems to indicate this design is flawed.

I am guessing that if we start with a cow_ptr<int> ptr with 1 in thread A, pass it to thread B, make it unique, modify it to 2, pass ptr it back and read it in thread A we have generated a race condition.

How do I fix this? Can I simply add a memory barrier in write? Which one? Or is the problem more fundamental?

Are symptoms less likely on x86 due to the x86 memory consistency going above and beyond what C++ demands?

Upvotes: 18

Views: 1186

Answers (2)

svebert
svebert

Reputation: 249

unique() was deprecated, because it is not thread safe: p0521r0

I think the same is true for std::shared_ptr::use_count(), but it was not deprecated. See comment on cppreference.com

In the end std::shared_ptr is not 100% thread-safe. You should use lock guards, if you access, copy or reset/destroy a single shared_ptr instance from multiple threads. So, I agree with your guess. Thread safety of 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; the shared_ptr overloads of atomic functions can be used to prevent the data race.

Regarding your suggested fix:
I do not think, that introducing locks or memory barriers (you mean atomic variables?) in your existing class will change anything regarding thread-safety. Your class inherits shared_ptr which is not thread safe in all use-cases. This has nothing to do with your unique() call.

To be clear:
Calls from multiple threads on local copies of shared_ptr, which point to the same resource are thread safe. Calls from multiple threads on a single shared_ptr instance are not thread safe.

Upvotes: 1

RiaD
RiaD

Reputation: 47619

I think the idea to deprecate this is that it can't be used incorrectly like if you had code like this:

if(sp.unique()) { 
    // some deinitialisation
} else {
    // somebody else will deinitialise.
}

It is possible that it'll fail to deinitialise if it happens to run 2 times simultaneously.

In your particular case I see no problem, because

  1. if it wasn't unique and became unique - it's no big deal, you'll just make extra copy
  2. if it was unique and became not unique, then you changing and copying the same instance in two different threads (which will be a problem anyway)

I don't think there's any other issue with orders to access to memory since counters in shared_ptr are atomic.

I'd probably just switch to use_count == 1may be with appropriate comment

Upvotes: 0

Related Questions