Storing thread in vector member variable triggers breakpoint

Question

I have a class called Executor with a vector<thread*> as a member variable. the submit(const std::function<void()>& f) member function is supposed to start the passed lambda as a thread and add it to the vector. The destructor joins all threads stored in the vector. The first lambda prints the result, but after that my code runs into a breakpoint somehow. What am I doing wrong?

here is my code for Executor.h:

#include <thread>
#include <mutex>
#include <iostream>
#include <vector>
#include <functional>

using namespace std;
class Executor
{
protected:
    vector<thread*> threads;
    mutex m;
public:
    void submit(const std::function<void()>& f);
    Executor();
    ~Executor();
};

Executor.cpp:

#include "Executor.h"

void Executor::submit(const std::function<void()>& f)
{
    
    {
        lock_guard<mutex> lock(m);
        thread t(f);
        
        threads.push_back(&t);
    }
    
}

Executor::Executor() {}

Executor::~Executor()
{
    for (unsigned int i = 0; i < threads.size(); i++)
    {
        threads.at(i)->join();
        delete threads.at(i);
    }
    threads.clear();
}

And main():

Executor ex;
        auto fact = [](){cout<< "factor(55) = " << factor(55); };
        auto pk = [](){cout << "P2(100, 50) = " << P2(100, 50); };
        auto fib = [](){cout << "fibonacci(45) = " << fibonacci(45); };


        ex.submit(fact);
        ex.submit(pk);
        ex.submit(fib);

Why do you have a vector of _pointers_ to threads? This does not make any sense. Create a vector of thread objects themselves. — Daniel Langr, Jan 06 '21 at 08:37

score 2 · Accepted Answer · answered Jan 06 '21 at 08:54

In threads.push_back(&t); the pointer &t is only valid until the end of the scope which t is declared in. This causes 3 problems:

At the end of the scope inside submit the thread is destructed without calling join causing std::terminate to be called
In the destructor of Executor ->join is called on a dangling pointer
In the destructor of Executor you call delete on something that wasn't allocated with new

There is no need to use pointers in this code, if you store your threads directly in the vector your code will be simpler and safer:

class Executor
{
protected:
    vector<thread> threads;
    mutex m;
public:
    void submit(const std::function<void()>& f);
    Executor();
    ~Executor();

    Executor(const Executor&) = delete;
    Executor& operator=(const Executor&) = delete;
};

void Executor::submit(const std::function<void()>& f)
{  
    {
        lock_guard<mutex> lock(m);
        threads.emplace_back(f);
    }   
}

Executor::Executor() {}

Executor::~Executor()
{
    for (auto& thread : threads)
    {
        thread.join();
    }
    threads.clear();
}

Note I've also deleted the copy constructor and assignment operator to comply with the rule of three

Storing thread in vector member variable triggers breakpoint

1 Answers1