pw_chrono_stl: simplify SystemTimer with recursive_mutex
Simplifies the STL's SystemTimer backend to use a single recurisve
mutex instead of two different mutexes along with an atomic which
appears to have a race condition likely related to the atomic.
Change-Id: I1c3d8740a143a840e1c38be310b581b77d79cb7f
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/52603
Reviewed-by: Wyatt Hepler <hepler@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Pigweed-Auto-Submit: Ewout van Bekkum <ewout@google.com>
diff --git a/pw_chrono_stl/public/pw_chrono_stl/system_timer_native.h b/pw_chrono_stl/public/pw_chrono_stl/system_timer_native.h
index 3bc3101..909f05e 100644
--- a/pw_chrono_stl/public/pw_chrono_stl/system_timer_native.h
+++ b/pw_chrono_stl/public/pw_chrono_stl/system_timer_native.h
@@ -13,7 +13,6 @@
// the License.
#pragma once
-#include <atomic>
#include <memory>
#include <mutex>
@@ -23,9 +22,6 @@
namespace pw::chrono::backend {
struct NativeSystemTimer {
- // The mutex is only used to ensure the public API is threadsafe.
- std::mutex api_mutex;
-
// Instead of using a more complex blocking timer cleanup, a shared_pointer is
// used so that the heap allocation is still valid for the detached threads
// even after the NativeSystemTimer has been destructed. Note this is shared
@@ -35,17 +31,18 @@
: callback(std::move(cb)) {}
const Function<void(SystemClock::time_point expired_deadline)> callback;
- // This mutex is used to ensure only one expiry callback is executed at a
- // time. This way there's no risk that a callback function attempting to
- // reschedule the timer is immediately preempted by that callback.
- //
- // Note this is required by the facade API contract.
- std::mutex callback_mutex;
+
+ // The mutex is used both to ensure the public API is threadsafe and to
+ // ensure that only one expiry callback is executed at time.
+ // A recurisve mutex is used as the timer callback must be able to invoke
+ // its own public API.
+ std::recursive_mutex mutex;
};
std::shared_ptr<CallbackContext> callback_context;
- // This is only shared with the last active timer if there is one.
- std::shared_ptr<std::atomic<bool>> active_timer_enabled;
+ // This is only shared with the last active timer if there is one. Note that
+ // this is guarded by the callback_context's mutex.
+ std::shared_ptr<bool> active_timer_enabled;
};
using NativeSystemTimerHandle = NativeSystemTimer&;
diff --git a/pw_chrono_stl/system_timer.cc b/pw_chrono_stl/system_timer.cc
index 112f5bd..5557042 100644
--- a/pw_chrono_stl/system_timer.cc
+++ b/pw_chrono_stl/system_timer.cc
@@ -22,31 +22,34 @@
using CallbackContext = backend::NativeSystemTimer::CallbackContext;
void SystemTimerThreadFunction(
- std::shared_ptr<std::atomic<bool>> timer_enabled,
+ std::shared_ptr<bool> timer_enabled,
std::shared_ptr<CallbackContext> callback_context,
SystemClock::time_point expiry_deadline) {
// Sleep until the requested deadline.
std::this_thread::sleep_until(expiry_deadline);
- // Only invoke the user's callback if this invocation has not been cancelled.
- if (timer_enabled->load()) {
- std::lock_guard lock(callback_context->callback_mutex);
- (callback_context->callback)(expiry_deadline);
+ {
+ std::lock_guard lock(callback_context->mutex);
+ // Only invoke the user's callback if this invocation has not been
+ // cancelled.
+ if (*timer_enabled) {
+ (callback_context->callback)(expiry_deadline);
+ }
}
}
} // namespace
void SystemTimer::InvokeAt(SystemClock::time_point timestamp) {
- std::lock_guard lock(native_type_.api_mutex);
+ std::lock_guard lock(native_type_.callback_context->mutex);
// First, cancel any outstanding requests.
if (native_type_.active_timer_enabled) {
- native_type_.active_timer_enabled->store(false);
+ *native_type_.active_timer_enabled = false;
}
// Second, active another detached timer thread with a new shared atomic bool.
- native_type_.active_timer_enabled = std::make_shared<std::atomic<bool>>(true);
+ native_type_.active_timer_enabled = std::make_shared<bool>(true);
std::thread(SystemTimerThreadFunction,
native_type_.active_timer_enabled,
native_type_.callback_context,
@@ -55,10 +58,10 @@
}
void SystemTimer::Cancel() {
- std::lock_guard lock(native_type_.api_mutex);
+ std::lock_guard lock(native_type_.callback_context->mutex);
if (native_type_.active_timer_enabled) {
- native_type_.active_timer_enabled->store(false);
+ *native_type_.active_timer_enabled = false;
}
}