What is the correct way of freeing std::thread* heap allocated memory?

Question

I'm declaring a pointer to a thread in my class.

class A{
   std::thread*    m_pThread;   
   bool StartThread();
   UINT DisableThread();
}

Here is how I call a function using a thread.

bool A::StartThread()
{
    bool mThreadSuccess = false;
    {
       try {
          m_pThread= new std::thread(&A::DisableThread, this);
          mThreadSuccess = true;
      }
      catch (...) {
         m_pDisable = false;
      }

      if(m_pThread)
      {
         m_pThread= nullptr;
      } 
   }
return mThreadSuccess;
}

Here is the function called by my thread spawned.

UINT A::DisableThread()
{
    //print something here.
    return 0;
}

If I call this StartThread() function 10 times. Will it have a memory leak?

for (i = 0; i<10; i++){
    bool sResult = StartThread();
      if (sResult) {
        m_pAcceptStarted = true;
      }
}

I don't understand the rationale behind even using a pointer here in the first place? Why not just do: `std::thread m_pThread;`? — Ruks, Sep 22 '21 at 10:45
The correct way would be to *not* use pointers or dynamic allocation. Use plain assignment to a plain `std::thread` object. — Some programmer dude, Sep 22 '21 at 10:47
@Ruks I'm handling legacy MFC code and I'm trying to migrate them to c++11/c++17. — DecryptDcode, Sep 22 '21 at 10:48

eerorika · Accepted Answer · 2021-09-22T11:11:50.490

3

What is the correct way of freeing
m_pThread= new std::thread(&A::DisableThread, this);

The correct way to free a non-array object created using allocating new is to use delete.

Avoid bare owning pointers and avoid unnecessary dynamic allocation. The example doesn't demonstrate any need for dynamic storage, and ideally you should use a std::thread member instead of a pointer.

If I call this StartThread() function 10 times. Will it have a memory leak?

Even a single call will result in a memory leak. The leak happens when you throw away the pointer value here:

m_pThread= nullptr;

could you add your better solution

Here's one:

auto future = std::async(std::launch::async, &A::DisableThread, this);
// do something while the other task executes in another thread
do_something();
// wait for the thread to finish and get the value returned by A::DisableThread
return future.get()

edited Sep 22 '21 at 11:11

answered Sep 22 '21 at 10:46

eerorika

232,697
12
197
326

Will this code `m_pThread= nullptr;` not enough? Where I should add `delete m_pThread` in my code? – DecryptDcode Sep 22 '21 at 10:50
@DecryptDcode As per my answer, that assignment is what causes the memory leak. You should delete when you no longer need the `std::thread` object. But more importantly, you probably shouldn't use dynamic allocation here in the first place. – eerorika Sep 22 '21 at 10:51
If i'm going to join the thread after exiting the for loop say, `if (m_pThread) { if (m_pThread->joinable()) { m_pThread->join(); } } delete m_pThread;` will it be fine? – DecryptDcode Sep 22 '21 at 10:58
@DecryptDcode It would work. But it would be better without the dynamic allocation. – eerorika Sep 22 '21 at 11:00
could you add your better solution without using dynamic allocation in your answer? So that I don't need to free the memory every time I respawn a thread. – DecryptDcode Sep 22 '21 at 11:03
You said every single call causes a memory leak. Does that mean that calling it 10 times will have 10 times memory leak? – DecryptDcode Sep 22 '21 at 11:50
@DecryptDcode Yes. – eerorika Sep 22 '21 at 12:02

score 1 · Answer 2 · answered Sep 22 '21 at 11:20

I'd personally would prefer using a threadpool in a real project but this example should give you an idea of how you could handle threads without new/delete.

#include <iostream>
#include <thread>
#include <vector>

class A
{
public:
    template<typename Fn>
    void CallAsync(Fn fn)
    {
        // put thread in vector
        m_threads.emplace_back(std::thread(fn));
    }

    ~A()
    {
        for (auto& thread : m_threads)
        {
            thread.join();
        }
    }

    void someHandler()
    {
        std::cout << "*";
    };

private:
    std::vector<std::thread> m_threads;
};

int main()
{
    A a;

    for (int i = 0; i < 10; ++i)
    {
        a.CallAsync([&a] { a.someHandler(); });
    }
}

What is the correct way of freeing std::thread* heap allocated memory?

2 Answers2