CODEWITHSUNDEEP
c++multithreading
MrEmper
MrEmper

Reputation: 235

My threading function doesn't stop the thread I want

I'm trying to make a stopwatch class, which has an seperate thread that counts down. The issue here is that when I use the cancel function or try setting up the stopwatch again after a timeout, my program crashes. At the moment I am sure it's due some threading issues, probably because of misuse. Is there anyone that can tell me why this doesn't work and can help me get it working? 

 Stopwatch.h

 class Stopwatch
    {
    public:
        void Run(uint64_t ticks);
        void Set(uint64_t ms);
        void Cancel();

    private:
        std::thread mythread;
    };



Stopwatch.cpp

void Stopwatch::Run(uint64_t ticks)
{
  uint64_t clockCycles = ticks;

  while(clockCycles > 0){
    std::this_thread::sleep_for(std::chrono::milliseconds(1));
    clockCycles--;
  }

  //do anything timeout related, probab a cout in the future
}

void Stopwatch::Set(uint64_t ms)
{
    mythread = std::thread(&Timer::Run, this, ms);
}

void Stopwatch::Cancel()
{
    mythread.join();
}

What I want is to call the stopwatch to set a time and get some timeout reaction. With the cancel function is can be stopped at any time. After that with the set function you can restart it.

Upvotes: 0

Views: 303

Answers (1)

Jeff Garrett
Jeff Garrett

Reputation: 7383

As noted in the comments, you must always eventually call join or detach for a std::thread which is joinable (i.e., has or had an associated running thread). The code fails to do this in three places. In Set, one uses std::thread's move-assignment to assign to the thread without regard to its previous contents. Calling Set multiple times without calling Cancel therefore is guaranteed to terminate. There is also no such call in the move-assignment operator or destructor of Stopwatch.

Second, because Cancel just calls join, it will wait for the timeout to happen and execute before returning, instead of canceling the timer. To cancel the timer, one must notify the thread. On the other end, the Run loop in the thread must have a way to be notified. The traditional way to do this is with a condition_variable.

For example:

class Stopwatch
{
public:
    Stopwatch() = default;

    Stopwatch(const Stopwatch&) = delete;
    Stopwatch(Stopwatch&&) = delete;
    Stopwatch& operator=(const Stopwatch&) = delete;
    Stopwatch& operator=(Stopwatch&& other) = delete;

    ~Stopwatch()
    {
        Cancel();
    }

    void Run(uint64_t ticks)
    {
        std::unique_lock<std::mutex> lk{mutex_};
        cv_.wait_for(lk, std::chrono::milliseconds(ticks), [&] { return canceled_; });
        if (!canceled_)
            /* timeout code here */;
    }

    void Set(uint64_t ms)
    {
        Cancel();
        thread_ = std::thread(&Stopwatch::Run, this, ms);
    }

    void Cancel()
    {
        if (thread_.joinable())
        {
            {
                std::lock_guard<std::mutex> lk{mutex_};
                canceled_ = true;
            }
            cv_.notify_one();
            thread_.join();
        }
    }

private:
    bool canceled_ = false;
    std::condition_variable cv_;
    std::mutex mutex_;
    std::thread thread_;
};

Upvotes: 1

Related Questions