18

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?

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • may this be duplicate of [https://stackoverflow.com/questions/41142315/why-is-stdshared-ptrunique-deprecated] – Silver May 11 '22 at 10:22
  • @Silver 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. – Yakk - Adam Nevraumont May 11 '22 at 13:59
  • 1
    The question itself have updated some information about the `use_count()` based on document http://www.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" – Silver May 12 '22 at 03:42
  • 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 – Silver May 12 '22 at 03:55
  • @Silver 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. – Yakk - Adam Nevraumont May 13 '22 at 00:48
  • 1
    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. – ALX23z May 15 '22 at 09:59

2 Answers2

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.

svebert
  • 220
  • 2
  • 12
  • 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. – Yakk - Adam Nevraumont Jul 21 '22 at 16:33
0

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

RiaD
  • 46,822
  • 11
  • 79
  • 123