Is a condition variable's .wait_for() an efficient way to perform a background task at a specific interval?

Question

I need to run an activity every so often while my program is running. In production code this is configurable with a default of 30 minutes, but in the example below I've used 5 seconds. Previously I had a std::thread that would loop once per second checking to see if it was time to run the activity OR if the program was closed. This allowed me to close the program at any time without having the .join() on the activity's thread block my application's exit waiting for its next iteration. At any moment it was less than a second away from checking to see if it should close or perform the activity.

I do not like the idea of wasting time checking every second for an activity that may only occur every 30 minutes while the program is running, so I attempted to switch it to a condition variable. I've included a small example of my implementation below. I want to be sure I'm using the right tools to do this. The issue I see with my code is unnecessary calls of the lambda expression which I'll explain below.

#include <iostream>
#include <thread>
#include <mutex>
#include <condition_variable>

bool asking_thread_to_quit;
std::mutex cv_mutex;
std::condition_variable cv;

void RunThread()
{
    {
        std::lock_guard<std::mutex> lock(cv_mutex);
        asking_thread_to_quit = false;
    }

    std::cout << "Started RunThread." << std::endl;
    while(true)
    {
        {
            std::unique_lock<std::mutex> lock(cv_mutex);
            std::chrono::seconds delay(5);
            if(cv.wait_for(lock, delay, [] { std::cout << "WAKEUP" << std::endl; return asking_thread_to_quit; })) // timed out
            {
                std::cout << "Breaking RunThread Loop." << std::endl;
                break;
            }
        }

        std::cout << "TIMER CODE!" << std::endl;
    }
}

int main(int argc, char *argv[])
{
    std::cout << "Program Started" << std::endl;
    std::thread run_thread(RunThread);

    // This is where the rest of the program would be implemented, but for the sake of this example, simply wait for user input to allow the thread to run in the background:
    char test;
    std::cin >> test;

    {
        std::lock_guard<std::mutex> lock(cv_mutex);
        asking_thread_to_quit = true;
    }
    cv.notify_all();

    std::cout << "Joining RunThread..." << std::endl;
    run_thread.join();
    std::cout << "RunThread Joined." << std::endl;

    return 0;
}

If you execute the program and allow for one 5-second iteration to pass, it gives the following output:

Program Started
Started RunThread.
WAKEUP
WAKEUP
TIMER CODE!
WAKEUP
q    <-- I typed this to quit.
Joining RunThread...
WAKEUP
Breaking RunThread Loop.
RunThread Joined.

You can see that it does the following:

1. (WAKEUP) Performs the check prior to waiting

2. Wait for five seconds

3. (WAKEUP) Performs the check

4. (TIMER CODE!) Executes the activity
5. (WAKEUP) Performs the check again before going back to waiting

Step 5 seems unnecessary as I just performed it a split second ago, but I believe it is necessary as .wait_for() doesn't know I'm using it inside of a while(true) loop. Is this something I'm stuck with, or is there a way to remove the initial check in the .wait_for() call? I'm guessing there is not as it would allow for the system to .wait_for() something that it doesn't need to wait for. This is what leads me to wonder if I'm using the right language features to begin with. Is there a better way?

The Answer

The answer given below goes into detail on other issues with my code as well as sparked an informative related conversation. I'm going to accept that answer as it helped me the most, but the quick answer to the question seems to be this:

asking_thread_to_quit could have been set to true during the TIMER CODE! section, requiring another check prior to waiting on the condition variable again.

Answer 1

Your code has a few issues with it.

void RunThread()
{
  asking_thread_to_quit = false;

This is a race condition. You shouldn't modify a non-atomic shared variable in two different threads without synchronization.

  std::cout << "Started RunThread." << std::endl;
  while(true)
  {
    std::unique_lock<std::mutex> lock(cv_mutex);
    std::chrono::seconds delay(5);

First using namespace std::literals::chrono_literals;. Then use 5s.

    if(cv.wait_for(lock, delay, [] { std::cout << "WAKEUP" << std::endl; return asking_thread_to_quit; })) // timed out
    {
      std::cout << "Breaking RunThread Loop." << std::endl;
      break;
    }
    else
    {
      std::cout << "TIMER CODE!" << std::endl;
    }

the TIMER CODE usually shouldn't run within the std::mutex lock, as that means anyone sending a message is blocked until the timer code is finished.

  }
}

Finally, WAKEUPs are spurious details. You could WAKEUP 50 times in that 5 seconds; condition variables do not guarantee a bounded number of checks.

asking_thread_to_quit = true;
cv.notify_all();

this again results in a race condition; your program does undefined behavior twice over now.

Changing asking_thread_to_quit to a std::atomic<bool> will get rid of the formal race condition and UB. It will, however, let your code miss a request to quit and mistakenly do another 5 second sleep followed by the task.

This is because the return value of your lambda could be calculated, then the asking_thread_to_quit=true and notify_all evaluates with nothing waiting on the condition variable (so nothing is woken up), then the condition variable is blocked on, 5 seconds pass, it wakes up returning false, then repeats the while loop.

With the mutex being held in all writes to the bool, the write cannot occur until after the lambda has returned and we are waiting on the condition with an unlocked mutex. This prevents the .notify_all() from being missed.

The cargo-cult solution to this is to always guard all reads and writes to asking_thread_to_quit by the cv_mutex. Then avoid holding the cv_mutex for any length of time, including while handling the timer wakeup.

std::unique_lock<std::mutex> lock_cv() {
  return std::unique_lock<std::mutex>(cv_mutex);
}
void RunThread()
{
  {
    auto lock = lock_cv();
    asking_thread_to_quit = false;
  }

  std::cout << "Started RunThread." << std::endl;
  while(true)
  {
    {
      auto lock = lock_cv();
      using namespace std::literals::chrono_literals;
      if(cv.wait_for(lock, 5s, [] { std::cout << "WAKEUP" << std::endl; return asking_thread_to_quit; })) // timed out
      {
        std::cout << "Breaking RunThread Loop." << std::endl;
        break;
      }
    }         
    std::cout << "TIMER CODE!" << std::endl;
  }
}

and in main:

{
  auto lock = lock_cv();
  asking_thread_to_quit = true;
}
cv.notify_all();

And yes, I intended for cv.notify_all() to be outside the mutex. It works; understanding why is outside the scope of the "cargo-cult" solution I'm providing here.

Finally, the WAKEUP is not spurious. The asking_thread_to_quit could have changed since the last time it was checked. Running the lambda guarantees we should fall asleep in a careful manner, with no gap between unlocking the mutex for waiting and waiting for notifications.

Spurious WAKEUPs can still occur; they would show up as more WAKEUPs than you expect.