C++ Concurrency segfault on mutex

Question

Hello,

i am quite new to C++ but I have 6 years Java experience, 2 years C experience and some knowledge of concurrency basics. I am trying to create a threadpool to handle tasks. it is below with the associated test main.

it seems like the error is generated from

void ThreadPool::ThreadHandler::enqueueTask(void (*task)(void)) {
    std::lock_guard<std::mutex> lock(queueMutex);

as said by my debugger, but doing traditional cout debug, i found out that sometimes it works without segfaulting and removing

threads.emplace(handler->getSize(), handler);

from ThreadPool::enqueueTask() improves stability greatly.

Overall i think it is related too my bad use of condition_variable (called idler).

compiler: minGW-w64 in CLion

.cpp

#include <iostream>
#include "ThreadPool.h"

ThreadPool::ThreadHandler::ThreadHandler(ThreadPool *parent) : parent(parent) {
    thread = std::thread([&]{
        while (this->parent->alive){
            if (getSize()){
                std::lock_guard<std::mutex> lock(queueMutex);
                (*(queue.front()))();
                queue.pop_front();
            } else {
                std::unique_lock<std::mutex> lock(idlerMutex);
                idler.wait(lock);
            }
        }
    });
}

void ThreadPool::ThreadHandler::enqueueTask(void (*task)(void)) {
    std::lock_guard<std::mutex> lock(queueMutex);
    queue.push_back(task);
    idler.notify_all();
}

size_t ThreadPool::ThreadHandler::getSize() {
    std::lock_guard<std::mutex> lock(queueMutex);
    return queue.size();
}

void ThreadPool::enqueueTask(void (*task)(void)) {
    std::lock_guard<std::mutex> lock(threadsMutex);
    std::map<int, ThreadHandler*>::iterator iter = threads.begin();
    threads.erase(iter);
    ThreadHandler *handler = iter->second;
    handler->enqueueTask(task);
    threads.emplace(handler->getSize(), handler);
}

ThreadPool::ThreadPool(size_t size) {
    for (size_t i = 0; i < size; ++i) {
        std::lock_guard<std::mutex> lock(threadsMutex);
        ThreadHandler *handler = new ThreadHandler(this);
        threads.emplace(handler->getSize(), handler);
    }
}

ThreadPool::~ThreadPool() {
    std::lock_guard<std::mutex> lock(threadsMutex);
    auto it = threads.begin(), end = threads.end();
    for (; it != end; ++it) {
        delete it->second;
    }
}

.h

#ifndef WLIB_THREADPOOL_H
#define WLIB_THREADPOOL_H

#include <mutex>
#include <thread>
#include <list>
#include <map>
#include <condition_variable>

class ThreadPool {
private:
    class ThreadHandler {
        std::condition_variable idler;
        std::mutex idlerMutex;
        std::mutex queueMutex;
        std::thread thread;
        std::list<void (*)(void)> queue;
        ThreadPool *parent;
    public:
        ThreadHandler(ThreadPool *parent);
        void enqueueTask(void (*task)(void));
        size_t getSize();
    };
    std::multimap<int, ThreadHandler*> threads;
    std::mutex threadsMutex;
public:
    bool alive = true;
    ThreadPool(size_t size);
    ~ThreadPool();

    virtual void enqueueTask(void (*task)(void));
};


#endif //WLIB_THREADPOOL_H

main:

#include <iostream>
#include <ThreadPool.h>

ThreadPool pool(3);

void fn() {
    std::cout << std::this_thread::get_id() << '\n';
    pool.enqueueTask(fn);
};

int main() {
    std::cout << "Hello, World!" << std::endl;
    pool.enqueueTask(fn);
    return 0;
}

Answer 1

Your main() function invokes enqueueTask().

Immediately afterwards, your main() returns.

This gets the gears in motion for winding down your process. This involves invoking the destructors of all global objects.

ThreadPool's destructor then proceeds to delete all dynamically-scoped threads.

While the threads are still running. Hilarity ensues.

You need to implement the process for an orderly shutdown of all threads.

This means setting active to false, kicking all of the threads in the shins, and then joining all threads, before letting nature take its course, and finally destroy everything.

P.S. -- you need to fix how alive is being checked. You also need to make access to alive thread-safe, protected by a mutex. The problem is that the thread could be holding a lock on one of two differented mutexes. This makes this process somewhat complicated. Some redesign is in order, here.