Why is the desctructor being invoked in this scenario?

Question

In the following code , I am not able to understand why the destructor of the class Buf is invoked twice. When debugging I can see that it is being invoked the first time when the running thread is leaving the function Test::produce. The second time is when leaving the main function which essentially is when destructing the class EventQueue, something I would expect.

However, I dont understand why when leaving the function Test::produce the destructor of Buf is invoked. Specifically, I create the class Buf as a r-value passing it to the EventQueue and move to its internal cache. In fact, this has created me the problem that I end up trying ti free the same pointer twice which throws an exception.

template<typename T>
class EventQueue{
public:
    void offer(T&& t) {
        m_queue.try_emplace(std::this_thread::get_id()).first->second.push(std::move(t));
    };

    std::unordered_map<std::thread::id, std::queue<T>> m_queue;

};

class Buf{
    const uint8_t *m_data;
    const size_t m_size;
public:
    Buf(const uint8_t *data, size_t size) : m_data(data), m_size(size) { }
    size_t size() const { return m_size; }
    const uint8_t *data() const { return m_data; }
    ~Buf()
    {
        std::cout << "dtor called " << std::endl;
        free((void *)m_data);
    }
};


class Test{ and was not expecting
public:
    Test(shared_ptr<EventQueue<Buf>> buf) : m_buf(buf)
    {
        std::thread t1 = std::thread([this] { this->produce(10); });
        t1.detach();
    };

    void produce(int msg_size) {
        m_buf->offer(Buf(new uint8_t[msg_size], 10));
    }
    std::shared_ptr<EventQueue<Buf>> m_buf;
};

int main()
{
    auto event_queue = std::make_shared<EventQueue<Buf>>();
    Test tt(event_queue);

    return 0;
}

When you move a value from one object to another, the original object still exists, but in a moved-from state. Moved-from or not, it is destroyed at the end of its lifetime. Moving an object does not and cannot cause its destructor to be skipped. — François Andrieux, Oct 12 '22 at 19:47
`m_buf->offer(Buf(new uint8_t[msg_size], 10));` manifests a temporary `Buf` object which gets moved into the memory location `EventQueue::m_queue` needs it to go... This is why you should always remove defaulted copy/move constructor/assignment operator, by defining move constructor/assignment operator as deleted, unless you know memberwise move will do the trick or you implement copy and/or move semantics yourself (or there is already a member resulting in the implicitly defined special members to be deleted)... — fabian, Oct 12 '22 at 19:47
Your `Buf` class contains serious bugs because it does not follow the rule of 0/5/3 : https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — François Andrieux, Oct 12 '22 at 19:48
I see `new`, but no `delete`. I do see a `free`, though and that can be worse. `new[]` pairs with `delete[]`. Nothing else can be countd on to get the job done. Can we talk you into using `std::vector`? — user4581301, Oct 12 '22 at 19:50
`std::cout << "dtor called " << std::endl;` -- You should be printing the value of `this`, not just a simple message, i.e. `std::cout << "dtor called on object " << this << std::endl;` More than likely you are not deleting the same object you believe you are deleting. It isn't a matter of the destructor being called twice -- destructors are called once, it is that you are dealing with different objects. — PaulMcKenzie, Oct 12 '22 at 20:05

The Dreams Wind · Accepted Answer · 2022-10-16T21:26:00.503

The destructor is called two times because you have two objects to destroy. First - the temporary you created as an argument for the offer function parameter:

void produce(int msg_size) {
    m_buf->offer(Buf(new uint8_t[msg_size], 10));
}

Second - when you add this temporary to std::queue container, it makes a copy under the hood:

void offer(T&& t) {
    m_queue.try_emplace(std::this_thread::get_id()).first->second.push(std::move(t));
};

Every temporary object created must always be destructed. However the problem is not about how many objects were destructed, but that you ignore the rules of zero, three and five here. I.e. if you create any of a destructor, a copy constructor or a copy-assignment operator, you are supposed to take care of all three. Another side effect is that the compiler will not generate the move constructor and move assignment operator for you when any of the big three are explicitly defined. Thus, when passing rvalue-reference to a Buf constructor, you actually ends up with a copy constructor.

However even if it was a default move constructor, it would not solve your problem, because resources represented with raw pointers which your class instance "owns" (and is supposed to delete at some point) are not quite compatible with the implicit move constructor, which merely does member-wise std::move:

For non-union class types (class and struct), the move constructor performs full member-wise move of the object's bases and non-static members, in their initialization order, using direct initialization with an xvalue argument.

For any built-in types (including raw pointers), it means that nothing actually happens and they are just copied.

Long story short: you have to nullify the source object's member raw pointer explicitly:

Buf(Buf&& other) noexcept : m_data{ std::exchange(other.m_data, nullptr) }, m_size{ other.m_size } {}

The better solution would be to not mess with rules of three/five and stick to rule of zero, by leveraging RAII idiom and letting automatic storage duration to handle the resources without explicitly allocating/releasing them:

class Buf{
    const std::vector<std::uint8_t> m_data;
public:
    Buf(std::vector<std::uint8_t> data) : m_data{ std::move(data) } { }

    const std::vector<std::uint8_t>& data() const {
        return m_data;
    }

};

Why is the desctructor being invoked in this scenario?

1 Answers1