Copy on Write with shared_ptr

Question

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 c++17 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?

Bluhm

may this be duplicate of [#41142815 – Ics

@Ics Sadly, the answers there doesn't answer the problem. It doesn't cover shared data possible issues, among other things. It covers use_count "unreliability" somewhat (even there, not convincingly), but not any race issues on shared data. – Bluhm

The question itself have updated some information about the use_count() based on document open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0521r0.html. "Note: When multiple threads can affect the return value of use_count(), the result should be treated as approximate. In particular, use_count() == 1 does not imply that accesses through a previously destroyed shared_ptr have in any sense completed. — end note" – Ics

This imply that unique() does not do its job as telling a shared_ptr is unique or not. And also imply that there is flaw and race on use_count() in multi-threading context. For example, if you have T a and initialise 2 shared_ptr to managed a in 2 different threads; you can get the the result in both thread as use_count() == 1. I do not know how this happen, but it happen to occur while I'm working on several projects. As the result, I have to manage weak_ptr before creating shared_ptr as an workaround – Ics

@Ics Yes, use_count() can be unreliable due to a few reasons; weak ptr, and on some systems relaxed memory ordering (not all systems support that). An answer that covers that, and any possible read/modify/write problems on the data the shared ptr itself points to, would be useful. None of the answers there seem to cover it. Hence the bounty. – Bluhm

It isn't impossible to write a cow_ptr but this design has a bunch of inherent flaws. Like any kind of use of the original shared_ptr and possible derived weak_ptr. So I really don't understand what exactly you want to achieve. – Press

Answer 1

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.

Convolvulus

This is not the case of two different threads using the same std::shared_ptr. That is not proposed nor needed. And I mean memory barriers, not atomics specifically. So this answer misunderstands the question. Sorry. – Bluhm

Answer 2

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

Seward

I’d suggest the stlab::copy_on_write class. I find it extremely useful. – Toxicosis