Innocuous code causes DGI module to delete itself

[mcatanzaro]

This completely innocuous code causes a DGI module (ESAgent) to delete itself:

void ESAgent::HandlePeerList(MessagePtr msg, PeerNodePtr peer)
{
    m_AllPeers = gm::GMAgent::ProcessPeerList(msg,GetConnectionManager());
}

where m_AllPeers is a PeerSet and ESAgent is a new DGI module following the same inheritance scheme as the other modules: that is, it publicly inherits from IPeerNode et. al.

In the constructor, we initialize m_AllPeers with a shared_ptr to our self, just like LB does:

            ESAgent::ESAgent(std::string uuid_, CBroker &broker):
            IPeerNode(uuid_, broker.GetConnectionManager()),
            m_broker(broker)
        {

            Logger.Trace << __PRETTY_FUNCTION__ << std::endl;
            PeerNodePtr self_(this);
            InsertInPeerSet(m_AllPeers, self_);

Now back to HandlePeerList. In the assignment operator, the reference count of each member of the previous peer set, including our self, is reduced by one. Since no other reference to us exists, the count goes to 0, and we delete our self. Unsurprisingly, this causes a crash.

I'm marking this as a LB issue because the terrible code in the constructor is copied from LB. The only reason LB does not crash is that, when receiving a new peer set, it manually loops through each peer and deletes the peer unless the peer is itself (where "itself" is not just an object representing that peer, but actually the one and only LBAgent, which is also an IPeerNode :S ).

This issue doesn't exist to fix the crash -- after all, ESAgent is not in our codebase. Rather, let's fix the confluence of terrible design that led to this:

• We need to clean up the obtuse inheritance hierarchy for DGI modules, issue #329.
• Observe that IPeerNode has no pure virtual functions and so must be renamed CPeerNode, or better, become a struct SPeerNode. (It doesn't even have a virtual destructor!! The stack trace does go through ~ESAgent but I truly don't know how that's possible without a virtual destructor. Maybe shenanigans inside Boost?)
• We can stop using shared_ptrs to IPeerNodes by making the the class copyable and observing that it only contains a string, which is usually faster to copy by value than by reference. shared_ptrs are way overkill for strings, especially considering they're probably interned anyway....

[mcatanzaro]

Moyeen, for a simple solution, try replacing this code in the constructor:

            PeerNodePtr self_(this);
            InsertInPeerSet(m_AllPeers, self_);

with this:

InsertInPeerSet(m_AllPeers, CGlobalPeerList::instance().Find(GetUUID())->second);

That should prevent the crash.

[mcatanzaro]

It would also be a good idea to privately inherit from IPeerNode, which would have disallowed this nonsense in the first place:

class ESAgent : public IReadHandler, private IPeerNode, public IAgent< boost::shared_ptr<IPeerNode> >

[mcatanzaro]

Haha, Stephen you fixed LB's awful constructor in the same way I suggested above in 6c3033b4d2e5191537a5cb99440c711044898185, which unfortunately was after 1.3 (which is what Moyeen is using).

[scj7t4]

So... this is fixed. GG.

[mcatanzaro]

No, of the three issues at the bottom of comment #0, all are unfixed and a separate issue has been created for only one of them.

[scj7t4]

Fixed in #381