Always use clock now function ptr in TaskRunner
Currently, the TaskRunner relies on the STL's understanding of the
system clock for some things, like the wait predicate, and uses the
actual platform::Clock for some unit tests. This causes unpredictable
behavior in our build bots, as well as violating best practices.
This patch moves to using a platform::ClockNowFunctionPtr in the
TaskRunnerFactory, always calling that FunctionPtr to get time updates
in the TaskRunnerImpl, and always passing a fake clock in unit tests.
Change-Id: I054f176ff34ee1bbd841e0f6cffef01de6af1af8
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/1560312
Commit-Queue: Jordan Bayles <jophba@chromium.org>
Reviewed-by: Peter Thatcher <pthatcher@google.com>
Reviewed-by: Ryan Keane <rwkeane@google.com>
diff --git a/api/impl/task_runner_impl.cc b/api/impl/task_runner_impl.cc
index ad061c2..695fd2a 100644
--- a/api/impl/task_runner_impl.cc
+++ b/api/impl/task_runner_impl.cc
@@ -36,6 +36,7 @@
const bool was_running = is_running_.exchange(false);
if (was_running) {
+ OSP_DVLOG << "Requesting stop...";
std::lock_guard<std::mutex> lock(task_mutex_);
run_loop_wakeup_.notify_one();
}
@@ -75,6 +76,7 @@
}
for (Task& task : current_tasks) {
+ OSP_DVLOG << "Running " << current_tasks.size() << " current tasks...";
task();
}
}
@@ -92,46 +94,55 @@
// Getting the time can be expensive on some platforms, so only get it once.
const auto current_time = now_function_();
while (!delayed_tasks_.empty() &&
- (delayed_tasks_.top().runnable_after < current_time)) {
+ (delayed_tasks_.top().runnable_after <= current_time)) {
tasks_.push_back(std::move(delayed_tasks_.top().task));
delayed_tasks_.pop();
}
}
+bool TaskRunnerImpl::ShouldWakeUpRunLoop() {
+ if (!is_running_) {
+ return true;
+ }
+
+ if (!tasks_.empty()) {
+ return true;
+ }
+
+ return !delayed_tasks_.empty() &&
+ (delayed_tasks_.top().runnable_after <= now_function_());
+}
+
std::unique_lock<std::mutex> TaskRunnerImpl::WaitForWorkAndAcquireLock() {
// These checks are redundant, as they are a subset of predicates in the
// below wait predicate. However, this is more readable and a slight
// optimization, as we don't need to take a lock if we aren't running.
if (!is_running_) {
+ OSP_DVLOG << "TaskRunner not running. Returning empty lock.";
return {};
}
std::unique_lock<std::mutex> lock(task_mutex_);
if (!tasks_.empty()) {
+ OSP_DVLOG << "TaskRunner lock acquired.";
return lock;
}
// Pass a wait predicate to avoid lost or spurious wakeups.
- const auto wait_predicate = [this] {
- // Either we got woken up because we aren't running
- // (probably just to end the thread), or we are running and have tasks to
- // do or schedule.
- return !this->is_running_ || !this->tasks_.empty() ||
- !this->delayed_tasks_.empty();
- };
-
- // TODO(jophba): find a predicate method that is compatible with our
- // fake clock, for easier use with testing.
- // We don't have any work to do currently, but know we have some in the pipe.
+ const auto wait_predicate = [this] { return ShouldWakeUpRunLoop(); };
if (!delayed_tasks_.empty()) {
+ // We don't have any work to do currently, but have some in the
+ // pipe.
+ OSP_DVLOG << "TaskRunner waiting for lock until delayed task ready...";
run_loop_wakeup_.wait_until(lock, delayed_tasks_.top().runnable_after,
wait_predicate);
-
+ } else {
// We don't have any work queued.
- } else if (tasks_.empty()) {
+ OSP_DVLOG << "TaskRunner waiting for lock...";
run_loop_wakeup_.wait(lock, wait_predicate);
}
+ OSP_DVLOG << "TaskRunner lock acquired.";
return lock;
}
} // namespace platform