From 3446c7c81bf349b767e87bac4e007a30da9faf40 Mon Sep 17 00:00:00 2001 From: marta-lokhova Date: Sat, 21 Feb 2026 20:00:57 -0800 Subject: [PATCH 1/2] Address gaps in concurrency thread safety --- src/bucket/BucketManager.h | 2 +- src/bucket/BucketSnapshotManager.h | 2 +- src/bucket/LiveBucketIndex.cpp | 14 +-- src/bucket/LiveBucketIndex.h | 6 +- src/invariant/InvariantManagerImpl.h | 2 +- src/ledger/LedgerManagerImpl.h | 8 +- src/overlay/FlowControl.h | 2 +- src/overlay/Hmac.cpp | 1 + src/overlay/Hmac.h | 21 +++-- src/overlay/Peer.h | 9 +- src/process/ProcessManagerImpl.cpp | 24 ++--- src/process/ProcessManagerImpl.h | 11 ++- src/util/GlobalChecks.h | 5 - src/util/MetricsRegistry.h | 2 +- src/util/Scheduler.h | 3 +- src/util/SimpleTimer.h | 2 +- src/util/ThreadAnnotations.h | 132 ++++++++++++++++++++++++++- src/util/Timer.cpp | 18 ++-- src/util/Timer.h | 11 +-- 19 files changed, 201 insertions(+), 74 deletions(-) diff --git a/src/bucket/BucketManager.h b/src/bucket/BucketManager.h index 24da0e171c..fc935ba0dd 100644 --- a/src/bucket/BucketManager.h +++ b/src/bucket/BucketManager.h @@ -101,7 +101,7 @@ class BucketManager : NonMovableOrCopyable // mLedgerStateMutex to prevent deadlocks. Code must NOT hold mBucketMutex // while trying to acquire LedgerManagerImpl::mLedgerStateMutex, as this // will cause a deadlock. - mutable RecursiveMutex mBucketMutex; + ANNOTATED_RECURSIVE_MUTEX(mBucketMutex); #ifdef THREAD_SAFETY private: diff --git a/src/bucket/BucketSnapshotManager.h b/src/bucket/BucketSnapshotManager.h index 09fbaf1575..a4b2184d3a 100644 --- a/src/bucket/BucketSnapshotManager.h +++ b/src/bucket/BucketSnapshotManager.h @@ -37,7 +37,7 @@ class BucketSnapshotManager : NonMovableOrCopyable AppConnector& mAppConnector; // Lock must be held when accessing any member variables holding snapshots - mutable SharedMutex mSnapshotMutex; + ANNOTATED_SHARED_MUTEX(mSnapshotMutex); // Snapshot that is maintained and periodically updated by BucketManager on // the main thread. When background threads need to generate or refresh a diff --git a/src/bucket/LiveBucketIndex.cpp b/src/bucket/LiveBucketIndex.cpp index a55f50a2fe..1142a621be 100644 --- a/src/bucket/LiveBucketIndex.cpp +++ b/src/bucket/LiveBucketIndex.cpp @@ -102,7 +102,7 @@ LiveBucketIndex::maybeInitializeCache(size_t totalBucketListAccountsSizeBytes, } // Cache is already initialized - if (std::shared_lock lock(mCacheMutex); mCache) + if (SharedLockShared lock(mCacheMutex); mCache) { return; } @@ -123,7 +123,7 @@ LiveBucketIndex::maybeInitializeCache(size_t totalBucketListAccountsSizeBytes, return; } - std::unique_lock lock(mCacheMutex); + SharedLockExclusive lock(mCacheMutex); if (totalBucketListAccountsSizeBytes < maxBucketListBytesToCache) { // We can cache the entire bucket @@ -202,7 +202,7 @@ LiveBucketIndex::getCachedEntry(LedgerKey const& k) const { if (shouldUseCache() && isCachedType(k)) { - std::shared_lock lock(mCacheMutex); + SharedLockShared lock(mCacheMutex); auto cachePtr = mCache->maybeGet(k); if (cachePtr) { @@ -323,7 +323,7 @@ LiveBucketIndex::shouldUseCache() const { if (mDiskIndex) { - std::shared_lock lock(mCacheMutex); + SharedLockShared lock(mCacheMutex); return mCache != nullptr; } @@ -353,7 +353,7 @@ LiveBucketIndex::maybeAddToCache( // earlier. mCacheMissMeter.Mark(); - std::unique_lock lock(mCacheMutex); + SharedLockExclusive lock(mCacheMutex); mCache->put(k, entry); } } @@ -392,7 +392,7 @@ LiveBucketIndex::getMaxCacheSize() const { if (shouldUseCache()) { - std::shared_lock lock(mCacheMutex); + SharedLockShared lock(mCacheMutex); return mCache->maxSize(); } @@ -405,7 +405,7 @@ LiveBucketIndex::getCurrentCacheSize() const { if (shouldUseCache()) { - std::shared_lock lock(mCacheMutex); + SharedLockShared lock(mCacheMutex); return mCache->size(); } diff --git a/src/bucket/LiveBucketIndex.h b/src/bucket/LiveBucketIndex.h index abd2582d23..2f232ddf9a 100644 --- a/src/bucket/LiveBucketIndex.h +++ b/src/bucket/LiveBucketIndex.h @@ -12,13 +12,13 @@ #include "ledger/LedgerHashUtils.h" // IWYU pragma: keep #include "util/NonCopyable.h" #include "util/RandomEvictionCache.h" +#include "util/ThreadAnnotations.h" #include "util/XDROperators.h" // IWYU pragma: keep #include "xdr/Stellar-ledger-entries.h" #include #include #include -#include namespace asio { @@ -65,8 +65,8 @@ class LiveBucketIndex : public NonMovableOrCopyable // The indexes themselves are thread safe, as they are immutable after // construction. The cache is not, all accesses must first acquire this // mutex. - mutable std::unique_ptr mCache{}; - mutable std::shared_mutex mCacheMutex; + mutable std::unique_ptr mCache GUARDED_BY(mCacheMutex){}; + ANNOTATED_SHARED_MUTEX(mCacheMutex); medida::Meter& mCacheHitMeter; medida::Meter& mCacheMissMeter; diff --git a/src/invariant/InvariantManagerImpl.h b/src/invariant/InvariantManagerImpl.h index 55d9bd7e18..44830cfe40 100644 --- a/src/invariant/InvariantManagerImpl.h +++ b/src/invariant/InvariantManagerImpl.h @@ -37,7 +37,7 @@ class InvariantManagerImpl : public InvariantManager std::string lastFailedWithMessage; }; - Mutex mutable mFailureInformationMutex; + ANNOTATED_MUTEX(mFailureInformationMutex); std::map mFailureInformation GUARDED_BY(mFailureInformationMutex); diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h index faa7ba5ffb..02b74ae196 100644 --- a/src/ledger/LedgerManagerImpl.h +++ b/src/ledger/LedgerManagerImpl.h @@ -296,11 +296,8 @@ class LedgerManagerImpl : public LedgerManager VirtualClock::time_point mLastClose; // Use mutex to guard ledger state during apply - mutable RecursiveMutex mLedgerStateMutex -#ifdef THREAD_SAFETY - ACQUIRED_BEFORE(BucketManager::mBucketMutex) -#endif - ; + ANNOTATED_RECURSIVE_MUTEX(mLedgerStateMutex, + ACQUIRED_BEFORE(BucketManager::mBucketMutex)); medida::Timer& mCatchupDuration; @@ -564,6 +561,7 @@ class LedgerManagerImpl : public LedgerManager virtual bool isApplying() const override { + releaseAssert(threadIsMain()); return mCurrentlyApplyingLedger; } void markApplyStateReset() override; diff --git a/src/overlay/FlowControl.h b/src/overlay/FlowControl.h index b3e98f1f60..f8d251feeb 100644 --- a/src/overlay/FlowControl.h +++ b/src/overlay/FlowControl.h @@ -67,7 +67,7 @@ class FlowControl size_t mTxQueueByteCount GUARDED_BY(mFlowControlMutex){0}; // Mutex to synchronize flow control state - Mutex mutable mFlowControlMutex; + ANNOTATED_MUTEX(mFlowControlMutex); // Is this peer currently throttled due to lack of capacity std::optional mLastThrottle GUARDED_BY(mFlowControlMutex); diff --git a/src/overlay/Hmac.cpp b/src/overlay/Hmac.cpp index f455318d8e..ee6ed72b64 100644 --- a/src/overlay/Hmac.cpp +++ b/src/overlay/Hmac.cpp @@ -83,6 +83,7 @@ Hmac::setAuthenticatedMessageBody(AuthenticatedMessage& aMsg, void Hmac::damageRecvMacKey() { + LOCK_GUARD(mMutex, guard); auto bytes = randomBytes(mRecvMacKey.key.size()); std::copy(bytes.begin(), bytes.end(), mRecvMacKey.key.begin()); } diff --git a/src/overlay/Hmac.h b/src/overlay/Hmac.h index c4850a75c7..6ed623975c 100644 --- a/src/overlay/Hmac.h +++ b/src/overlay/Hmac.h @@ -11,17 +11,22 @@ using namespace stellar; +namespace stellar +{ +class Peer; +} + class Hmac { -#ifndef USE_TRACY - Mutex mMutex; -#else - TracyLockable(std::mutex, mMutex); +#ifdef THREAD_SAFETY + friend class stellar::Peer; #endif - HmacSha256Key mSendMacKey; - HmacSha256Key mRecvMacKey; - uint64_t mSendMacSeq{0}; - uint64_t mRecvMacSeq{0}; + + ANNOTATED_MUTEX(mMutex); + HmacSha256Key mSendMacKey GUARDED_BY(mMutex); + HmacSha256Key mRecvMacKey GUARDED_BY(mMutex); + uint64_t mSendMacSeq GUARDED_BY(mMutex){0}; + uint64_t mRecvMacSeq GUARDED_BY(mMutex){0}; public: bool setSendMackey(HmacSha256Key const& key); diff --git a/src/overlay/Peer.h b/src/overlay/Peer.h index fcba076869..c5853743a4 100644 --- a/src/overlay/Peer.h +++ b/src/overlay/Peer.h @@ -195,12 +195,9 @@ class Peer : public std::enable_shared_from_this, #endif // Mutex to protect PeerState, which can be accessed and modified from - // multiple threads -#ifndef USE_TRACY - RecursiveMutex mutable mStateMutex; -#else - mutable TracyLockable(std::recursive_mutex, mStateMutex); -#endif + // multiple threads. + // LOCK ORDERING: mStateMutex must be acquired before Hmac::mMutex. + ANNOTATED_RECURSIVE_MUTEX(mStateMutex, ACQUIRED_BEFORE(Hmac::mMutex)); Hmac mHmac; // Does local node have capacity to read from this peer diff --git a/src/process/ProcessManagerImpl.cpp b/src/process/ProcessManagerImpl.cpp index ee1e866bf2..395670090a 100644 --- a/src/process/ProcessManagerImpl.cpp +++ b/src/process/ProcessManagerImpl.cpp @@ -197,7 +197,7 @@ class ProcessExitEvent::Impl size_t ProcessManagerImpl::getNumRunningProcesses() { - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); size_t n = 0; for (auto const& pe : mProcesses) { @@ -212,7 +212,7 @@ ProcessManagerImpl::getNumRunningProcesses() size_t ProcessManagerImpl::getNumRunningOrShuttingDownProcesses() { - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); return mProcesses.size(); } @@ -262,7 +262,7 @@ ProcessManagerImpl::isShutdown() const void ProcessManagerImpl::shutdown() { - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); if (!mIsShutdown) { mIsShutdown = true; @@ -281,7 +281,7 @@ ProcessManagerImpl::shutdown() void ProcessManagerImpl::tryProcessShutdownAll() { - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); for (auto const& pe : mProcesses) { tryProcessShutdown(pe.second); @@ -292,7 +292,7 @@ bool ProcessManagerImpl::tryProcessShutdown(std::shared_ptr pe) { ZoneScoped; - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); checkInvariants(); if (!pe) @@ -364,7 +364,7 @@ asio::error_code ProcessManagerImpl::handleProcessTermination(int pid, int status) { ZoneScoped; - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); checkInvariants(); auto pair = mProcesses.find(pid); @@ -668,14 +668,14 @@ ProcessManagerImpl::ProcessManagerImpl(Application& app) , mTmpDir( std::make_unique(app.getTmpDirManager().tmpDir("process"))) { - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); startWaitingForSignalChild(); } void ProcessManagerImpl::startWaitingForSignalChild() { - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); mSigChild.async_wait( std::bind(&ProcessManagerImpl::handleSignalChild, this)); } @@ -696,7 +696,7 @@ ProcessManagerImpl::reapChildren() { // Store tuples (pid, status) std::vector> signaledChildren; - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); for (auto const& pair : mProcesses) { int const pid = pair.first; @@ -840,7 +840,7 @@ std::weak_ptr ProcessManagerImpl::runProcess(std::string const& cmdLine, std::string outFile) { ZoneScoped; - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); auto pe = std::shared_ptr(new ProcessExitEvent(mIOContext)); @@ -867,7 +867,7 @@ ProcessManagerImpl::maybeRunPendingProcesses() { return; } - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); while (!mPending.empty() && getNumRunningOrShuttingDownProcesses() < mMaxProcesses) { @@ -905,7 +905,7 @@ ProcessManagerImpl::maybeRunPendingProcesses() void ProcessManagerImpl::checkInvariants() { - std::lock_guard guard(mProcessesMutex); + RECURSIVE_LOCK_GUARD(mProcessesMutex, guard); if (mIsShutdown) { releaseAssertOrThrow(mPending.empty()); diff --git a/src/process/ProcessManagerImpl.h b/src/process/ProcessManagerImpl.h index b29a5f0c1a..675d95cd1d 100644 --- a/src/process/ProcessManagerImpl.h +++ b/src/process/ProcessManagerImpl.h @@ -5,6 +5,7 @@ #pragma once #include "process/ProcessManager.h" +#include "util/ThreadAnnotations.h" #include "util/TmpDir.h" #include #include @@ -18,14 +19,15 @@ class ProcessManagerImpl : public ProcessManager { // Subprocesses will be removed asynchronously, hence the lock on // just the mProcesses member. - std::recursive_mutex mProcessesMutex; + ANNOTATED_RECURSIVE_MUTEX(mProcessesMutex); // Stores a map from pid to running-or-shutting-down processes. // Any ProcessExitEvent should be stored either in mProcesses // or in mPending (before it's launched). - std::map> mProcesses; + std::map> + mProcesses GUARDED_BY(mProcessesMutex); - bool mIsShutdown{false}; + std::atomic mIsShutdown{false}; size_t const mMaxProcesses; asio::io_context& mIOContext; // These are only used on POSIX, but they're harmless here. @@ -33,7 +35,8 @@ class ProcessManagerImpl : public ProcessManager std::unique_ptr mTmpDir; uint64_t mTempFileCount{0}; - std::deque> mPending; + std::deque> + mPending GUARDED_BY(mProcessesMutex); void maybeRunPendingProcesses(); void checkInvariants(); diff --git a/src/util/GlobalChecks.h b/src/util/GlobalChecks.h index 0e8b51b7b2..f87a87463e 100644 --- a/src/util/GlobalChecks.h +++ b/src/util/GlobalChecks.h @@ -46,13 +46,8 @@ void dbgAbort(); #endif -#ifndef USE_TRACY using RecursiveLockGuard = RecursiveMutexLocker; using LockGuard = MutexLocker; -#else -using RecursiveLockGuard = std::lock_guard; -using LockGuard = std::lock_guard; -#endif #define RECURSIVE_LOCK_GUARD(mutex_, guardName) \ RecursiveLockGuard guardName(mutex_) #define LOCK_GUARD(mutex_, guardName) LockGuard guardName(mutex_) diff --git a/src/util/MetricsRegistry.h b/src/util/MetricsRegistry.h index b2c057f83d..9cd0656ebe 100644 --- a/src/util/MetricsRegistry.h +++ b/src/util/MetricsRegistry.h @@ -10,7 +10,7 @@ namespace stellar // Wrapper around medida::MetricsRegistry to support registering `SimpleTimer`s class MetricsRegistry : public medida::MetricsRegistry { - Mutex mLock; + ANNOTATED_MUTEX(mLock); // Note that it is safe to hand out references to this map because values // have pointer stability. std::map mSimpleTimers GUARDED_BY(mLock); diff --git a/src/util/Scheduler.h b/src/util/Scheduler.h index 7fb25aa527..6f1b2a5860 100644 --- a/src/util/Scheduler.h +++ b/src/util/Scheduler.h @@ -4,6 +4,7 @@ #pragma once +#include #include #include #include @@ -183,7 +184,7 @@ class Scheduler // or run. size_t mSize{0}; - bool mIsShutdown{false}; + std::atomic mIsShutdown{false}; void trimSingleActionQueue(Qptr q, std::chrono::steady_clock::time_point now); diff --git a/src/util/SimpleTimer.h b/src/util/SimpleTimer.h index d247eaefde..91d0cc4698 100644 --- a/src/util/SimpleTimer.h +++ b/src/util/SimpleTimer.h @@ -43,7 +43,7 @@ class SimpleTimer medida::Counter& mMaxSampleValue; std::int64_t mMax GUARDED_BY(mLock); - Mutex mLock; + ANNOTATED_MUTEX(mLock); std::chrono::nanoseconds const mDurationUnit; diff --git a/src/util/ThreadAnnotations.h b/src/util/ThreadAnnotations.h index 5f878f9d91..df889e7f9b 100644 --- a/src/util/ThreadAnnotations.h +++ b/src/util/ThreadAnnotations.h @@ -18,6 +18,10 @@ #include #include +#ifdef USE_TRACY +#include +#endif + #if defined(__clang__) && (!defined(SWIG)) #define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x)) #else @@ -130,8 +134,22 @@ // class). #define SCOPED_LOCKABLE THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable) -// Defines an annotated interface for mutexes. -// These methods can be implemented to use any internal mutex implementation. +// Helper macros for declaring mutexes with optional thread safety annotations. +// These macros handle the conditional compilation based on THREAD_SAFETY and +// USE_TRACY flags, eliminating scattered #ifdef blocks in user code. + +// When THREAD_SAFETY is enabled, applies the given annotations to the mutex +// declaration. Otherwise, the annotations are stripped (becoming a no-op). +#ifdef THREAD_SAFETY +#define ANNOTATE_FOR_THREAD_SAFETY(...) __VA_ARGS__ +#else +#define ANNOTATE_FOR_THREAD_SAFETY(...) +#endif + +// Mutex wrapper that conditionally uses Tracy instrumentation. +// When USE_TRACY is enabled, this wraps std::mutex with Tracy tracking. +// When THREAD_SAFETY is enabled, it enables static thread safety checks. +#ifndef USE_TRACY class LOCKABLE Mutex : public stellar::NonMovableOrCopyable { private: @@ -155,9 +173,33 @@ class LOCKABLE Mutex : public stellar::NonMovableOrCopyable mMutex.unlock(); } }; +#else +// When Tracy is enabled, we use TracyLockable for profiling visibility. +// Note: We still provide a wrapper class for consistency with non-Tracy builds. +// Users should access the underlying TracyLockable directly for profiling. +class Mutex : public stellar::NonMovableOrCopyable +{ + private: + std::mutex mMutex; + + public: + void + Lock() + { + mMutex.lock(); + } + + void + Unlock() + { + mMutex.unlock(); + } +}; +#endif // MutexLocker is an RAII class that acquires a mutex in its constructor, and // releases it in its destructor. +#ifndef USE_TRACY template class SCOPED_LOCKABLE MutexLockerT : public stellar::NonMovableOrCopyable { @@ -174,6 +216,25 @@ class SCOPED_LOCKABLE MutexLockerT : public stellar::NonMovableOrCopyable mut.Unlock(); } }; +#else +// Tracy's Lockable uses standard lock()/unlock() interface +template +class MutexLockerT : public stellar::NonMovableOrCopyable +{ + private: + MutexType& mut; + + public: + MutexLockerT(MutexType& mu) : mut(mu) + { + mu.lock(); + } + ~MutexLockerT() + { + mut.unlock(); + } +}; +#endif // Defines an annotated interface for shared mutexes (read-write locks). // These methods can be implemented to use any internal shared_mutex @@ -217,6 +278,7 @@ class LOCKABLE SharedMutex : public stellar::NonMovableOrCopyable // SharedLockShared is an RAII class that acquires a shared mutex in shared // mode in its constructor, and releases it in its destructor. +#ifndef USE_TRACY class SCOPED_LOCKABLE SharedLockShared : public stellar::NonMovableOrCopyable { private: @@ -232,6 +294,24 @@ class SCOPED_LOCKABLE SharedLockShared : public stellar::NonMovableOrCopyable mut.UnlockShared(); } }; +#else +// Tracy's SharedLockable uses standard lock_shared()/unlock_shared() +class SharedLockShared : public stellar::NonMovableOrCopyable +{ + private: + tracy::SharedLockable& mut; + + public: + SharedLockShared(tracy::SharedLockable& mu) : mut(mu) + { + mu.lock_shared(); + } + ~SharedLockShared() + { + mut.unlock_shared(); + } +}; +#endif // Defines an annotated interface for recursive mutexes. // These methods can be implemented to use any internal recursive_mutex @@ -260,8 +340,56 @@ class LOCKABLE RecursiveMutex : public stellar::NonMovableOrCopyable } }; +#ifndef USE_TRACY using MutexLocker = MutexLockerT; using RecursiveMutexLocker = MutexLockerT; using SharedLockExclusive = MutexLockerT; +#else +using MutexLocker = MutexLockerT>; +using RecursiveMutexLocker = + MutexLockerT>; +using SharedLockExclusive = + MutexLockerT>; +#endif + +// Mutex declaration macros that handle conditional compilation. +// These macros eliminate the need for scattered #ifdef blocks throughout the +// code. +// +// Usage examples: +// ANNOTATED_MUTEX(mMyMutex) +// ANNOTATED_RECURSIVE_MUTEX(mMyMutex, ACQUIRED_BEFORE(other)) +// +// The macros automatically: +// - Choose between TracyLockable and standard Mutex based on USE_TRACY +// - Apply thread safety annotations only when THREAD_SAFETY is enabled + +// Standard mutex declaration with optional thread safety annotations. +// The annotations parameter can be empty or contain one or more annotations. +#ifndef USE_TRACY +#define ANNOTATED_MUTEX(VarName, ...) \ + Mutex mutable VarName ANNOTATE_FOR_THREAD_SAFETY(__VA_ARGS__) +#else +#define ANNOTATED_MUTEX(VarName, ...) mutable TracyLockable(std::mutex, VarName) +#endif + +// Recursive mutex declaration with optional thread safety annotations. +#ifndef USE_TRACY +#define ANNOTATED_RECURSIVE_MUTEX(VarName, ...) \ + RecursiveMutex mutable VarName ANNOTATE_FOR_THREAD_SAFETY(__VA_ARGS__) +#else +#define ANNOTATED_RECURSIVE_MUTEX(VarName, ...) \ + mutable TracyLockable(std::recursive_mutex, VarName) +#endif + +// Shared (read-write) mutex declaration with optional thread safety +// annotations. +#ifndef USE_TRACY +#define ANNOTATED_SHARED_MUTEX(VarName, ...) \ + SharedMutex mutable VarName ANNOTATE_FOR_THREAD_SAFETY(__VA_ARGS__) +#else +#define ANNOTATED_SHARED_MUTEX(VarName, ...) \ + mutable TracySharedLockable(std::shared_mutex, VarName) +#endif #endif // THREAD_ANNOTATIONS_H_ diff --git a/src/util/Timer.cpp b/src/util/Timer.cpp index 9f46b26e1e..2d776faccf 100644 --- a/src/util/Timer.cpp +++ b/src/util/Timer.cpp @@ -38,7 +38,7 @@ VirtualClock::now() const noexcept } else { - std::lock_guard lock(mVirtualNowMutex); + LOCK_GUARD(mVirtualNowMutex, lock); return mVirtualNow; } } @@ -52,7 +52,7 @@ VirtualClock::system_now() const noexcept } else { - std::lock_guard lock(mVirtualNowMutex); + LOCK_GUARD(mVirtualNowMutex, lock); auto offset = mVirtualNow.time_since_epoch(); return std::chrono::system_clock::time_point( std::chrono::duration_cast< @@ -260,7 +260,7 @@ VirtualClock::shutdown() // Clear pending queue for the scheduler { - std::lock_guard guard(mPendingActionQueueMutex); + LOCK_GUARD(mPendingActionQueueMutex, guard); mPendingActionQueue = std::queue, std::string, Scheduler::ActionType>>(); @@ -275,7 +275,7 @@ void VirtualClock::setCurrentVirtualTime(time_point t) { releaseAssert(mMode == VIRTUAL_TIME); - std::lock_guard lock(mVirtualNowMutex); + LOCK_GUARD(mVirtualNowMutex, lock); // Maintain monotonicity in VIRTUAL_TIME mode. releaseAssert(t >= mVirtualNow); mVirtualNow = t; @@ -398,7 +398,7 @@ VirtualClock::crank(bool block) // results are waiting in the pending queue. bool hasPendingActions = false; { - std::lock_guard guard(mPendingActionQueueMutex); + LOCK_GUARD(mPendingActionQueueMutex, guard); hasPendingActions = !mPendingActionQueue.empty(); } if (!hasPendingActions) @@ -414,7 +414,7 @@ VirtualClock::crank(bool block) // Transfer any pending actions to the scheduler, counting them as // "progress" also. { - std::lock_guard guard(mPendingActionQueueMutex); + LOCK_GUARD(mPendingActionQueueMutex, guard); while (!mPendingActionQueue.empty()) { auto& f = mPendingActionQueue.front(); @@ -464,7 +464,7 @@ VirtualClock::postAction(std::function&& f, std::string&& name, bool queueWasEmpty = false; { - std::lock_guard lock(mPendingActionQueueMutex); + LOCK_GUARD(mPendingActionQueueMutex, lock); queueWasEmpty = mPendingActionQueue.empty(); mPendingActionQueue.emplace(std::move(f), std::move(name), type); } @@ -497,7 +497,7 @@ VirtualClock::getActionQueueSize() const { size_t pending = 0; { - std::lock_guard guard(mPendingActionQueueMutex); + LOCK_GUARD(mPendingActionQueueMutex, guard); pending = mPendingActionQueue.size(); } return pending + mActionScheduler->size(); @@ -574,7 +574,7 @@ VirtualClock::advanceToNext() auto nextEvent = next(); // jump forward in time, if needed { - std::lock_guard lock(mVirtualNowMutex); + LOCK_GUARD(mVirtualNowMutex, lock); if (mVirtualNow < nextEvent) { mVirtualNow = nextEvent; diff --git a/src/util/Timer.h b/src/util/Timer.h index 8ac82fc4d9..2dbe2350cc 100644 --- a/src/util/Timer.h +++ b/src/util/Timer.h @@ -10,13 +10,12 @@ #include "util/asio.h" #include "util/NonCopyable.h" #include "util/Scheduler.h" +#include "util/ThreadAnnotations.h" #include #include #include -#include #include -#include #include namespace stellar @@ -166,7 +165,7 @@ class VirtualClock Mode const mMode; size_t nRealTimerCancelEvents{0}; - time_point mVirtualNow; + time_point mVirtualNow GUARDED_BY(mVirtualNowMutex); std::atomic mBackgroundWorkCount{0}; @@ -188,10 +187,10 @@ class VirtualClock std::chrono::steady_clock::time_point mLastDispatchStart; std::unique_ptr mActionScheduler; - mutable std::mutex mPendingActionQueueMutex; + ANNOTATED_MUTEX(mPendingActionQueueMutex); std::queue< std::tuple, std::string, Scheduler::ActionType>> - mPendingActionQueue; + mPendingActionQueue GUARDED_BY(mPendingActionQueueMutex); using PrQueue = std::priority_queue, @@ -208,7 +207,7 @@ class VirtualClock // timer should be last to ensure it gets destroyed first RealSteadyTimer mRealTimer; - std::mutex mutable mVirtualNowMutex; + ANNOTATED_MUTEX(mVirtualNowMutex); public: // A VirtualClock is instantiated in either real or virtual mode. In real From 0b59f279912135fe6c8f948b93ceb9ee30d54d5b Mon Sep 17 00:00:00 2001 From: marta-lokhova Date: Wed, 4 Mar 2026 07:03:27 -0800 Subject: [PATCH 2/2] Add thread-safety-beta for more coverage --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 98a74ec27b..9347b5da33 100644 --- a/configure.ac +++ b/configure.ac @@ -149,7 +149,7 @@ AC_ARG_ENABLE([threadsafety], AS_IF([test "x$enable_threadsafety" = "xyes"], [ AC_MSG_NOTICE([enabling thread safety static analysis, see https://clang.llvm.org/docs/ThreadSafetyAnalysis.html]) AS_CASE(["$CXX"], - [clang*], [CXXFLAGS="$CXXFLAGS -Wthread-safety -Werror=thread-safety -DTHREAD_SAFETY"], + [clang*], [CXXFLAGS="$CXXFLAGS -Wthread-safety -Werror=thread-safety -Wthread-safety-beta -Werror=thread-safety-beta -DTHREAD_SAFETY"], [AC_MSG_WARN([Thread safety analysis is only supported with Clang compiler, skipping])]) ])