Multiple shared_ptr storing same pointer

Question

Consider this program:

#include <memory>
#include <iostream>

class X
  : public std::enable_shared_from_this<X>
{
public:
  struct Cleanup1 { void operator()(X*) const; };
  struct Cleanup2 { void operator()(X*) const; };
  std::shared_ptr<X> lock1();
  std::shared_ptr<X> lock2();
};

std::shared_ptr<X> X::lock1()
{
  std::cout << "Resource 1 locked" << std::endl;
  return std::shared_ptr<X>(this, Cleanup1());
}

std::shared_ptr<X> X::lock2()
{
  std::cout << "Resource 2 locked" << std::endl;
  return std::shared_ptr<X>(this, Cleanup2());
}

void X::Cleanup1::operator()(X*) const
{
  std::cout << "Resource 1 unlocked" << std::endl;
}

void X::Cleanup2::operator()(X*) const
{
  std::cout << "Resource 2 unlocked" << std::endl;
}

int main()
{
  std::cout << std::boolalpha;

  X x;
  std::shared_ptr<X> p1 = x.lock1();
  {
    std::shared_ptr<X> p2 = x.lock2();
  }
}

I don't see anything in the C++11 Standard section 20.7.2 suggesting any of this is invalid. It's a bit unusual to have two shared_ptr objects store the same pointer &x but not share ownership, and to use "deleters" that do not end the lifetime of *get(), but nothing forbids it. (And if either of those are entirely unintended, it would be difficult to explain why some shared_ptr member functions accept a std::nullptr_t value.) And as expected, the program outputs:

Resource 1 locked
Resource 2 locked
Resource 2 unlocked
Resource 1 unlocked

But now if I add a bit to main():

int main()
{
  std::cout << std::boolalpha;

  X x;
  std::shared_ptr<X> p1 = x.lock1();
  bool test1( x.shared_from_this() );
  std::cout << "x.shared_from_this() not empty: " << test1 << std::endl;
  {
    std::shared_ptr<X> p2 = x.lock2();
  }
  try {
    bool test2( x.shared_from_this() );
    std::cout << "x.shared_from_this() not empty: " << test2 << std::endl;
  } catch (std::exception& e) {
    std::cout << "caught: " << e.what() << std::endl;
  }
}

then things get trickier. With g++ 4.6.3, I get the output:

Resource 1 locked
x.shared_from_this() not empty: true
Resource 2 locked
Resource 2 unlocked
caught: std::bad_weak_ptr
Resource 1 unlocked

Why would the second call to shared_from_this() fail? All the requirements of 20.7.2.4p7 are met:

Requires: enable_shared_from_this<T> shall be an accessible base class of T. *this shall be a subobject of an object t of type T. There shall be at least one shared_ptr instance p that owns &t.

[T is X, t is x, p is p1.]

But g++'s enable_shared_from_this essentially follows the suggested implementation from the (non-normative) "Note" in 20.7.2.4p10, using a private weak_ptr member in class enable_shared_from_this. And it seems impossible to account for this sort of issue without doing something considerably more complicated in enable_shared_from_this.

Is this a defect in the Standard? (If so, no comment is needed here on what the solution "should" be: add a requirement so the example program invokes Undefined Behavior, change the Note to not suggest such a simple implementation would be sufficient,....)

Answer 1

  X x;
  std::shared_ptr<X> p1 = x.lock1();
  (...sniped...)
}

Such code is breaks the semantics of "owning" "smart pointers":

• they can be copied
• as long as one copy is kept around, the owned object is kept around

This invariant is so essential, I'd argue that such practice should be rejected by code review. But there is a variant of what you suggest that satisfies the invariant:

• the object must be dynamically managed (so, not automatic)
• any family of owning objects has shared ownership of the dynamically managed object
• each member of a family has shared ownership of the "deleter" of that family

So here we have shared owning objects that are part of different "families" of owning objects, they aren't "equivalent" as they have different:

• "deleters" object
• use_count() values
• control blocks
• owner_before results

but they all prevent the destruction of the same object; this is done by keeping a copy of the shared_ptr in each and every "deleter" object.

A clean replacement for std::shared_from_this is used to have complete control over the initialization of the std::weak_ptr<T> member.

#include <memory>
#include <iostream>
#include <cassert>

// essentially like std::shared_from_this
// unlike std::shared_from_this the initialization IS NOT implicit
// calling set_owner forces YOU to THINK about what you are doing!

template <typename T>
class my_shared_from_this
{
    std::weak_ptr<T> weak;
public:
    void set_owner(std::shared_ptr<T>);
    std::shared_ptr<T> shared_from_this() const;
};

// shall be called exactly once
template <typename T>
void my_shared_from_this<T>::set_owner(std::shared_ptr<T> shared)
{
    assert (weak.expired());
    weak = shared;
}

template <typename T>
std::shared_ptr<T> my_shared_from_this<T>::shared_from_this() const
{
    assert (!weak.expired());
    return weak.lock();
}

class X : public my_shared_from_this<X>
{
public:
  struct Cleanup1 { 
    std::shared_ptr<X> own;
    Cleanup1 (std::shared_ptr<X> own) : own(own) {}
    void operator()(X*) const; 
  };

  struct Cleanup2 { 
    std::shared_ptr<X> own;
    Cleanup2 (std::shared_ptr<X> own) : own(own) {}
    void operator()(X*) const; 
  };

  std::shared_ptr<X> lock1();
  std::shared_ptr<X> lock2();

  X();
  ~X();
};

// new shared owner family with shared ownership with the other ones
std::shared_ptr<X> X::lock1()
{
  std::cout << "Resource 1 locked" << std::endl;
  // do NOT call set_owner here!!!
  return std::shared_ptr<X>(this, Cleanup1(shared_from_this()));
}

std::shared_ptr<X> X::lock2()
{
  std::cout << "Resource 2 locked" << std::endl;
  return std::shared_ptr<X>(this, Cleanup2(shared_from_this()));
}

void X::Cleanup1::operator()(X*) const
{
  std::cout << "Resource 1 unlocked" << std::endl;
}

void X::Cleanup2::operator()(X*) const
{
  std::cout << "Resource 2 unlocked" << std::endl;
}

X::X()
{
  std::cout << "X()" << std::endl;
}

X::~X()
{
  std::cout << "~X()" << std::endl;
}

// exposes construction and destruction of global vars
struct GlobDest {
  int id;
  explicit GlobDest(int id);
  ~GlobDest();
};

GlobDest::GlobDest(int id) 
  : id(id) 
{
    std::cout << "construction of glob_dest #" << id << std::endl;
}

GlobDest::~GlobDest() {
    std::cout << "destruction of glob_dest #" << id << std::endl;
}

GlobDest glob_dest0 {0};
std::shared_ptr<X> glob;
GlobDest glob_dest1 {1};

std::shared_ptr<X> make_shared_X()
{
    std::cout << "make_shared_X" << std::endl;
    std::shared_ptr<X> p = std::make_shared<X>();
    p->set_owner(p);
    return p;
}

int test()
{
  std::cout << std::boolalpha;

  std::shared_ptr<X> p = make_shared_X();
  static std::shared_ptr<X> stat;
  {
    std::shared_ptr<X> p1 = p->lock1();
    stat = p1;
    {
      std::shared_ptr<X> p2 = p->lock2();
      glob = p2;
      std::cout << "exit scope of p2" << std::endl;
    }
    std::cout << "exit scope of p1" << std::endl;
  }
  std::cout << "exit scope of p" << std::endl;
}

int main()
{
  test();
  std::cout << "exit main" << std::endl;
}

Output:

construction of glob_dest #0
construction of glob_dest #1
make_shared_X
X()
Resource 1 locked
Resource 2 locked
exit scope of p2
exit scope of p1
exit scope of p
exit main
Resource 1 unlocked
destruction of glob_dest #1
Resource 2 unlocked
~X()
destruction of glob_dest #0

Answer 2

This issue and other related ones were clarified in C++17. Now std::enable_shared_from_this<T> is specified as if having a single std::weak_ptr<T> weak_this; member. For a non-array specialization of std::shared_ptr, that member is assigned by std::shared_ptr constructors, std::make_shared, and std::allocate_shared as described in [util.smartptr.shared.const]/1:

Enables shared_­from_­this with p, for a pointer p of type Y*, means that if Y has an unambiguous and accessible base class that is a specialization of enable_­shared_­from_­this, then remove_­cv_­t<Y>* shall be implicitly convertible to T* and the constructor evaluates the statement:

if (p != nullptr && p->weak_this.expired())
  p->weak_this = shared_ptr<remove_cv_t<Y>>(*this, const_cast<remove_cv_t<Y>*>(p));

So the correct behavior of the second main in my OP is now that no exception will be thrown and both "not empty" checks will show true. Since at the call to lock2() the internal weak_ptr is already owned and therefore not expired(), lock2() leaves the weak_ptr unchanged, and so the second call to shared_from_this() returns a shared_ptr which shared ownership with p1.

Answer 3

I agree this is a hole in the specification, thus a defect. It's basically the same as http://open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#2179 although that issue comes at it from a slightly different (and IMHO more obviously broken) angle.

I'm not sure I agree that this is a misuse of shared_ptr, I think it's fine to do that with shared_ptrs, because unlike the code in issue 2179 you use no-op deleters. I think the problem is when you try to combine that kind of use of shared_ptr with enable_shared_from_this.

So my first thought was to fix it by extending the requirements of shared_from_this:

Requires: enable_shared_from_this<T> shall be an accessible base class of T. *this shall be a subobject of an object t of type T. There shall be at least one shared_ptr instance p that owns &t and any other shared_ptr instances that own &t shall share ownership with p.

This isn't quite sufficient though, because your example meets that requirement: at the second call to shared_from_this() there is only one owner (p1) but you've already "corrupted" the state of the enable_shared_from_this base class by calling lock2().

A smaller form of the program is:

#include <memory>
using namespace std;

int main()
{
  struct X : public enable_shared_from_this<X> { };
  auto xraw = new X;
  shared_ptr<X> xp1(xraw);   // #1
  {
    shared_ptr<X> xp2(xraw, [](void*) { });  // #2
  }
  xraw->shared_from_this();  // #3
}

All three of libstdc++, libc++ and VC++ (Dinkumware) behave the same and throw bad_weak_ptr at #3, because at #2 they update the weak_ptr<X> member of the base class to make it share ownership with xp2, which goes out of scope leaving the weak_ptr<X> in the expired state.

Interestingly boost::shared_ptr doesn't throw, instead #2 is a no-op and #3 returns a shared_ptr that shares ownership with xp1. This was done in response to a bug report with almost exactly the same example as the one above.

Answer 4

Yes, there is a defect here in C++11. In allowing this:

It's a bit unusual to have two shared_ptr objects store the same pointer &x but not share ownership, and to use "deleters" that do not end the lifetime of *get(), but nothing forbids it.

This should be explicitly stated to be undefined behavior, regardless of what the "deleters" do. Sure, it may be technically not illegal to do things that way.

However, you are lying to people who use the code. The expectation of anyone who receives a shared_ptr is that they now have ownership of the object. So long as they keep that shared_ptr (or a copy thereof) around, the object it points to will still exists.

That is not the case with your code. So I would say that it is syntactically correct but semantically invalid.

The language for shared_from_this is fine. It's the language for shared_ptr that needs changing. It should state that it is undefined behavior to create two separate unique pointers that "own" the same pointer.