Smart pointers Object pool test application fails at finish/exit

Question

I have written a multi-threaded app in Qt/C++11 , Windows.
The idea was to have and recycle some strings from a pool, using smart pointers.
Here is stringpool.cpp:

#include "stringpool.h"

QMutex StringPool::m_mutex;
int StringPool::m_counter;
std::stack<StringPool::pointer_type<QString>> StringPool::m_pool;

StringPool::pointer_type<QString> StringPool::getString()
{
    QMutexLocker lock(&m_mutex);

    if (m_pool.empty())
    {
        add();
    }
    auto inst = std::move(m_pool.top());

    m_pool.pop();
    return inst;
}

void StringPool::add(bool useLock, QString * ptr)
{
    if(useLock)
        m_mutex.lock();

    if (ptr == nullptr)
    {
        ptr = new QString();
        ptr->append(QString("pomo_hacs_%1").arg(++m_counter));
    }

    StringPool::pointer_type<QString> inst(ptr, [this](QString * ptr) { add(true, ptr); });
    m_pool.push(std::move(inst));

    if(useLock)
        m_mutex.unlock();
}

And here is stringpool.h:

#pragma once

#include <QMutex>
#include <QString>
#include <functional>
#include <memory>
#include <stack>

class StringPool
{
public:
    template <typename T> using pointer_type = std::unique_ptr<T, std::function<void(T*)>>;
    //
    StringPool() = default;
    pointer_type<QString> getString();

private:
    void add(bool useLock = false, QString * ptr = nullptr);
    //
    static QMutex m_mutex;
    static int m_counter;
    static std::stack<pointer_type<QString>> m_pool;
};

And here is the test app:

#include <QtCore>
#include "stringpool.h"

static StringPool Pool;


class Tester : public QThread
{
public:
    void run() override
    {
        for(int i = 0; i < 20; i++)
        {
            {
                auto str = Pool.getString();
                fprintf(stderr, "Thread %p : %s \n", QThread::currentThreadId(), str->toUtf8().data());
                msleep(rand() % 500);
            }
        }
        fprintf(stderr, "Thread %p : FINITA! \n", QThread::currentThreadId());
    }
};

#define MAX_TASKS_NBR       3

int main(int argc, char *argv[])
{
    QCoreApplication app(argc, argv);

    Tester tester[MAX_TASKS_NBR];

    for(auto i = 0; i < MAX_TASKS_NBR; i++)
        tester[i].start();

    for(auto i = 0; i < MAX_TASKS_NBR; i++)
        tester[i].wait();
    //
    return 0;
}

It compiles ok, it runs and produces the following result:

Well, the idea is that the app runs (apparently) OK.
But immediately after it finishes, I have this error:

Does anyone have an idea how can I fix this?

Answer 1

The reason for this error has to do with the smart pointer and not the multithreading.

You define pointer_type as an alias for unique_ptr with a custom deleter

template <typename T> using pointer_type = std::unique_ptr<T, std::function<void(T*)>>;

You create strings with custom deleters

void StringPool::add(bool useLock, QString * ptr)
{    
    if (ptr == nullptr)
    {
        ptr = new QString();
        ptr->append(QString("pomo_hacs_%1").arg(++m_counter));
    }

    StringPool::pointer_type<QString> inst(ptr, [this](QString * ptr) { add(true, ptr); }); // here
    m_pool.push(std::move(inst));
}

At the end of the program, m_pool goes out of scope and runs its destructor.

Consider the path of execution...m_pool will try to destroy all its members. For each member, the custom deleter. The custom deleter calls add. add pushes the pointer to the stack.

Logically this is an infinite loop. But it's more likely to create some kind of undefined behavior by breaking the consistency of the data structure. (i.e. The stack shouldn't be pushing new members while it is being destructed). An exception might occur due to function stack overflow or literal stack overflow (heh) when there is not enough memory to add to the stack data structure. Since the exception occurs in a destructor unhandled, it ends the program immediately. But it could also very likely be a seg fault due to the pushing while destructing.

Fixes:

I already didn't like your add function.

StringPool::pointer_type<QString> StringPool::getString()
{
    QMutexLocker lock(&m_mutex);

    if (m_pool.empty())
    {
        auto ptr = new QString(QString("pomo_hacs_%1").arg(++m_counter));
        return pointer_type<QString>(ptr, [this](QString* ptr) { reclaim(ptr); });
    }

    auto inst = std::move(m_pool.top());    
    m_pool.pop();
    return inst;
}

void StringPool::reclaim(QString* ptr)
{
    QMutexLocker lock(&m_mutex);

    if (m_teardown)
        delete ptr;
    else
        m_pool.emplace(ptr, [this](QString* ptr) { reclaim(ptr); });
}

StringPool::~StringPool()
{
    QMutexLocker lock(&m_mutex);
    m_teardown = true;
}

StringPool was a static class but with this fix it must now be a singleton class.

It might be tempting to pull m_teardown out of the critical section, but it is shared data, so doing will open the door for race conditions. As a premature optimization, you could make m_teardown an std::atomic<bool> and perform a read check before entering the critical section (can skip the critical section if false) but this requires 1) you check the value again in the critical section and 2) you change from true to false exactly once.