Handle immediate destruction of a std::thread wrapper class

Question

I am writing a OO wrapper for std::thread. A simplified version of the code is shown below. The issue with this class is that if it is destroyed immediately it can throw an error because doWork is called from the thread at the same time as the class is being destroyed (pure virtual method called).

The test case is shown at the bottom.

How can I make this class more safe? A more complicated example would be even worse if MyConThread had member variables that were being used from MyConThread::doWork.

I realize that I have a similar issue on startup where doWork can be called before the derived class is constructed.

#include <thread>

class ConThread {
public:
    ConThread ()
    :t_ (doWorkInternal, this)
    {}
    ~ConThread ()
    {
        if (t_.joinable()) {
            t_.join();//avoid a crash because std::thread will terminate the app if the thread is still running in it's destructor
        }
    }

    std::thread& get () {return t_;};
    protected:
    virtual void doWork ()=0;
private:
    static void doWorkInternal (ConThread* t)
    {
        try {
            t->doWork ();
        } catch (...)
        {};

    }
    std::thread t_;
};

The issue I am running into is with the test case below:

class MyConThread: public ConThread
{
public:
    long i=0;
protected:    
    void doWork () override
    {
        for (long j=0; j<1000000_ && requestedToTerminate_==false; j++)
        {
            ++i;
        }
    }
};
TEST(MyConThreadTest, TestThatCanBeDestroyed)
{
    MyConThread mct (); //<== crashes when being destroyed because thread calls t->doWork ()
}

This isn't a matter of "OO": you're just writing a wrapper that (tries to) present a different API for the underlying functionality. — , May 20 '17 at 20:41
And, incidentally, I think virtual functions are a far less convenient and useful than the `std::function` mechanism `std::thread` uses. — , May 20 '17 at 20:43
Note: there's a reason that std::thread does not join like this: deadlock bait. — Billy ONeal, May 20 '17 at 20:55
@Hurkyl: std::thread does not use std::function (though the interface is similar). There's no virtual dispatch or memory allocation going on here. — Billy ONeal, May 20 '17 at 20:56

score 3 · Answer 1 · edited May 23 '17 at 12:10

First of all, your program crashes regardless of whether thread object is being destroyed or not. It's very easy to check, just insert some delay after the creation of an object:

using namespace std::chrono_literals;

TEST(MyConThreadTest, TestThatCanBeDestroyed)
{
    MyConThread mct ();
    std::this_thread::sleep_for(100s);
}

The crash happens because you're calling a virtual method from the constructor, which is generally a very bad idea. Basically, in C++ objects are created in order from base to derived and when you call your pure virtual method in ctor the overload cannot be handled yet (because derived is not yet constructed). See this answer as well.

So, first rule: don't ever call virtual methods (no matter pure or defined) from constructor or destructor.

I think the easiest way to fix this here would be to add start method which actually starts the thread. Like this:

ConThread()
{
}

void start()
{
    t_ = std::thread(doWorkInternal, this);
}

Generally, I don't like the idea of mixing logic and thread objects together, because by doing this you're violating Single Responsibility Principle. Your object does two things - it is a thread, and it also has a piece of your own logic. It is usually better to treat these separately, that's why std::thread provides means to "pass" logic into it via construction and it is not designed for being used as a base class. I found a good article about this, it's about Qt threads rather than std threads, but concepts are the same.

What I usually do in my code (which is also not ideal, but cleaner):

std::thread readerThread([]
{
    DatasetReader reader;
    reader.init();
    reader.run();
});
std::thread mesherThread([]
{
    Mesher mesher;
    mesher.init();
    mesher.run();
});
readerThread.join();
mesherThread.join();

If you want to join your thread automatically in dtor, just create a wrapper around std::thread, but keep the interface for passing logic into it (like lambda, or function pointer and parameters, etc.)

norisknofun · Answer 2 · 2017-05-20T21:45:45.017

You have 2 problem : 1. you are tricking the compiler to call an non-exiting func 2. the constructor is left before ensuring the thread really starts.

For 1: use template. Pass a runner class needing simple void run() For 2: use a bool to ensure thread started. you can even give it to your runner: void run(bool * started);

Result (version without state bool injected to runner) :

    template < class runner_class>
    class ConThread {
    public:
        ConThread() // respect order of init
            :started_(false)
            , runner_()
            , t_([this] 
        {
            started_ = true;
            runner_.run(); 
        })
        {
            while (!started_); // wait thread is REALLY started ...
        }
        ~ConThread()
        {
            if (t_.joinable()) {
                t_.join();
            }
        }

        std::thread& get() { return t_; };
    private: // beware: order of declaration is important here
        std::atomic_bool started_;
        runner_class runner_;
        std::thread t_;
    };

score 0 · Answer 3 · answered May 20 '17 at 21:08

In C++ a base class cannot run code after a derived class has been set up automatically; there is no "post construct" call by default.

As the derived class's work function is not valid until the derived class is fully constructed, this means the base class cannot schedule it to be run unless the derived class explicitly states when it is ready and completwly constructed.

They way std got around this with std thread was by injecting the desired behaviour as an argument to thread's constructor. Now the bahaviour is fully construxted by the time thread is, so thread is free to schedule it to run.

This means we are not using inheritance, at least not the default built in C++ virtual function table based inheritance. However, not using the language-provided OO does not mean your code is not OO.

We could shoe horn this into language provided OO in a few ways; require you both inherit from your thread interface, and have a template derived helper under your type that kicks things off.

But maybe better is following C++ std patterns and splitting the executed object from the executing object is a far better idea. Have the concept of an executable object (which can be as simple as "has an operator()", or more complex), and a threading abstraction that consumes these executable objects. Both problems are sufficiently complex that your code may be cleaner.

score 0 · Answer 4 · answered May 22 '17 at 20:31

Thanks for all the feedback. This is what I ended up doing.

//
// Created by pbeerken on 5/18/17.
//

#ifndef LIBCONNECT_CONTHREAD_H
#define LIBCONNECT_CONTHREAD_H

#include <thread>
#include <future>

class ConRunnable2
{
public:
    virtual void doWork ()=0;
    void requestToTerminate () {requestedToTerminate_=true;};
protected:
   bool requestedToTerminate_=false;
};

template <class ClassToRun>
class ConThread2
{
public:
    //constructor forwards arguments to the ClassToRun constructor
    template <typename ... Arguments > ConThread2 (Arguments ... args)
            :toRun_ (args ...)
             , t_ ([this] {
                started_.set_value();
                toRun_.doWork();
            })
    {
        started_.get_future().wait(); //wait till the thread is really started
    }

    ~ConThread2()
    {
        toRun_.requestToTerminate ();
        if (t_.joinable ()){
            t_.join ();
        }
    }
    void requestToTerminate () {toRun_.requestToTerminate ();};

    std::thread& getThread () {return t_;};

    ClassToRun& get () {return toRun_;};
private:
    std::promise <void> started_;
    ClassToRun toRun_;
    std::thread t_;
};

#endif //LIBCONNECT_CONTHREAD_H

Which passes this test:

#include <iostream>

#include <gtest/gtest.h>
#include "../ConThread.h"
#include <chrono>
#include <future>


class MyClassToBeRun: public ConRunnable2
{
public:
    MyClassToBeRun (int loopSize)
            :loopSize_ (loopSize)
    {};

    void doWork () override
    {
        for (long j=0; j<loopSize_ && requestedToTerminate_==false; j++)
        {
            ++i_;
        }

        p_.set_value();
    }
    long i_=0;
    long loopSize_=0;

    std::promise <void> p_;
};


TEST(MyConThread2Test, TestThatItRunsInASeparateThread)
{
    ConThread2<MyClassToBeRun> ct (10000);
    ct.get ().p_.get_future ().wait ();
    EXPECT_EQ (10000,ct.get ().i_);
}

TEST(MyConThread2Test, TestThatCanBeDestroyed)
{
    ConThread2<MyClassToBeRun> ct (1'000'000'000);
}

Handle immediate destruction of a std::thread wrapper class

4 Answers4