C++ - shared_ptr of abstract class

Question

I have the following code:

#include <string>
#include <queue>
#include <thread>
#include <iostream>
using namespace std;

class MsgType {
public:
    virtual string getData() const = 0;
    static MsgType* getMsg();
};

class Msg1 : public MsgType {
    string getData() const override final {
        return "Msg1";
    }
};

class Msg2 : public MsgType {
    string getData() const override final {
        return "Msg2";
    }
};

queue<shared_ptr<MsgType>> allMsgs;

MsgType* MsgType::getMsg() {
    shared_ptr<MsgType> msg_sp = nullptr;
    if (!allMsgs.empty()) {
        msg_sp = allMsgs.front();
        allMsgs.pop();
    }
    if (msg_sp) {
        MsgType* mt = msg_sp.get();
        cout << "[in the method] " << mt->getData() << endl;
        return mt;
    } else {
        return nullptr;
    }
}

int main() {
    MsgType* msg1 = new Msg1();
    MsgType* msg2 = new Msg2();
    shared_ptr<MsgType> msg;
    msg.reset(msg1);
    allMsgs.push(msg);
    msg.reset(msg2);
    allMsgs.push(msg);
    MsgType* tryGetMsg = MsgType::getMsg();
    cout << "[out of the method] " << tryGetMsg->getData() << endl;
}

In the MsgType::getMsg() method I can see the output, but in the main() I can't. I belive that it's trying to call MsgType::getData() which is virtual.
How can I get the MsgType outside of this method, in a way that I can access the derived class' methods?

Thanks!

Answer 1

The immediate fix is to just return a shared_ptr from getMsg:

shared_ptr<MsgType> MsgType::getMsg() {
    shared_ptr<MsgType> msg_sp;
    if (!allMsgs.empty()) {
        msg_sp = allMsgs.front();
        allMsgs.pop();
    }
    if (msg_sp) {
        cout << "[in the method] " << msg_sp->getData() << endl;
    }
    return msg_sp;
}

and stop converting needlessly between smart and raw pointers.

The message object must be kept alive until the caller has finished using it. Since you're using shared_ptr to manage the object lifetime, you need a shared_ptr to continue existing as long as you want to use the object.

In general, mixing raw and smart pointers to the same objects is risky, because the smart pointers can only track the references they know about: that is, shared_ptr has to know everywhere a pointer to the object is being shared. It can only do this if every one of those pointers is a shared_ptr.

---

Note also that the easy way to diagnose object lifetime problems is to write a destructor that logs something. This brings us on to the second problem: in order for MsgType to be a suitable abstract base class here, it needs a virtual destructor.

Without that, the shared_ptr will try to destroy your object when the refcount becomes zero, but be unable (in general) to do so correctly.

class MsgType {
public:
    virtual ~MsgType() {}
    virtual string getData() const = 0;
};

---

Veering finally into code review, I intentionally omitted getMsg above.

Having a class static method to access a global queue is just weird. If you want to keep that layout, the allMsgs queue should probably be class static as well.

Instead, it's probably better to just keep a msg_queue object wherever you actually need it, with no statics or globals.

Answer 2

I don't know why you are mixing std::shared_ptr and C-style pointers this way, but let's ignore this and assume it's just as an exercise.

Having a look at the bottom half of your code (slightly reduced), we have this:

std::queue<std::shared_ptr<MsgType>> allMsgs;

MsgType* MsgType::getMsg();

int main() {
    MsgType* msg1 = new Msg1();
    std::shared_ptr<MsgType> msg;
    msg.reset(msg1);               // <--- 1. here, msg1 is owned by msg
    allMsgs.push(msg);             // <--- 2. now, msg1 is also owned by allMsgs
    msg.reset();                   // <--- 3. msg1 only owned by allMsgs

    MsgType* tryGetMsg = MsgType::getMsg();  // <--- see below : nobody keeping msg1 alive!
    std::cout << "[out of the method] " << tryGetMsg->getData() << std::endl;
}

MsgType* MsgType::getMsg() {
    std::shared_ptr<MsgType> msg_sp = nullptr;
    if (!allMsgs.empty()) {
        msg_sp = allMsgs.front();  // <--- 4. msg1 owned by msg_sp & allMsgs
        allMsgs.pop();             // <--- 5. msg1 owned by msg_sp only
    }
    if (msg_sp) {
        MsgType* mt = msg_sp.get();
        std::cout << "[in the method] " << mt->getData() << std::endl;
        return mt;
    } else {
        return nullptr;
    }
}                                  // <--- 6. msg_sp destroyed... oh oh... msg1 dead :) 

As as small addition, you can construct a shared base class pointer directly from a derived one, e.g.

auto msg_sp = std::shared_ptr<MsgType>(std::make_shared<Msg1>());

Answer 3

Here:

MsgType* MsgType::getMsg() {
    shared_ptr<MsgType> msg_sp = nullptr;
    if (allMsgs.empty()) {
        msg_sp = allMsgs.front();
        allMsgs.pop();
    }
    if (msg_sp) {
        MsgType* mt = msg_sp.get();
        cout << "[in the method] " << mt->getData() << endl;
        return mt;
    } else {
        return nullptr;
    }
}

When allMsgs is not empty you you copy front then pop. At that moment there is a single shared_ptr managing that object: msg_sp. Then you retrieve a raw pointer via get and return that, but when the function returns the use-count decrements to 0 and the managed object is destroyed. The returned pointer is invalid.

I find your mixing of raw and shared_ptr a little confusing. When you have a shared_ptr managing the lifetime of the object then you cannot first get a raw pointer, then let the shared_ptr destroy the managed object and still use the raw pointer. You need to properly transfer ownership when you don't want the shared pointer to destroy the managed object.