C++11: std::thread inside a class executing a function member with thread initialisation in the constructor

Question

I'm trying to use std::thread from C++11. I couldn't find anywhere if it is possible to have a std::thread inside a class executing one of its function members. Consider the example below... In my try (below), the function is run().

I compile with gcc-4.4 with -std=c++0x flag.

#ifndef RUNNABLE_H
#define RUNNABLE_H

#include <thread>

class Runnable
{
    public:
        Runnable() : m_stop(false) {m_thread = std::thread(Runnable::run,this); }
        virtual ~Runnable() { stop(); }
        void stop() { m_stop = false; m_thread.join(); }
    protected:
        virtual void run() = 0;
        bool m_stop;
    private:
        std::thread m_thread;
};


class myThread : public Runnable{
protected:
    void run() { while(!m_stop){ /* do something... */ }; }
};

#endif // RUNNABLE_H

I'm getting this error and others: (same error with and without the $this)

Runnable.h|9|error: no matching function for call to ‘std::thread::thread(<unresolved overloaded function type>, Runnable* const)’|

When passing a pointer.

Runnable.h|9|error: ISO C++ forbids taking the address of an unqualified or parenthesized non-static member function to form a pointer to member function.  Say ‘&Runnable::run’|

Solution 1

Here's some code to mull over:

#ifndef RUNNABLE_H
#define RUNNABLE_H

#include <atomic>
#include <thread>

class Runnable
{
public:
    Runnable() : m_stop(), m_thread() { }
    virtual ~Runnable() { try { stop(); } catch(...) { /*??*/ } }

    Runnable(Runnable const&) = delete;
    Runnable& operator =(Runnable const&) = delete;

    void stop() { m_stop = true; m_thread.join(); }
    void start() { m_thread = std::thread(&Runnable::run, this); }

protected:
    virtual void run() = 0;
    std::atomic<bool> m_stop;

private:
    std::thread m_thread;
};


class myThread : public Runnable
{
protected:
    void run() { while (!m_stop) { /* do something... */ }; }
};

#endif // RUNNABLE_H

---

Some notes:

• Declaring m_stop as a simple bool as you were is horribly insufficient; read up on memory barriers
• std::thread::join can throw so calling it without a try..catch from a destructor is reckless
• std::thread and std::atomic<> are non-copyable, so Runnable should be marked as such, if for no other reason than to avoid C4512 warnings with VC++

Solution 2

That approach is wrong.

The problem is that while the object is still under construction its type is still not the most derived type, but the type of the constructor that is executing. That means that when you start the thread the object is still a Runnable and the call to run() can be dispatched to Runnable::run(), which is pure virtual, and that in turn will cause undefined behavior.

Even worse, you might run into a false sense of security, as it might be the case that under some circumstances the thread that is being started might take long enough for the current thread to complete the Runnable constructor, and enter the myThread object, in which case the new thread will execute the correct method, but change the system where you execute the program (different number of cores, or the load of the system, or any other unrelated circumstance) and the program will crash in production.

Comments

[Etienne de Martel]

And that is why there needs a start() to be a function that does the actual starting work.

[Cubbi]

I don't think the thread taking long enough to get scheduled would change the behavior, doesn't its constructor make a copy (thus slicing) the result of bind(&Runnable::run, this)?

[David Rodríguez - dribeas]

@Etienne de Martel: There are different approaches, the start() method option is clearly one of them. Another is the approach taken by boost and c++0x: detach the runnable object from the thread object, that will take the operation to run as argument to the constructor. That ensures that the object that is going to be executed is fully constructed. That is, of course, if you don't force your way into a framework that breaks it...

[David Rodríguez - dribeas]

@Cubbi: this is a Runnable*, and it is copied --the pointer-- but the pointee (*this) is not copied. Basically you are passing a pointer to a not yet fully constructed object to the thread. The passing of the pointer is not a problem (the standard guarantees that you can pass the pointer) but dereferencing it is UB.

[Cubbi]

Ah, right. I'm just so used to reference_wrapping everything passed to threads that can't be copied.

[David Rodríguez - dribeas]

I don't think that the m_stop variable needs to be volatile, std::atomic<bool> will take care of the multithreading issues, and volatile is of no extra help there.

[ildjarn]

@David Rodríguez : §29.6.5/3 in the FDIS explicitly says otherwise. If the std::atomic<> object is not volatile, then accesses to that object may be reordered; there's nothing special about the std::atomic<> type that changes this fact. (re-pasting due to uneditable typo)

[David Rodríguez - dribeas]

I did not notice the typo in the previous comment, but I think that it was more precise than the new version. What the standard says in that paragraph is that operations on the atomic object can be merged (does not say reordered). My understanding of it is that std::atomic<int> i = 0; for ( ; i < 10; i = i + 1 ) {} can be transformed into std::atomic<int> i = 10; (this is not feasible with volatile variables, where all reads and writes are required in the final executable). I haven't had enough time to read (as in fully digest) the atomic sections of the standard, though.

[deft_code]

-1: std::atomic (lockfree) and volatile(hardware) are orthogonal concepts. See my question Why is the volatile qualifier used through out std::atomic?, in particular @Herb Sutter's answer. Volatile is not needed here, std::atomic's semantics are sufficient to prevent harmful reordering. Everything else in the answer is fine however. I'm just in the middle of holy war against volatile-based-concurrency in my day job.

[Ricardo]

Why *this is passed in std::thread(&Runnable::run, *this) constructor? Shouldn't be just the this pointer? So the thread access the same object and bool attribute?

[ildjarn]

@Ricardo : I was assuming boost::bind semantics, where *this works as expected. You're right that it should just be this for std::thread<>.

[Jim Hunziker]

I think a problem with the suggested code is that the destructor of the derived class will be called before the destructor for Runnable. So if run() is using resources that were allocated in the derived class, they will disappear before Runnable's destructor tells the run() method to stop. (Perhaps stop() should be virtual or pure virtual.)

[ildjarn]

@JimHunziker : An excellent point, and in my mind also a great argument in favor of the approach that was taken for standardization (function + arguments in the constructor, no 'start' method) vs. the overly-OOPish approach of inheritance. I.e., my code is harder to reason about. :-]

[TonySalimi]

Why in the constructor of the Runnable class m_thread() should be called? Is it necessary?

[ildjarn]

@hsalimi : Not strictly, no, as std::thread's default constructor would still be called if omitted. It's just a matter of personal preference for me that if I have a member initializer list, I list every base and data member in it explicitly.