CODEWITHSUNDEEP
c++
cyrusbehr
cyrusbehr

Reputation: 1301

What is the proper design pattern for a callback in destructor which uses a pointer which may or may not point to valid object?

Let's say we have the following scenario.

We have a ImageManager class which is used to internally store and manage image data. The ImageManager class has a public member populateImage, which will read an image into memory, then return a populated MyImage which is a std::shared_ptr around an Image object. This object will contain a uuid which maps to the image data which is managed by ImageManager.

Finally, we define a callback function which gets called in the Image destructor and properly cleans up the image data stored by the ImageManager.

Here is the example:

#include <functional>
#include <vector>
#include <unordered_map>
#include <memory>

class Image {
public:
    Image() = default;
    ~Image(){
        if (m_deleter) {
            m_deleter();
        }
    }
private:
    friend class ImageManager;
    std::function<void()> m_deleter = nullptr;
    unsigned int m_uuid;
};

using MyImage = std::shared_ptr<Image>;

class ImageManager {
public:
    void removeImage(unsigned int uuid) {
        auto iter = m_imageMap.find(uuid);
        if (iter == m_imageMap.end()) {
            throw std::runtime_error("Unable to find image for UUID: " + std::to_string(uuid));
        }
        m_imageMap.erase(iter);
    }

    void populateImage(MyImage& image) {
        image = std::make_shared<Image>();

        static unsigned int uuid = 0;
        ++uuid;

        image->m_uuid = uuid;

        auto img = std::vector<uint8_t>(1000, 0);
        m_imageMap[uuid] = img;

        unsigned int currentUUID = uuid;
        auto callbackFunc = [this, currentUUID]() {
            this->removeImage(currentUUID);
        };

        image->m_deleter = callbackFunc;
    }
private:
    std::unordered_map<unsigned int, std::vector<uint8_t>> m_imageMap;
};

The issues arise when our instance of the ImageManager falls out of scope before our instances of Image, for example in the following driver code:

int main() {
    MyImage img1, img2;
    {
        ImageManager manager;
        manager.populateImage(img1);
        manager.populateImage(img2);
    }
}

Running this program prints:

terminate called after throwing an instance of 'std::runtime_error'
  what():  Unable to find image for UUID: 2

But ultimately I understand that this is undefined behavior as the this pointer in m_deleter no longer points to a valid object.

What is the proper design pattern in order to avoid this problem?

Upvotes: 2

Views: 472

Answers (3)

eerorika
eerorika

Reputation: 238351

Since the callback depends on the manager, it should have ownership.

It cannot have proper ownership because the manager already owns the (object that contains) the callback, but it can have weak shared ownership. A working adjustment of your example:

class ImageManager : std::enable_shared_from_this<ImageManager> {
// ...

    auto callbackFunc = [this_w = weak_from_this(), currentUUID]() {
        if (auto this_ = this_w.lock())
            this_->removeImage(currentUUID);
    };
// ...

    auto manager = std::make_shared<ImageManager>();
    manager->populateImage(img1);
    manager->populateImage(img2);

Upvotes: 1

Daniel
Daniel

Reputation: 31579

One solution for this would be to remove the callbacks when the ImageManager is destroyed.
In the ImageManager destructor you can loop through m_imageMap and set all deleters to null.

Upvotes: 0

Dmitry Kuzminov
Dmitry Kuzminov

Reputation: 6584

Overall the design smells. If the ImageManager is a local object, and MyImage are outside of the scope, why do you need to delete the items in the map?

Anyway, I promised you to show the shared/weak idiom. Wrap the map into a shared_ptr. That would mean that the map will be destroyed together with the ImageManager (if you wouldn't copy this shared pointer):

std::shared_ptr<std::unordered_map<unsigned int, cv::Mat>> m_imageMap;

Store a weak pointer in the lambda:

    std::weak_ptr<std::unordered_map<unsigned int, cv::Mat>> weakPtr = m_imageMap;
    auto callbackFunc = [weakPtr, uuid]() {
        auto imageMap = weakPtr.lock();
        if (imageMap)
            auto iter = imageMap.find(uuid);
            if (iter == imageMap.end()) {
                throw std::runtime_error("Unable to find image for UUID: " + std::to_string(uuid));
            }
            imageMap.erase(iter);
        }
    };
    image->m_deleter = callbackFunc;

Weak pointer will know whether the shared pointer counterpart is destroyed or not.

Keep in mind that you shouldn't use std::make_shared in this case: otherwise the memory associated with the map will be freed only when the last image is destroyed.

Upvotes: 1

Related Questions