From bb529d03175fc988a9d7b593bf60e21b6803cbb7 Mon Sep 17 00:00:00 2001 From: Jingchen Date: Mon, 26 Jan 2026 21:39:28 +0000 Subject: [PATCH] fix: Stop embedded tests from hanging on ARM by using `atomic_flag` (#6248) This change replaces the mutex `stoppingMutex_`, the `atomic_bool` variable `isTimeToStop`, and the conditional variable `stoppingCondition_` with an `atomic_flag` variable. When `xrpld` is running the embedded tests as a child process, it has a control thread (the app bundle thread) that starts the application, and an application thread (the thread that executes `app_->run()`). Due to the relaxed memory ordering on ARM, it's not guaranteed that the application thread can see the change of the value resulting from the `isTimeToStop.exchange(true)` call before it is notified by `stoppingCondition_.notify_all()`, even though they do happen in the right order in the app bundle thread in `ApplicationImp::signalStop`. We therefore often get into the situation where `isTimeToStop` is `true`, but the application thread is waiting for `stoppingCondition_` to notify, because the app bundle thread may have already notified before the application thread actually starts waiting. Switching to a single `atomic_flag` variable makes sure that there's only one synchronisation object and then the memory order guarantee provided by c++ can make sure that `notify_all` gets synchronised after `test_and_set` does. Fixing this issue will stop the unit tests hanging forever and then we should see less (or hopefully no) time out errors in daily github action runs --- src/xrpld/app/main/Application.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 15abff9b14..785b02dbb8 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -203,11 +203,7 @@ public: boost::asio::signal_set m_signals; - // Once we get C++20, we could use `std::atomic_flag` for `isTimeToStop` - // and eliminate the need for the condition variable and the mutex. - std::condition_variable stoppingCondition_; - mutable std::mutex stoppingMutex_; - std::atomic isTimeToStop = false; + std::atomic_flag isTimeToStop; std::atomic checkSigs_; @@ -1539,10 +1535,7 @@ ApplicationImp::run() getLoadManager().activateStallDetector(); } - { - std::unique_lock lk{stoppingMutex_}; - stoppingCondition_.wait(lk, [this] { return isTimeToStop.load(); }); - } + isTimeToStop.wait(false, std::memory_order_relaxed); JLOG(m_journal.debug()) << "Application stopping"; @@ -1629,14 +1622,14 @@ ApplicationImp::run() void ApplicationImp::signalStop(std::string msg) { - if (!isTimeToStop.exchange(true)) + if (!isTimeToStop.test_and_set(std::memory_order_acquire)) { if (msg.empty()) JLOG(m_journal.warn()) << "Server stopping"; else JLOG(m_journal.warn()) << "Server stopping: " << msg; - stoppingCondition_.notify_all(); + isTimeToStop.notify_all(); } } @@ -1655,7 +1648,7 @@ ApplicationImp::checkSigs(bool check) bool ApplicationImp::isStopping() const { - return isTimeToStop.load(); + return isTimeToStop.test(std::memory_order_relaxed); } int