Removing back pointers with the use of explicit instantiation declarations causes std::bad_weak_ptr exception

Question

I have developed some code that compiles correctly, but fails at (debug) runtime. I am using VS2015.

Background: I am building an advanced message engine. To make the programmatic addition of new messages maintainable, in the production code I took the time to craft the initial messages using explicit initialization declaration C++ construct. This works and makes the crafting of new messages cookie-cutter, not to mention reducing the maintenance of the messaging guts to almost nothing. Here is the skeleton code for this functionality:

#include <memory>

template< typename D_T >
struct H // prototype for all explicit initialization declarations (EID)
{
  H( D_T& d ) : x { d } {}
  D_T& x;
};

template< typename D_T >
struct B  // base class for derived objects D1 and D2
{
  B( D_T& d ) : d { d } {}
  D_T& d; // a kind of backptr initialized when the EIDs are contructed

  // actual EIDs a and b
  H< D_T > a { d };
  H< D_T > b { d };
};

struct D1 : public B< D1 >
{
  D1() : B( *this ) {}
  void Func1() {}
};

struct D2 : public B< D2 >
{
  D2() : B( *this ) {}
  void Func2() {}
};



int main()
{
  D1 d1;
  D2 d2;

  // as designed either derived object can access either explicitly initialized member a or b 
  d1.a.x.Func1(); // OK
  d1.b.x.Func1(); // OK
  d2.a.x.Func2(); // OK
  d2.b.x.Func2(); // OK

  return 0;
}

This code compiles and runs.

But my derived objects in the real code are shared ptrs. I therefore added this functionality to the code. Notice that I am obtaining the derived class’es this ptr using the enable_shared_from_this construct:

#include <memory>

template< typename D_T >
struct H
{
  H( std::shared_ptr< D_T >& d ) : x { d } {}
  std::shared_ptr< D_T >& x;
};

template< typename D_T >
struct B
{
  B( std::shared_ptr< D_T >& d ) : d { d } {}
  std::shared_ptr< D_T >& d;

  H< D_T > a { d }; // a is initialized with D1
  H< D_T > b { d };
};

struct D1: public std::enable_shared_from_this< D1 >, public B< D1 >
{
  D1() : B( shared_from_this() ) {} // runtime error: bad weak prt
  void Func1() {}
};

struct D2: public std::enable_shared_from_this< D2 >, public B< D2 >
{
  D2() : B( shared_from_this() ) {}
  void Func2() {}
};

int main()
{
  D1 d1;
  D2 d2;

  d1.a.x->Func1();
  d1.b.x->Func1();
  d2.a.x->Func2();
  d2.b.x->Func2();

  return 0;
}

This code compiles. However, it does not run and at the D1 constructor, it breaks with exception std::bad_weak_ptr.

I have attempted to change shared ptrs to weak ptrs without success. Anyone see the problem?

Edit 1: Per @pat’s observation that shared_from_this() is not callable from the constructor, see the modified code below which now compiles and runs:

#include <memory>

template< typename D_T >
struct H
{
  H( D_T& d ) : x { d } {}
  D_T& x;
};

template< typename D_T >
struct B
{
  B( D_T& d ) : d { d } {}
  D_T& d;

  H< D_T > a { d };
  H< D_T > b { d };
};

struct D1 : public std::enable_shared_from_this< D1 >, public B< D1 >
{
  D1() : B( *this ) {}
  void Func1() {}
};

struct D2 : public std::enable_shared_from_this< D1 >, public B< D2 >
{
  D2() : B( *this ) {}
  void Func2() {}
};

int main()
{
  D1 d1;
  D2 d2;

  d1.a.x.Func1();
  d1.b.x.Func1();
  d2.a.x.Func2();
  d2.b.x.Func2();

  return 0;
}

Edit 2: The code below is a re-write of my original posting's code and builds on @pat's answer. Here is what was changed: The explicit instantiation declarations (EID) were moved to their derived classes. B is no longer trying to reference the derived object. This was a plain mistake. The weak_ptr as a back pointer was replaced by a simple back ptr (as was the case in the prototypes). No problem with leaks since the derived objects (D1 and D2) own the object outright. (In the producion code the member types are shared ptrs to prevent leaks.)

#include <memory>
#include <cassert>

template< typename D_T >
struct H
{
  H( D_T* d ) : x { d } {}
  D_T* x;
  int qq { 0 };
};

struct B
{
  B() {}
  int rr { 0 };
};

struct D1 : public B
{
  H< D1 > a { this }; // explicit instantiation declaration 
  int ss { 0 };
};

struct D2 : public B
{
  H< D2 > b { this }; // explicit instantiation declaration
  int tt { 0 };
};


int main()
{
  D1 d1;
  D2 d2;
  d1.rr = 99;
  d2.b.x->rr = 88;
  assert( d1.rr == d1.a.x->rr ); // OK
  assert( d2.rr == d2.b.x->rr ); // OK

  return 0;
}

The design invariant that the code maintenance complexity be reduced from exponential (as was the case in the prototypes) to linear when adding any number of EIDs has been achieved.

Answer 1

The object has to be managed by a shared pointer for shared_from_this to work. It's actually undefined behavior in C++14 to call shared_from_this on an object which is not already managed by a shared_ptr. So, you won't be able to call shared_from_this from a constructor since the object won't be inside a shared_ptr at that point.

Example from cppreference...

struct Good: std::enable_shared_from_this<Good>
{
    std::shared_ptr<Good> getptr() {
        return shared_from_this();
    }
};
// Bad: shared_from_this is called without having std::shared_ptr owning the caller 
try {
    Good not_so_good;
    std::shared_ptr<Good> gp1 = not_so_good.getptr();
} catch(std::bad_weak_ptr& e) {
    // undefined behavior (until C++17) and std::bad_weak_ptr thrown (since C++17)
    std::cout << e.what() << '\n';    
}

Answer 2

Objects in C++ either have automatic lifetime or dynamic lifetime.

Varibles of automatic lifetime cannot meaningfully be managed by shared_ptr, barring a strange "deleter". Automatic lifetime can also be called "on the stack".

Your code has a whole pile of misunderstandings.

First, references only rarely extend lifetime. Storing a std::shared_ptr<X>& in a class is almost always a bad idea; it will not extend the lifetime of anything, let alone the X: even the shared_ptr's lifetime won't be extended.

B( shared_from_this() )

shared_from_this() creates a shared_ptr<T>, not a shared_ptr<T>&, passing it to B's constructor that expects a reference is nonsense. That this compiles at all is a flaw in MSVC2015, where it by default implements an extension that permits rvalues to be cast to references.

The fact that you are calling it in a constructor also means it cannot work. shared_from_this() calls .lock() on a weak_ptr stored in enable_shared_from_this, which is populated once this is actually managed with a shared_ptr.

This almost always doesn't happen until after the object is created. In your case, it never happens, but even if you where creating it in make_shared it wouldn't happen until after the object constructor is complete.

Backing up, your storage of &s in your first code is bad. Storing & in a type gives it reference semantics on assign and copy, and doing so is very very questionable unless you really understand what it means. The fact that & is supposed to be *this means you are doing it wrong: a copy construct or assignment won't maintain that invariant.

Similar problems occur in your bottom shared pointer design; reseating those shared pointers is not going to happen automatically, so they'll do the wrong thing by default.

Having an object with shared pointers to itself is a horrible idea; it is an immortal object, because it provides itself with its own life.

I honestly have almost no idea what this code is supposed to do, because insofar as I can understand it, it is doing harmful things to no purpose. Maybe you are trying to have member objects that know who the object that owns them is? And have somehow attached this to being required for an advanced messaging system?

If your goal is a broadcaster/listener system, the question becomes what exactly are your requirements? The weaker your requirements, the simpler the system is, and complexity has large costs.

If most of your broadcasters have a handful of listeners that do not change insanely more often than the frequency of broadcast, a simple broadcaster/listener system which stores weak pointers in the broadcaster and shared pointer tokens in the listeners would solve your problem.

using token = std::shared_ptr<void>;
template<class...Args>
struct broadcaster {
  using invoker = std::function<void(Args...)>;
  using sp_message = std::shared_ptr<invoker>;
  using wp_message = std::weak_ptr<invoker>;

  token register( sp_message msg ) {
    listeners.push_back(msg);
    return msg;
  }
  token register( invoker f ) {
    auto msg = std::make_shared<invoker>(std::move(f));
    return register( std::move(msg) );
  }
  void operator()( Args...args )
  {
    auto it = std::remove_if( listeners.begin(), listeners.end(),
      [](auto&& ptr) { return !ptr.lock(); }
    };
    listeners.erase(it, listeners.end());

    auto tmp = listeners;
    for (auto&& target:tmp)
      if (auto pf = target.lock())
        (*pf)(args...);
  }
private:
  std::vector<wp_message> listeners;
};

code not tested.

Here our broadcaster<int> b; can have std::function<void(int)> f passed to it. It returns a shared_ptr<void> aka token. So long as that shared_ptr<void> persists, calling b(7) will call f(7).

Alternatively, you can pass a shared_ptr<std::function<void(int)>>. Then so long as that shared_ptr, or the token returned, persists, the listener will be broadcast to. (This permits you to tie the lifetime to some other shared_ptr)

It cleans up its listeners, removing dead ones, before each time it broadcasts.

If the broadcaster dies before the listener, the listener is not informed (unless you set up a broadcaster to say exactly that!) The source of a message is not included, unless it is included in the signature of the broadcaster.

tokens are not signature-dependent; so a class that listens to many broadcasters for its entire life can have a std::vector<token>.

If it needs to track its broadcasters lifetime, we can write an on destroy broadcaster:

struct on_destroy:broadcaster<on_destroy const*> {
  ~on_destroy() {
    (*this)(this);
  }
};

Then we can add a listen-to thing:

struct listen_until_gone {
  template<class...Args>
  void register( on_destroy& d, broadcaster<Args...>& b, std::function<void(Args...)> m )
  {
    auto it = listeners.find(&d);
    if (it != listeners.end()) {
      listeners[&d] = {d.register( [this](on_destroy const*d){
        this->listeners.erase(d);
      })};
    }
    listeners[&d].push_back(
      b.register( std::move(m) )
    );
  }
private:
  std::unordered_map< on_destroy const*, std::vector<token> > listeners;
};

now a listener can have a listen_until_gone listen;.

To listen to a given broadcaster who has an on_destroy, we do:

listen.register(
  bob.on_destroy, bob.name_change,
  [this]( std::string const& new_name ){
    this->bob_name_changed(new_name);
  }
);

and forget about it.

But if broadcasters tend to outlive listeners, I just listen and store it in a vector.