Asynchronous dispatch queue

Question

Background:

This is supposed to be the sole worker thread to carry out long-lasting jobs in a GUI application. The GUI thread should be able to schedule tasks in a non-blocking manner and the tasks should que-up until the thread gets around executing them. I tried my best to make it exception safe.

Code:

#include <condition_variable>
#include <functional>
#include <mutex>
#include <queue>
#include <thread>
#include <utility>

class WorkerThread final {
 public:
  using Task = std::function<void(void)>;

 private:
  /* this mutex must be locked before
   * modifying state of this class */
  std::mutex _mutex;

  /* list of tasks to be executed */
  std::queue<Task> _toDo;

  /* The thread waits for this signal when
   * there are no tasks to be executed.
   * `notify_one` should be called to
   * wake up the thread and have it go
   * through the tasks. */
  std::condition_variable _signal;

  /* This flag is checked by the thread
   * before going to sleep. If it's set,
   * thread exits the event loop and terminates. */
  bool _stop = false;

  /* the thread is constructed at the
   * end so everything is ready by
   * the time it executes. */
  std::thread _thread;

 private:
  /* entry point for the thread */
  void ThreadMain() noexcept {
    /* Main event loop. */
    while (true) {
      /* not locked yet */
      std::unique_lock lock{_mutex, std::defer_lock_t{}};  // noexcept

      /* Consume all tasks */
      while (true) {
        /* locked while we see if
         * there are any tasks left */
        lock.lock();  // won't throw

        if (_toDo.empty()) {  // noexcept
          // Finished tasks. Mutex stays locked
          break;
        }

        // Pop the front task
        // move shouldn't throw
        auto const task = std::move(_toDo.front());
        _toDo.pop();

        // Allow other tasks to
        // be added while we're executing one
        lock.unlock();  // won't throw

        try {
          // execute task
          task();  // May throw. Will be caught.
        } catch (...) {
          // log if throws
        }
      }

      // queue is empty (and mutex is still locked)

      /* if `_stop` is set, unlock
       * mutex (in lock destructor)
       * and stop the thread */
      if (_stop) return;

      // wait for further notice (and unlock the mutex)
      _signal.wait(lock);  // won't throw
    }
  }

 public:
  template <class Func>
  void Schedule(Func&& func) {
    // lock the mutex so we can add a new task
    std::lock_guard<std::mutex> guard{_mutex};

    // push the task
    // May throw. RAII lock will be unlocked. State is valid
    _toDo.push(std::forward<Func>(func));

    // notify the worker thread in case it's sleeping
    _signal.notify_one();
  }

  WorkerThread() : _thread(&WorkerThread::ThreadMain, this) {}

  ~WorkerThread() {
    std::unique_lock lock{_mutex};  // won't throw
    // tell the thread to finish up
    _stop = true;

    // wake up the thread in case it's sleeping
    _signal.notify_one();  // noexcept

    lock.unlock();  // won't throw

    // wait for the thread to finish up
    _thread.join();  // won't throw since ThreadMain is noexcept
  }

  WorkerThread(WorkerThread const&) = delete;
  WorkerThread& operator=(WorkerThread const&) = delete;
  WorkerThread(WorkerThread&&) = delete;
  WorkerThread& operator=(WorkerThread&&) = delete;
};

// Example driver code

#include <chrono>
#include <iostream>

int main() {
  using namespace std::chrono_literals;

  int constexpr sz = 100;

  int vars[sz];

  {
    WorkerThread thread;
    for (int i = 0; i < sz; ++i) {
      thread.Schedule([&vars, i] {
        std::this_thread::sleep_for(1ms);
        vars[i] = i;
      });
    }
  }

  for (auto const var : vars) std::cout << var << '\n';
}

The parts marked with // won't throw are parts I believe won't possibly throw even though aren't marked with noexcept.

Answer 1

Remove redundant comments

You added a lot of comments to the code, but many of them are not very useful. Comments should be used to explain what the code does if this is not clear from reading the code itself. But for example:

std::queue<Task> _toDo;

I can see just from this line of code that this is a queue of tasks to be done, so the comment you wrote doesn't add any new information to this. Adding unnecessary comments just increases the noise to signal ratio, and actually makes the code less readable.

Avoid starting names with underscores

The C++ standard reserves some names starting with underscores. Unless you want to learn the exact rules, I recommend you don't start any names with an underscore, but instead use the prefix m_ or a single underscore as a suffix.

Avoid manually locking and unlocking mutexes

I recommed you just use a lock guard without std::defer_lock_t to lock those regions of code that need exclusive access to the data structures. So in MainThread(), I would write:

while (true) {
    std::function<void(void)> task;

    {
        std::unique_lock lock{_mutex};
        _signal.wait_for(lock, []{ return _stop || !_toDo.empty(); });

        if (_stop && _toDo.empty())
            break;

        task = std::move(_toDo.front());
        _toDo.pop();        
    }

    task();
}

Notify without the lock held

While your code is correct, it is slightly more efficient to call notify_one() if you don't hold the lock. So for example, in the destructor you can write:

~WorkerThread() {
    {
        std::unique_lock lock{_mutex};
        _stop = true;
    }

    _signal.notify_one();
    _thread.join();
}