2
\$\begingroup\$

I have a class that (among others) needs to run an action at fixed intervals of time. So I created a thread for this:

struct Logger
{
   std::chrono::milliseconds flush_interval_;
   std::thread flush_thread_;

   Logger(...) : flush_thread_{&Logger::flush_thread_function, this}, ... {}

   auto flush_cb() -> void; // this is the action that need to be ran periodically

   auto flush_thread_function() -> void
   {
       while (true)
       {
           std::this_thread::sleep_for(flush_interval_);
           flush_cb();
       }
   }
};

The problem with this is of course stopping this thread when the object is destroyed. I came with a solution involving condition_variable that seems to work, but since I don't have much experience with multi-threading and I am just getting my head around condition_variable I want to make sure the solution is OK.

This is the part of the class that deals only with this:

class Logger
{
private:
    std::chrono::milliseconds flush_interval_;

    bool flush_stop_ = false;
    std::condition_variable flush_stop_cv_{};
    std::mutex flush_stop_mutex_{};

    std::thread flush_thread_;

public:

    Logger(std::chrono::milliseconds flush_interval = 1s)
        : flush_interval_{flush_interval},
          flush_thread_{&Logger::flush_thread_function, this}
    {
    }

    ~Logger()
    {
        {
            std::lock_guard<std::mutex> lock_guard{flush_stop_mutex_};
            flush_stop_ = true;
        }

        flush_stop_cv_.notify_all();
        flush_thread_.join();
    }
private:

    auto flush_cb() -> void;

    auto flush_thread_function() -> void
    {
        auto prev_flush = std::chrono::system_clock::now();

        while (true)
        {
            auto next_flush = prev_flush + flush_interval_;

            std::unique_lock<std::mutex> lock{flush_stop_mutex_};
            auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

            if (stop_reason == std::cv_status::timeout)
            {
                flush_cb();
                prev_flush = std::chrono::system_clock::now();
            }
            else if (flush_stop_)
            {
                // could call flush_cb() one last time here,
                // but I have another mechanism that deals with unflushed log entries
                break;
            }
            else
            {
                // this is a spurious wakeup
                // nothing to do here
            }
        }
    }
};

I am interested if my logic is solid and if the implementation is (thread-) safe. I chose wait_until and this overload in particular because the condition won't tell me if it was a timeout or a spurious wakeup. Another part that I am not entirely sure it's ok is the locking/unlocking of flush_stop_mutex_ in ~Logger() and the use of lock in flush_thread_function. As I understand after waking flush_stop_mutex_ is locked protecting flush_stop_ until the end of the current iteration of while, so I reason it is correct.

\$\endgroup\$
  • \$\begingroup\$ dealing with exceptions in ~Logger() is another matter. flush_stop_mutex_.lock() and flush_thread_.join() can throw and I don't know how to deal with those inside the destructor. Don't know if that would be on topic here, so I leave it just as a comment. \$\endgroup\$ – bolov Aug 6 '17 at 17:58
3
\$\begingroup\$

Your code is almost correct. Here's your code stripped down:

    while (true)
    {
        auto next_flush = prev_flush + flush_interval_;
        std::unique_lock<std::mutex> lock{flush_stop_mutex_};
        auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

        // stopping logic omitted
    }

Notice that the unique_lock is destroyed at the }, and so the mutex is not locked for a little while, until the next time you loop around and hit the std::unique_lock line. If another thread destroys the Logger during this period, it'll notify flush_stop_cv_ but you won't be listening and so you'll miss the notification... and that was your one chance to hear it, so now you'll never stop and your program is frozen.

Solution: If you're waiting for a notification that you absolutely must hear, then absolutely do not stop listening. Don't drop that lock!

    std::unique_lock<std::mutex> lock{flush_stop_mutex_};
    while (true)
    {
        auto next_flush = prev_flush + flush_interval_;
        auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

        // stopping logic omitted
    }

Or if you're really worried about holding the mutex for some reason, then check again after taking the lock but before going to sleep:

    while (true)
    {
        auto next_flush = prev_flush + flush_interval_;
        std::unique_lock<std::mutex> lock{flush_stop_mutex_};
        if (flush_stop_) break;  // IMPORTANT LINE ADDED
        auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

        // stopping logic omitted
    }

Otherwise, looks good to me.


condition_variable::wait_until is a fairly obscure function, IMO. Here you're assuming that it is implemented correctly and efficiently. I'm not sure that it always is, but I have no evidence to the contrary. The only immediately obvious way to get rid of wait_until is almost certainly worse: You could set another thread to notify the condition variable every flush_interval_ seconds, and detect spurious wakeups by computing std::chrono::system_clock::now() - prev_flush_, instead of by looking for std::cv_status::timeout.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.