threading: never require logging from sync.h
What the developer wrote
Authored by Cory Fields
threading: never require logging from sync.h
sync.h is low-level and should not require any other subsystems.
Move the lone remaining logging call to the .cpp. Any cost incurred by an
additional function call should be trivial compared to the logging itself.
What changed, and why it matters
This is a straightforward internal code cleanup: it moves a debug-only logging call out of a low-level threading header file and into a source file. There is no user-facing change, no bug fix, and no security relevance visible in the commit.
No security action required. Treat as a normal maintainability refactor.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit refactors DEBUG_LOCKCONTENTION handling in Bitcoin Core’s sync primitives. Previously, sync.h included logging/timer.h and inlined a LOG_TIME_MICROS_WITH_CATEGORY call when lock contention was detected. The patch removes the header dependency from sync.h, declares a ContendedLock() template, and defines it in sync.cpp. The behavior is functionally identical: when DEBUG_LOCKCONTENTION is defined, a contended mutex still logs and then locks. The change only reduces coupling between the sync and logging subsystems.
Changed components
src/sync.hsrc/sync.cppInspect captured patch +29 / −7
diff --git a/src/sync.cpp b/src/sync.cpp
index e59f86b9..0e5c623d 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -4,6 +4,7 @@
#include <sync.h>
+#include <logging/timer.h>
#include <tinyformat.h>
#include <util/log.h>
#include <util/strencodings.h>
@@ -19,6 +20,19 @@
#include <utility>
#include <vector>
+#ifdef DEBUG_LOCKCONTENTION
+
+template <typename LockType>
+void ContendedLock(std::string_view name, std::string_view file, int nLine, LockType& lock)
+{
+ LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", name, file, nLine), BCLog::LOCK);
+ lock.lock();
+}
+template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::mutex>& lock);
+template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::recursive_mutex>& lock);
+
+#endif
+
#ifdef DEBUG_LOCKORDER
//
// Early deadlock detection.
diff --git a/src/sync.h b/src/sync.h
index 548b51d9..00fdaae9 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -6,10 +6,6 @@
#ifndef BITCOIN_SYNC_H
#define BITCOIN_SYNC_H
-#ifdef DEBUG_LOCKCONTENTION
-#include <logging/timer.h>
-#endif
-
#include <threadsafety.h> // IWYU pragma: export
#include <util/macros.h>
@@ -77,6 +73,16 @@ inline void DeleteLock(void* cs) {}
inline bool LockStackEmpty() { return true; }
#endif
+/*
+ * Called when a mutex fails to lock immediately because it is held by another
+ * thread, or spuriously. Responsible for locking the lock before returning.
+ */
+#ifdef DEBUG_LOCKCONTENTION
+
+template <typename LockType>
+void ContendedLock(std::string_view name, std::string_view file, int nLine, LockType& lock);
+#endif
+
/**
* Template mixin that adds -Wthread-safety locking annotations and lock order
* checking to a subset of the mutex API.
@@ -151,10 +157,12 @@ private:
{
EnterCritical(pszName, pszFile, nLine, Base::mutex());
#ifdef DEBUG_LOCKCONTENTION
- if (Base::try_lock()) return;
- LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", pszName, pszFile, nLine), BCLog::LOCK);
-#endif
+ if (!Base::try_lock()) {
+ ContendedLock(pszName, pszFile, nLine, static_cast<Base&>(*this));
+ }
+#else
Base::lock();
+#endif
}
bool TryEnter(const char* pszName, const char* pszFile, int nLine)
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.