Stop class method running in thread

Question

I have created a C++ class named Timer, it has 3 methods:

start()
stop()
print()

The start() method, enables a flag named run, setting its value to true.

The stop() method, disable the flag, setting its value to false.

The print() method, executes a while() under condition if run == true, and print some text sleeping a half second.

Timer.hpp

#ifndef TIMER
#define TIMER 1

#include <iostream>
#include <cstdbool>

#include <unistd.h>

class Timer{
private:
    bool run;

public:
    void start();
    void stop();
    void print();

};

#endif

Timer.cpp

#include "Timer.hpp"

void Timer::start(){
    this->run = true;
    this->print();
    return;
}

void Timer::stop(){
    this->run = false;
    return;
}

void Timer::print(){

    int counter = 0;

    while(this->run == true){

        std::cout << counter << std::endl;
        counter++;

        usleep(500000);
    }

    return;
}

main.cpp

#include <pthread.h>

#include "Timer.hpp"

void *handler(void *argument){

    ((Timer *) argument)->start();

    return argument;
}

int main(void){

    Timer *timer = new Timer();
    pthread_t timer_thread;
    int mainCounter = 0;

    pthread_create(&timer_thread, NULL, handler, (void *) &timer);

    while(true){

        if(mainCounter == 100){
            std::cout << "Stopping..." << std::endl;
            timer->stop();
        }

        std::cout << " => " << mainCounter << std::endl;
        mainCounter++;

        usleep(50000);
    }

    return 0;
}

My problem.

I've created a thread to handle the execution of method start(), after I've created a condition inside main thread in which after mainCounter get 100 iterations, it executes timer->stop(), but it doesn't stop the timer loop.

When mainCounter arrives to 100th iteration, it can't stop the loop inside thread.

The instruction to compile is:

g++ Timer.cpp -c 
g++ Timer.cpp main.cpp -o main -lpthread

The output is:

9
 => 90
 => 91
 => 92
 => 93
 => 94
 => 95
 => 96
 => 97
 => 98
 => 99
10
Stopping...
 => 100
 => 101
 => 102
 => 103
 => 104
 => 105
 => 106
 => 107
 => 108
 => 109
11

Try it online!

If you're going to read and write a variable (such as your `run` boolean variable) from multiple threads, you either need to guard all accesses to that variable with a mutex, or make the variable atomic using `std::atomic`. Otherwise you're invoking undefined behavior and your program may not do what you expect it to. (see: https://stackoverflow.com/questions/31978324/what-exactly-is-stdatomic ) — Jeremy Friesner, Apr 09 '19 at 17:07
Are you aware that you never exit the loop **in your main thread**? — r3mus n0x, Apr 09 '19 at 17:11
@r3musn0x I know i've never exit the main thread, the problem is that i want to stop the loop inside thread. — Ivan Botero, Apr 09 '19 at 17:15
@IvanBotero, it just seems from the output you've provided that it's the main thread that continues printing and the secondary thread might have already exited. — r3mus n0x, Apr 09 '19 at 17:16
Typo-ish-> `(void *) &timer)` timer's already a pointer. You're passing in a pointer to a pointer and using it (`((Timer *) argument)->start();`) as a mere pointer. Lose the `&` — user4581301, Apr 09 '19 at 17:19
You actually stopped worker thread. It's the main thread that continues to run, printing those values. — Timmy_A, Apr 09 '19 at 17:21
@Timmy_A worker thread probably was not stopped. `this` inside `Timer::print` isn't pointing to the same thing as `timer` — user4581301, Apr 09 '19 at 17:24
@user4581301 Sure. User has edited his output when I was writing my answer. — Timmy_A, Apr 09 '19 at 17:26
Try to mark run member field with the keyword volatile. But the real error @user4581301 has already described. Using pointer to pointer instead of pointer is the issue. In the worker thread you are modifying different memory, not one that belongs to 'timer' variable. So 'run' is never set to true. — Timmy_A, Apr 09 '19 at 17:29
Thanks for your comments, I've added an example over ideone.com. — Ivan Botero, Apr 09 '19 at 17:34

Gilles-Philippe Paillé · Accepted Answer · 2019-04-09T18:13:38.693

2

As mentionned by @user4581301, this will solve the problem:

pthread_create(&timer_thread, NULL, handler, (void *) &timer);

should be

pthread_create(&timer_thread, NULL, handler, (void *) timer);

The problem is that timer points to the allocated Timer class while &timer points somewhere on the stack. While running the thread, you are trying to access the run class member, but since this is pointing to the stack, you are actually reading an incorrect value.

Other notes: declare bool run as std::atomic_bool run, or use any mutex mechanism for thread-safety. Also, always delete your allocated variables: delete timer.

edited Apr 09 '19 at 18:13

answered Apr 09 '19 at 17:48

Gilles-Philippe Paillé

3,195
1
8
21

2

Better wording: *points somewhere on the stack* to *points to `timer` itself, and is already a pointer.* because we know exactly where it points. There is a slightly smarter alternative: Don't use dynamic allocation in the first place: `Timer timer;` Then you can `(void *) &timer`, `timer.stop();`, and not leak `timer` because it wasn't `delete`d. As for the atomic problem, I'm a hack so I'd probably use `volatile sig_atomic_t run;` – user4581301 Apr 09 '19 at 17:54

Stop class method running in thread

Try it online!

1 Answers1