std::thread creation with class member function - best practice

Question

Let's say we have some Class SomeAlgorithm, that has been initialized with a bunch of data and other variables. We now want to use its member function execute to start computing. Let's create a std::threadand join it with the main thread.

#include <thread>
#include <memory>

class SomeAlgorithm {
public:
    void execute();
private:
    // variables, eg. std::vector<int> data
};

int main() {
    // (1)
    {
        SomeAlgorithm a1;
        std::thread t1(&SomeAlgorithm::execute, &a1);
        t1.join();
    }
    // (2)
    {
        SomeAlgorithm* a2 = new SomeAlgorithm();
        std::thread t2(&SomeAlgorithm::execute, a2);
        t2.join();
        delete a2;
    }
    // (3)
    {
        std::unique_ptr<SomeAlgorithm> a3(new SomeAlgorithm());
        std::thread t3(&SomeAlgorithm::execute, a3.get());
        t3.join();
    }
}

What is the preferred way to create such a thread? Are there any best practices? And what is the savest way in terms of memory use that ensures that the allocated memory gets freed?

`SomeAlgorithm a1();` isn't what you think it is, so I'd start by fixing that. And `a1.~SomeAlgorithm();` is a recipe for disaster. Assuming you fixed the first problem, do you think that's the only time the destructor for `a1` will fire? Then, it's a matter of opinion (with the fast majority being anything *but* manually managing memory via operators `new` and `delete`. The "safest" way to memory management is to not actually do it yourself; Use automatics and orchestrate your code around managed lifetime to manage resources. — WhozCraig, Nov 30 '19 at 12:09
i fixed it, sorry just wrote the code as example, didnt test it — Nic, Nov 30 '19 at 12:13
I wouldn't explicitly call a destructor where just limiting the scope is enough. — Kit., Nov 30 '19 at 12:13
@Kit. outside of placement-new management, I cannot think of a time I would *ever* manually fire a destructor. And I avoid placement-new scenarios like the plague, so that problem somewhat solves itself =) — WhozCraig, Nov 30 '19 at 12:16
(1) is preferred (3) isn't valid anyway, but even if fixed, I prefer (1) above anything else. Of course that's imho, and like a lot of things, everyones got one of those. — WhozCraig, Nov 30 '19 at 12:23
@WhozCraig thank you for your opinion! I did not test the code, how do i need to fix example (3) to make it work? Also, why do you prefer (1)? — Nic, Nov 30 '19 at 12:31
`std::thread t3(&SomeAlgorithm::execute, a3.get());` - I prefer (1) because it is by far the simplest. It isn't going to always be appropriate (if the type were *huge* and therefore not appropriate for automatic storage, a smart pointer scenario could be viable like (3)). Unrelated, `a3.reset()` is superfluous; it's going to happen anyway as soon as that scope closes. Removing that, you'll see just how similar (1) and (3) really are; only the locale of existence differentiate the two (automatic vs. automatic-managed dynamic). — WhozCraig, Nov 30 '19 at 12:35
@Nic I've updated my answer with some important info about `unique_ptr`. Please check it out. `a3.get()` is a bug. — bolov, Nov 30 '19 at 14:36

score 1 · Answer 1 · answered Nov 30 '19 at 12:53

Normally (at least for me) it's (1) in the form:

{
    SomeAlgorithm a1;
    std::thread t1([&a1]() { a1.execute(); });
    t1.join();
}

(3) is only used if you really want to disown the object from your main thread. Such as, for example (from C++14 on):

{
    auto a3 = std::make_unique<SomeAlgorithm>();
    std::thread t3([a{move(a3)}] { a->execute(); });
    t3.join();
}

It may show your intent more clearly (if you don't care about the actual results in the main thread), but it is less efficient, mostly because of cross-thread heap allocation/deallocation.

(2) is just a deprecated (since C++11) way of doing (3).

bolov · Accepted Answer · 2019-11-30T14:31:57.680

Never use (2). This has nothing to do with threads. Never use explicit new. Smart pointers if you need owning pointers.

As for (3) in your example you don't need pointers. You would need if the thread outlived the scope of the object, e.g.:

std::thread t3;
{
    auto a3 = std::make_unique<SomeAlgorithm>();
    t3 = std::thread{&SomeAlgorithm::execute, std::move(a3)};
}

t3.join();

Also there are some issues with your code. Use make_unique. There are some subtle bugs that can creep without it. As I've said, never use explicit new. But most importantly, don't use get() as it nullifies the whole point of the smart pointer.

As for (1) I would personally prefer:

// (1)
{
    SomeAlgorithm a1;
    std::thread t1{[&]{ a1.execute(); }};
    t1.join();
}

`get()` is totally fine if you have to pass the pointer down a call chain, while the smart pointer is still alive. Otherwise you either have to move it (with `unique_ptr`) or you have time to inc/dec the ref count constantly (`shared_ptr`). See also [my question about passing shared_ptr to a function](https://stackoverflow.com/questions/37610494/passing-const-shared-ptrt-versus-just-shared-ptrt-as-parameter). — Mike Lischke, Nov 30 '19 at 14:41

std::thread creation with class member function - best practice

2 Answers2