std::thread : how to declare a thread in class body as a normal member?

Question

I want to have a thread for each instance of Page object. At a time only one of them can execute (simply checks if pointer to current running thread is joinable or not..)

class Page : public std::vector<Step>
{
    // ....
    void play();
    void start(); // check if no other thread is running. if there is a running thread, return. else join starter
    std::thread starter; // std::thread running this->play()
    static std::thread* current; // pointer to current running thread
    // ...
};

I want to be able to fire-up starter threads of Page objects. for example like this:

Page x , y , z;
// do some stuff for initialize pages..
x.start();
// do some other work
y.start(); // if x is finished, start y otherwise do nothing
// do some other lengthy work
z.start(); // if x and y are not running, start z

I can't manage to declare started as a member of Page. I found that it's because of the fact std::threads can only initialized at declaration time. (or something like that, cause it's not possible to copy a thread)

void x()
{
}
//...
std::thread t(x);          // this is ok
std::thread r;             // this is wrong, but I need this !
r = std::thread(this->y);  // no hope
r = std::thread(y);        // this is wrong too

@deft_code: I'm aware of problems with container inheritance. Considering the fact that lifetime of my 'Page' objects is bounded to lifetime of whole program, destructor of Page will never be called until everything is finished and even segmentation faults are not important. (I'm trying to dodge myself to avoid rewriting a couple of 3 weeks old 2500 lines of code, and YES, I'm the most sluggish student of cs ever...) — sorush-r, Feb 13 '12 at 20:03
What do you want `y` to do when `x` is still busy? It sounds like you want it to silently do nothing, but that doesn't seem right. Also what does `Page` do if `start` is called twice? — deft_code, Feb 13 '12 at 20:59
@deft_code: when there is at least one page running its starter thread, any other call to start() simply do nothing. If start was called twice only first call will start thread. if second call is called after first thread is finished, then second call will start thread (if there is not any other page playing...). — sorush-r, Feb 13 '12 at 21:19
By `Doing NOTHING` I mean returning immediately. Not waiting other thread to finish. just like a statement that has no effect. — sorush-r, Feb 13 '12 at 21:43
Why not have one thread in the background that `Page` shares instead of one per instance? — deft_code, Feb 13 '12 at 22:00

score 3 · Answer 1 · answered Feb 13 '12 at 19:34

3

You can initialize the thread to the function to run by using a member initializer list. For example, consider this constructor for Page:

class Page {
public:
    Page(); // For example

private:
    std::thread toRun;
};

Page::Page() : toRun(/* function to run */) {
    /* ... */
}

Notice how we use the initialization list inside the Page constructor to initialize toRun to the function that ought to be run. This way, toRun is initialized as if you had declared it as a local variable

std::string toRun(/* function to run */);

That said, there are two major problems I think that you must address in your code. First, you should not inherit from std::vector or any of the standard collections classes. Those classes don't have their destructors marked virtual, which means that you can easily invoke undefined behavior if you try to treat your Page as a std::vector. Instead, consider making Page hold a std::vector as a direct subobject. Also, you should not expose the std::thread member of the class. Data members should, as a general rule, be private to increase encapsulation, make it easier to modify the class in the future, and prevent people from breaking all of your class's invariants.

Hope this helps!

answered Feb 13 '12 at 19:34

templatetypedef

362,284
104
897
1,065

The problem is that I want `toRun` to run the member function of `Page` in other words : `Page::Page() : toRun(this->play)`. – sorush-r Feb 13 '12 at 19:38
1

@Sorush Rabiee- Sure, you can do that! Just initialize `toRun` to `[this]() {this->play();}` using the new C++11 lambda expressions. – templatetypedef Feb 13 '12 at 19:39
1

@SorushRabieee- Can you please elaborate on what's going wrong? Compile error? Linker error? Runtime error? – templatetypedef Feb 13 '12 at 20:04
sorry, last line: error: use of deleted function 'Page::Page(const Page&)' – sorush-r Feb 13 '12 at 20:09
@Sorush : `std::thread` is non-copyable (only movable), and since `Page` holds one by value `Page` itself is consequently also non-copyable (only movable). This is inherent in your design; if you don't like it, you need to rethink your design. – ildjarn Feb 13 '12 at 20:11
error: 'Page::Page(const Page&)' is implicitly deleted because the default definition would be ill-formed: page.h:16: error: use of deleted function 'std::thread::thread(const std::thread&)' – sorush-r Feb 13 '12 at 20:11
Sorry I'm completely confused! Cause there is not any assignment operator in `Page` :-/ – sorush-r Feb 13 '12 at 20:15
@SorushRabiee- I am assuming that this is because you are trying to copy a `Page` object somewhere. This will not work (as ildjarn has mentioned), because it's not meaningful to duplicate a thread. You may need to reevaluate your design and choose a different alternative. – templatetypedef Feb 13 '12 at 20:15
Oh... I'm trying to `push_back` a `Page` to a vector. Seems that there is no other way to avoid re-writing all of those not-so-well-written codes :( Thank you – sorush-r Feb 13 '12 at 20:20
@SorushRabiee- You could alternatively have a `vector` that holds `std::shared_ptr`s to the `Page`s, so that you don't need to make any copies. – templatetypedef Feb 13 '12 at 20:21
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/7642/discussion-between-sorush-rabiee-and-templatetypedef) – sorush-r Feb 13 '12 at 21:52

deft_code · Answer 2 · 2012-02-13T21:58:46.610

Never publicly inherit from a std container, unless the code is meant to be throw away code. An honestly it's terrifying how often throw away code becomes production code when push comes to shove.

I understand you don't want to reproduce the whole std::vector interface. That is tedious write, a pain to maintain, and honestly could create bugs.

Try this instead

class Page: private std::vector
{
public:
  using std::vector::push_back;
  using std::vector::size;
  // ...
};

Ignoring the std::vector issue this should work for the concurrency part of the problem.

class Page
{
  ~Page( void )
  {
    m_thread.join();
  }

  void start( void );

private:

  // note this is private, it must be to maintain the s_running invariant
  void play( void )
  {
    assert( s_current == this );

    // Only one Page at a time will execute this code.

    std::lock_guard<std::mutex> _{ s_mutex };
    s_running = nullptr;
  }

  std::thread m_thread;

  static Page* s_running;
  static std::mutex s_mutex;
};

Page* Page::s_running = nullptr;
std::mutex Page::s_mutex;
std::condition Page::s_condition;

void Page::start( void )
{
  std::lock_guard<std::mutex> _{ s_mutex };

  if( s_running == nullptr )
  {
    s_running = this;
    m_thread = std::thread{ [this](){ this->play(); } };
  }
}

This solution is may have initialization order issues if Page is instantiate before main()

std::thread : how to declare a thread in class body as a normal member?

2 Answers2

Linked