14

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’|
ildjarn
  • 62,044
  • 9
  • 127
  • 211
Mooks
  • 141
  • 1
  • 1
  • 3

2 Answers2

17

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++
ildjarn
  • 62,044
  • 9
  • 127
  • 211
  • 3
    I don't think that the `m_stop` variable needs to be `volatile`, `std::atomic` will take care of the multithreading issues, and `volatile` is of no extra help there. – David Rodríguez - dribeas May 10 '11 at 22:19
  • @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) – ildjarn May 13 '11 at 01:00
  • 2
    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 i = 0; for ( ; i < 10; i = i + 1 ) {}` can be transformed into `std::atomic 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. – David Rodríguez - dribeas May 13 '11 at 07:45
  • 3
    -1: `std::atomic` (lockfree) and `volatile`(hardware) are orthogonal concepts. See my question [Why is the volatile qualifier used through out std::atomic?](http://stackoverflow.com/q/2479067/28817), 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. – deft_code Nov 19 '11 at 00:26
  • 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? – Ricardo Apr 30 '12 at 08:51
  • @Ricardo : I was assuming `boost::bind` semantics, where `*this` works as expected. You're right that it should just be `this` for `std::thread<>`. – ildjarn Apr 30 '12 at 16:09
  • 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.) – Jim Hunziker Oct 08 '14 at 18:17
  • @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. :-] – ildjarn Oct 09 '14 at 06:44
  • Why in the constructor of the Runnable class m_thread() should be called? Is it necessary? – TonySalimi Feb 05 '19 at 11:50
  • @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. – ildjarn Feb 05 '19 at 12:10
16

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.

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
  • 1
    And that is why there needs a `start()` to be a function that does the actual starting work. – Etienne de Martel May 10 '11 at 21:41
  • 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)? – Cubbi May 10 '11 at 21:48
  • 1
    @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 May 10 '11 at 21:50
  • @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. – David Rodríguez - dribeas May 10 '11 at 21:50
  • Ah, right. I'm just so used to reference_wrapping everything passed to threads that can't be copied. – Cubbi May 10 '11 at 21:56