CODEWITHSUNDEEP
c++multithreadingstdvector
user8958070
user8958070

Reputation:

Storing thread in vector member variable triggers breakpoint

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);

Upvotes: 0

Views: 90

Answers (1)

Alan Birtles
Alan Birtles

Reputation: 36389

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:

  1. At the end of the scope inside submit the thread is destructed without calling join causing std::terminate to be called
  2. In the destructor of Executor ->join is called on a dangling pointer
  3. 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

Upvotes: 2

Related Questions