From 7b816c4e00e286a6dcdf0d9e09c710e1d745a0db Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 16:12:48 +0000 Subject: [PATCH 01/10] threading: rename CSemaphore methods to match std::semaphore --- src/net.cpp | 4 ++-- src/sync.h | 12 ++++++------ src/wallet/sqlite.cpp | 18 +++++++++--------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 66ded1041cd..b78eb749b3b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3428,13 +3428,13 @@ void CConnman::Interrupt() if (semOutbound) { for (int i=0; ipost(); + semOutbound->release(); } } if (semAddnode) { for (int i=0; ipost(); + semAddnode->release(); } } } diff --git a/src/sync.h b/src/sync.h index b22956ef1ab..749a368516b 100644 --- a/src/sync.h +++ b/src/sync.h @@ -321,14 +321,14 @@ public: CSemaphore& operator=(const CSemaphore&) = delete; CSemaphore& operator=(CSemaphore&&) = delete; - void wait() noexcept + void acquire() noexcept { std::unique_lock lock(mutex); condition.wait(lock, [&]() { return value >= 1; }); value--; } - bool try_wait() noexcept + bool try_acquire() noexcept { std::lock_guard lock(mutex); if (value < 1) { @@ -338,7 +338,7 @@ public: return true; } - void post() noexcept + void release() noexcept { { std::lock_guard lock(mutex); @@ -361,7 +361,7 @@ public: if (fHaveGrant) { return; } - sem->wait(); + sem->acquire(); fHaveGrant = true; } @@ -370,13 +370,13 @@ public: if (!fHaveGrant) { return; } - sem->post(); + sem->release(); fHaveGrant = false; } bool TryAcquire() noexcept { - if (!fHaveGrant && sem->try_wait()) { + if (!fHaveGrant && sem->try_acquire()) { fHaveGrant = true; } return fHaveGrant; diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 49096c91aa3..9b6215f9371 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -445,7 +445,7 @@ void SQLiteBatch::Close() try { m_database.Open(); // If TxnAbort failed and we refreshed the connection, the semaphore was not released, so release it here to avoid deadlocks on future writes. - m_database.m_write_semaphore.post(); + m_database.m_write_semaphore.release(); } catch (const std::runtime_error&) { // If open fails, cleanup this object and rethrow the exception m_database.Close(); @@ -498,7 +498,7 @@ bool SQLiteBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite) if (!BindBlobToStatement(stmt, 2, value, "value")) return false; // Acquire semaphore if not previously acquired when creating a transaction. - if (!m_txn) m_database.m_write_semaphore.wait(); + if (!m_txn) m_database.m_write_semaphore.acquire(); // Execute int res = sqlite3_step(stmt); @@ -508,7 +508,7 @@ bool SQLiteBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite) LogPrintf("%s: Unable to execute statement: %s\n", __func__, sqlite3_errstr(res)); } - if (!m_txn) m_database.m_write_semaphore.post(); + if (!m_txn) m_database.m_write_semaphore.release(); return res == SQLITE_DONE; } @@ -522,7 +522,7 @@ bool SQLiteBatch::ExecStatement(sqlite3_stmt* stmt, std::span b if (!BindBlobToStatement(stmt, 1, blob, "key")) return false; // Acquire semaphore if not previously acquired when creating a transaction. - if (!m_txn) m_database.m_write_semaphore.wait(); + if (!m_txn) m_database.m_write_semaphore.acquire(); // Execute int res = sqlite3_step(stmt); @@ -532,7 +532,7 @@ bool SQLiteBatch::ExecStatement(sqlite3_stmt* stmt, std::span b LogPrintf("%s: Unable to execute statement: %s\n", __func__, sqlite3_errstr(res)); } - if (!m_txn) m_database.m_write_semaphore.post(); + if (!m_txn) m_database.m_write_semaphore.release(); return res == SQLITE_DONE; } @@ -651,12 +651,12 @@ std::unique_ptr SQLiteBatch::GetNewPrefixCursor(std::spanExec(m_database, "BEGIN TRANSACTION"); if (res != SQLITE_OK) { LogPrintf("SQLiteBatch: Failed to begin the transaction\n"); - m_database.m_write_semaphore.post(); + m_database.m_write_semaphore.release(); } else { m_txn = true; } @@ -672,7 +672,7 @@ bool SQLiteBatch::TxnCommit() LogPrintf("SQLiteBatch: Failed to commit the transaction\n"); } else { m_txn = false; - m_database.m_write_semaphore.post(); + m_database.m_write_semaphore.release(); } return res == SQLITE_OK; } @@ -686,7 +686,7 @@ bool SQLiteBatch::TxnAbort() LogPrintf("SQLiteBatch: Failed to abort the transaction\n"); } else { m_txn = false; - m_database.m_write_semaphore.post(); + m_database.m_write_semaphore.release(); } return res == SQLITE_OK; } From d870bc94519a68a861bb0ceca19f96c6ba22fbd7 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 18:30:04 +0000 Subject: [PATCH 02/10] threading: add temporary semaphore aliases --- src/sync.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/sync.h b/src/sync.h index 749a368516b..8c1d26c1343 100644 --- a/src/sync.h +++ b/src/sync.h @@ -348,6 +348,9 @@ public: } }; +using BinarySemaphore = CSemaphore; +using Semaphore = CSemaphore; + /** RAII-style semaphore lock */ class CSemaphoreGrant { @@ -427,4 +430,7 @@ public: } }; +using BinarySemaphoreGrant = CSemaphoreGrant; +using SemaphoreGrant = CSemaphoreGrant; + #endif // BITCOIN_SYNC_H From 6790ad27f1570926cef81ef097edaa8b8e70b270 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 18:33:42 +0000 Subject: [PATCH 03/10] scripted-diff: rename CSemaphoreGrant and CSemaphore for net -BEGIN VERIFY SCRIPT- sed -i -e 's|CSemaphoreGrant|SemaphoreGrant|g' -e 's|CSemaphore|Semaphore|g' src/net.h src/net.cpp -END VERIFY SCRIPT- --- src/net.cpp | 16 ++++++++-------- src/net.h | 10 +++++----- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index b78eb749b3b..5e7b8bda351 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1893,7 +1893,7 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ if (max_connections != std::nullopt && existing_connections >= max_connections) return false; // Max total outbound connections already exist - CSemaphoreGrant grant(*semOutbound, true); + SemaphoreGrant grant(*semOutbound, true); if (!grant) return false; OpenNetworkConnection(CAddress(), false, std::move(grant), address.c_str(), conn_type, /*use_v2transport=*/use_v2transport); @@ -2409,7 +2409,7 @@ void CConnman::ProcessAddrFetch() // peer doesn't support it or immediately disconnects us for another reason. const bool use_v2transport(GetLocalServices() & NODE_P2P_V2); CAddress addr; - CSemaphoreGrant grant(*semOutbound, /*fTry=*/true); + SemaphoreGrant grant(*semOutbound, /*fTry=*/true); if (grant) { OpenNetworkConnection(addr, false, std::move(grant), strDest.c_str(), ConnectionType::ADDR_FETCH, use_v2transport); } @@ -2583,7 +2583,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std PerformReconnections(); - CSemaphoreGrant grant(*semOutbound); + SemaphoreGrant grant(*semOutbound); if (interruptNet) return; @@ -2961,7 +2961,7 @@ void CConnman::ThreadOpenAddedConnections() AssertLockNotHeld(m_reconnections_mutex); while (true) { - CSemaphoreGrant grant(*semAddnode); + SemaphoreGrant grant(*semAddnode); std::vector vInfo = GetAddedNodeInfo(/*include_connected=*/false); bool tried = false; for (const AddedNodeInfo& info : vInfo) { @@ -2974,7 +2974,7 @@ void CConnman::ThreadOpenAddedConnections() CAddress addr(CService(), NODE_NONE); OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; - grant = CSemaphoreGrant(*semAddnode, /*fTry=*/true); + grant = SemaphoreGrant(*semAddnode, /*fTry=*/true); } // See if any reconnections are desired. PerformReconnections(); @@ -2985,7 +2985,7 @@ void CConnman::ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char *pszDest, ConnectionType conn_type, bool use_v2transport) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, SemaphoreGrant&& grant_outbound, const char *pszDest, ConnectionType conn_type, bool use_v2transport) { AssertLockNotHeld(m_unused_i2p_sessions_mutex); assert(conn_type != ConnectionType::INBOUND); @@ -3336,11 +3336,11 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) if (semOutbound == nullptr) { // initialize semaphore - semOutbound = std::make_unique(std::min(m_max_automatic_outbound, m_max_automatic_connections)); + semOutbound = std::make_unique(std::min(m_max_automatic_outbound, m_max_automatic_connections)); } if (semAddnode == nullptr) { // initialize semaphore - semAddnode = std::make_unique(m_max_addnode); + semAddnode = std::make_unique(m_max_addnode); } // diff --git a/src/net.h b/src/net.h index 9fdec52115e..9fab1a345b0 100644 --- a/src/net.h +++ b/src/net.h @@ -729,7 +729,7 @@ public: // Setting fDisconnect to true will cause the node to be disconnected the // next time DisconnectNodes() runs std::atomic_bool fDisconnect{false}; - CSemaphoreGrant grantOutbound; + SemaphoreGrant grantOutbound; std::atomic nRefCount{0}; const uint64_t nKeyedNetGroup; @@ -1136,7 +1136,7 @@ public: bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, SemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); bool CheckIncomingNonce(uint64_t nonce); void ASMapHealthCheck(); @@ -1491,8 +1491,8 @@ private: */ std::atomic m_local_services; - std::unique_ptr semOutbound; - std::unique_ptr semAddnode; + std::unique_ptr semOutbound; + std::unique_ptr semAddnode; /** * Maximum number of automatic connections permitted, excluding manual @@ -1614,7 +1614,7 @@ private: struct ReconnectionInfo { CAddress addr_connect; - CSemaphoreGrant grant; + SemaphoreGrant grant; std::string destination; ConnectionType conn_type; bool use_v2transport; From 793166d3810ef3c08cc55c16a17d6d77ae6fabb5 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 18:36:14 +0000 Subject: [PATCH 04/10] wallet: change the write semaphore to a BinarySemaphore Follow-up commits will make a distinction between Semaphore and BinarySemaphore. --- src/wallet/sqlite.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index a3e0de1fcc2..c17ca3a8c9f 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -127,7 +127,7 @@ public: // Batches must acquire this semaphore on writing, and release when done writing. // This ensures that only one batch is modifying the database at a time. - CSemaphore m_write_semaphore; + BinarySemaphore m_write_semaphore; bool Verify(bilingual_str& error); From e6ce5f9e78741ef7f88a8ad237f4b772da921dc3 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 18:39:23 +0000 Subject: [PATCH 05/10] scripted-diff: rename CSemaphore and CSemaphoreGrant CountingSemaphore and CountingSemaphoreGrant model std::counting_semaphore. -BEGIN VERIFY SCRIPT- sed -i -e 's|CSemaphoreGrant|CountingSemaphoreGrant|g' -e 's|CSemaphore|CountingSemaphore|g' src/sync.h -END VERIFY SCRIPT- --- src/sync.h | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/sync.h b/src/sync.h index 8c1d26c1343..45298a0ee84 100644 --- a/src/sync.h +++ b/src/sync.h @@ -304,7 +304,7 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE * * See https://en.wikipedia.org/wiki/Semaphore_(programming) */ -class CSemaphore +class CountingSemaphore { private: std::condition_variable condition; @@ -312,14 +312,14 @@ private: int value; public: - explicit CSemaphore(int init) noexcept : value(init) {} + explicit CountingSemaphore(int init) noexcept : value(init) {} // Disallow default construct, copy, move. - CSemaphore() = delete; - CSemaphore(const CSemaphore&) = delete; - CSemaphore(CSemaphore&&) = delete; - CSemaphore& operator=(const CSemaphore&) = delete; - CSemaphore& operator=(CSemaphore&&) = delete; + CountingSemaphore() = delete; + CountingSemaphore(const CountingSemaphore&) = delete; + CountingSemaphore(CountingSemaphore&&) = delete; + CountingSemaphore& operator=(const CountingSemaphore&) = delete; + CountingSemaphore& operator=(CountingSemaphore&&) = delete; void acquire() noexcept { @@ -348,14 +348,14 @@ public: } }; -using BinarySemaphore = CSemaphore; -using Semaphore = CSemaphore; +using BinarySemaphore = CountingSemaphore; +using Semaphore = CountingSemaphore; /** RAII-style semaphore lock */ -class CSemaphoreGrant +class CountingSemaphoreGrant { private: - CSemaphore* sem; + CountingSemaphore* sem; bool fHaveGrant; public: @@ -386,11 +386,11 @@ public: } // Disallow copy. - CSemaphoreGrant(const CSemaphoreGrant&) = delete; - CSemaphoreGrant& operator=(const CSemaphoreGrant&) = delete; + CountingSemaphoreGrant(const CountingSemaphoreGrant&) = delete; + CountingSemaphoreGrant& operator=(const CountingSemaphoreGrant&) = delete; // Allow move. - CSemaphoreGrant(CSemaphoreGrant&& other) noexcept + CountingSemaphoreGrant(CountingSemaphoreGrant&& other) noexcept { sem = other.sem; fHaveGrant = other.fHaveGrant; @@ -398,7 +398,7 @@ public: other.sem = nullptr; } - CSemaphoreGrant& operator=(CSemaphoreGrant&& other) noexcept + CountingSemaphoreGrant& operator=(CountingSemaphoreGrant&& other) noexcept { Release(); sem = other.sem; @@ -408,9 +408,9 @@ public: return *this; } - CSemaphoreGrant() noexcept : sem(nullptr), fHaveGrant(false) {} + CountingSemaphoreGrant() noexcept : sem(nullptr), fHaveGrant(false) {} - explicit CSemaphoreGrant(CSemaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) + explicit CountingSemaphoreGrant(CountingSemaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) { if (fTry) { TryAcquire(); @@ -419,7 +419,7 @@ public: } } - ~CSemaphoreGrant() + ~CountingSemaphoreGrant() { Release(); } @@ -430,7 +430,7 @@ public: } }; -using BinarySemaphoreGrant = CSemaphoreGrant; -using SemaphoreGrant = CSemaphoreGrant; +using BinarySemaphoreGrant = CountingSemaphoreGrant; +using SemaphoreGrant = CountingSemaphoreGrant; #endif // BITCOIN_SYNC_H From 1acacfbad780f95d1596010ba446dd9ea268fa10 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 18:54:59 +0000 Subject: [PATCH 06/10] threading: make CountingSemaphore/CountingSemaphoreGrant template types --- src/sync.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/sync.h b/src/sync.h index 45298a0ee84..bef029427ab 100644 --- a/src/sync.h +++ b/src/sync.h @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -304,6 +305,7 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE * * See https://en.wikipedia.org/wiki/Semaphore_(programming) */ +template ::max()> class CountingSemaphore { private: @@ -348,14 +350,15 @@ public: } }; -using BinarySemaphore = CountingSemaphore; -using Semaphore = CountingSemaphore; +using BinarySemaphore = CountingSemaphore<1>; +using Semaphore = CountingSemaphore<>; /** RAII-style semaphore lock */ +template ::max()> class CountingSemaphoreGrant { private: - CountingSemaphore* sem; + CountingSemaphore* sem; bool fHaveGrant; public: @@ -410,7 +413,7 @@ public: CountingSemaphoreGrant() noexcept : sem(nullptr), fHaveGrant(false) {} - explicit CountingSemaphoreGrant(CountingSemaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) + explicit CountingSemaphoreGrant(CountingSemaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) { if (fTry) { TryAcquire(); @@ -430,7 +433,7 @@ public: } }; -using BinarySemaphoreGrant = CountingSemaphoreGrant; -using SemaphoreGrant = CountingSemaphoreGrant; +using BinarySemaphoreGrant = CountingSemaphoreGrant<1>; +using SemaphoreGrant = CountingSemaphoreGrant<>; #endif // BITCOIN_SYNC_H From f21365c4fc7f6f45194f5b725192f0054e2daf13 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 18:59:25 +0000 Subject: [PATCH 07/10] threading: replace CountingSemaphore with std::counting_semaphore --- src/sync.h | 57 ++++-------------------------------------------------- 1 file changed, 4 insertions(+), 53 deletions(-) diff --git a/src/sync.h b/src/sync.h index bef029427ab..6f2830fd93b 100644 --- a/src/sync.h +++ b/src/sync.h @@ -301,64 +301,15 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE //! gcc and the -Wreturn-stack-address flag in clang, both enabled by default. #define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }()) -/** An implementation of a semaphore. - * - * See https://en.wikipedia.org/wiki/Semaphore_(programming) - */ -template ::max()> -class CountingSemaphore -{ -private: - std::condition_variable condition; - std::mutex mutex; - int value; - -public: - explicit CountingSemaphore(int init) noexcept : value(init) {} - - // Disallow default construct, copy, move. - CountingSemaphore() = delete; - CountingSemaphore(const CountingSemaphore&) = delete; - CountingSemaphore(CountingSemaphore&&) = delete; - CountingSemaphore& operator=(const CountingSemaphore&) = delete; - CountingSemaphore& operator=(CountingSemaphore&&) = delete; - - void acquire() noexcept - { - std::unique_lock lock(mutex); - condition.wait(lock, [&]() { return value >= 1; }); - value--; - } - - bool try_acquire() noexcept - { - std::lock_guard lock(mutex); - if (value < 1) { - return false; - } - value--; - return true; - } - - void release() noexcept - { - { - std::lock_guard lock(mutex); - value++; - } - condition.notify_one(); - } -}; - -using BinarySemaphore = CountingSemaphore<1>; -using Semaphore = CountingSemaphore<>; +using BinarySemaphore = std::binary_semaphore; +using Semaphore = std::counting_semaphore<>; /** RAII-style semaphore lock */ template ::max()> class CountingSemaphoreGrant { private: - CountingSemaphore* sem; + std::counting_semaphore* sem; bool fHaveGrant; public: @@ -413,7 +364,7 @@ public: CountingSemaphoreGrant() noexcept : sem(nullptr), fHaveGrant(false) {} - explicit CountingSemaphoreGrant(CountingSemaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) + explicit CountingSemaphoreGrant(std::counting_semaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) { if (fTry) { TryAcquire(); From 1f89e2a49a2170a57b14d993f181f29233b7d250 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 19:39:47 +0000 Subject: [PATCH 08/10] scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones -BEGIN VERIFY SCRIPT- sed -i 's|BinarySemaphore|std::binary_semaphore|g' src/wallet/sqlite.h sed -i 's|SemaphoreGrant|CountingGrant|g' src/net.h src/net.cpp sed -i 's|Semaphore|std::counting_semaphore<>|g' src/net.h src/net.cpp sed -i 's|CountingGrant|CountingSemaphoreGrant<>|g' src/net.h src/net.cpp -END VERIFY SCRIPT- --- src/net.cpp | 16 ++++++++-------- src/net.h | 10 +++++----- src/wallet/sqlite.h | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 5e7b8bda351..00fec2bca3e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1893,7 +1893,7 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ if (max_connections != std::nullopt && existing_connections >= max_connections) return false; // Max total outbound connections already exist - SemaphoreGrant grant(*semOutbound, true); + CountingSemaphoreGrant<> grant(*semOutbound, true); if (!grant) return false; OpenNetworkConnection(CAddress(), false, std::move(grant), address.c_str(), conn_type, /*use_v2transport=*/use_v2transport); @@ -2409,7 +2409,7 @@ void CConnman::ProcessAddrFetch() // peer doesn't support it or immediately disconnects us for another reason. const bool use_v2transport(GetLocalServices() & NODE_P2P_V2); CAddress addr; - SemaphoreGrant grant(*semOutbound, /*fTry=*/true); + CountingSemaphoreGrant<> grant(*semOutbound, /*fTry=*/true); if (grant) { OpenNetworkConnection(addr, false, std::move(grant), strDest.c_str(), ConnectionType::ADDR_FETCH, use_v2transport); } @@ -2583,7 +2583,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std PerformReconnections(); - SemaphoreGrant grant(*semOutbound); + CountingSemaphoreGrant<> grant(*semOutbound); if (interruptNet) return; @@ -2961,7 +2961,7 @@ void CConnman::ThreadOpenAddedConnections() AssertLockNotHeld(m_reconnections_mutex); while (true) { - SemaphoreGrant grant(*semAddnode); + CountingSemaphoreGrant<> grant(*semAddnode); std::vector vInfo = GetAddedNodeInfo(/*include_connected=*/false); bool tried = false; for (const AddedNodeInfo& info : vInfo) { @@ -2974,7 +2974,7 @@ void CConnman::ThreadOpenAddedConnections() CAddress addr(CService(), NODE_NONE); OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; - grant = SemaphoreGrant(*semAddnode, /*fTry=*/true); + grant = CountingSemaphoreGrant<>(*semAddnode, /*fTry=*/true); } // See if any reconnections are desired. PerformReconnections(); @@ -2985,7 +2985,7 @@ void CConnman::ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, SemaphoreGrant&& grant_outbound, const char *pszDest, ConnectionType conn_type, bool use_v2transport) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CountingSemaphoreGrant<>&& grant_outbound, const char *pszDest, ConnectionType conn_type, bool use_v2transport) { AssertLockNotHeld(m_unused_i2p_sessions_mutex); assert(conn_type != ConnectionType::INBOUND); @@ -3336,11 +3336,11 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) if (semOutbound == nullptr) { // initialize semaphore - semOutbound = std::make_unique(std::min(m_max_automatic_outbound, m_max_automatic_connections)); + semOutbound = std::make_unique>(std::min(m_max_automatic_outbound, m_max_automatic_connections)); } if (semAddnode == nullptr) { // initialize semaphore - semAddnode = std::make_unique(m_max_addnode); + semAddnode = std::make_unique>(m_max_addnode); } // diff --git a/src/net.h b/src/net.h index 9fab1a345b0..a017440c212 100644 --- a/src/net.h +++ b/src/net.h @@ -729,7 +729,7 @@ public: // Setting fDisconnect to true will cause the node to be disconnected the // next time DisconnectNodes() runs std::atomic_bool fDisconnect{false}; - SemaphoreGrant grantOutbound; + CountingSemaphoreGrant<> grantOutbound; std::atomic nRefCount{0}; const uint64_t nKeyedNetGroup; @@ -1136,7 +1136,7 @@ public: bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, SemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CountingSemaphoreGrant<>&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); bool CheckIncomingNonce(uint64_t nonce); void ASMapHealthCheck(); @@ -1491,8 +1491,8 @@ private: */ std::atomic m_local_services; - std::unique_ptr semOutbound; - std::unique_ptr semAddnode; + std::unique_ptr> semOutbound; + std::unique_ptr> semAddnode; /** * Maximum number of automatic connections permitted, excluding manual @@ -1614,7 +1614,7 @@ private: struct ReconnectionInfo { CAddress addr_connect; - SemaphoreGrant grant; + CountingSemaphoreGrant<> grant; std::string destination; ConnectionType conn_type; bool use_v2transport; diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index c17ca3a8c9f..dda1fab204c 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -127,7 +127,7 @@ public: // Batches must acquire this semaphore on writing, and release when done writing. // This ensures that only one batch is modifying the database at a time. - BinarySemaphore m_write_semaphore; + std::binary_semaphore m_write_semaphore; bool Verify(bilingual_str& error); From fd1546989293b110ad8d86d71f362a11dab3611c Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 19:43:55 +0000 Subject: [PATCH 09/10] threading: semaphore: remove temporary convenience types --- src/sync.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/sync.h b/src/sync.h index 6f2830fd93b..4e5efca4490 100644 --- a/src/sync.h +++ b/src/sync.h @@ -301,9 +301,6 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE //! gcc and the -Wreturn-stack-address flag in clang, both enabled by default. #define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }()) -using BinarySemaphore = std::binary_semaphore; -using Semaphore = std::counting_semaphore<>; - /** RAII-style semaphore lock */ template ::max()> class CountingSemaphoreGrant @@ -385,6 +382,5 @@ public: }; using BinarySemaphoreGrant = CountingSemaphoreGrant<1>; -using SemaphoreGrant = CountingSemaphoreGrant<>; #endif // BITCOIN_SYNC_H From 6f7052a7b96f058568af9aed2f014ae7a25e0f68 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 19:53:26 +0000 Subject: [PATCH 10/10] threading: semaphore: move CountingSemaphoreGrant to its own header --- src/net.h | 1 + src/semaphore_grant.h | 93 +++++++++++++++++++++++++++++++++++++++++++ src/sync.h | 83 -------------------------------------- src/wallet/sqlite.h | 2 + 4 files changed, 96 insertions(+), 83 deletions(-) create mode 100644 src/semaphore_grant.h diff --git a/src/net.h b/src/net.h index a017440c212..4cb4cb906e2 100644 --- a/src/net.h +++ b/src/net.h @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include diff --git a/src/semaphore_grant.h b/src/semaphore_grant.h new file mode 100644 index 00000000000..b1f815f269e --- /dev/null +++ b/src/semaphore_grant.h @@ -0,0 +1,93 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_SEMAPHORE_GRANT_H +#define BITCOIN_SEMAPHORE_GRANT_H + +#include + +/** RAII-style semaphore lock */ +template ::max()> +class CountingSemaphoreGrant +{ +private: + std::counting_semaphore* sem; + bool fHaveGrant; + +public: + void Acquire() noexcept + { + if (fHaveGrant) { + return; + } + sem->acquire(); + fHaveGrant = true; + } + + void Release() noexcept + { + if (!fHaveGrant) { + return; + } + sem->release(); + fHaveGrant = false; + } + + bool TryAcquire() noexcept + { + if (!fHaveGrant && sem->try_acquire()) { + fHaveGrant = true; + } + return fHaveGrant; + } + + // Disallow copy. + CountingSemaphoreGrant(const CountingSemaphoreGrant&) = delete; + CountingSemaphoreGrant& operator=(const CountingSemaphoreGrant&) = delete; + + // Allow move. + CountingSemaphoreGrant(CountingSemaphoreGrant&& other) noexcept + { + sem = other.sem; + fHaveGrant = other.fHaveGrant; + other.fHaveGrant = false; + other.sem = nullptr; + } + + CountingSemaphoreGrant& operator=(CountingSemaphoreGrant&& other) noexcept + { + Release(); + sem = other.sem; + fHaveGrant = other.fHaveGrant; + other.fHaveGrant = false; + other.sem = nullptr; + return *this; + } + + CountingSemaphoreGrant() noexcept : sem(nullptr), fHaveGrant(false) {} + + explicit CountingSemaphoreGrant(std::counting_semaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) + { + if (fTry) { + TryAcquire(); + } else { + Acquire(); + } + } + + ~CountingSemaphoreGrant() + { + Release(); + } + + explicit operator bool() const noexcept + { + return fHaveGrant; + } +}; + +using BinarySemaphoreGrant = CountingSemaphoreGrant<1>; + +#endif // BITCOIN_SEMAPHORE_GRANT_H diff --git a/src/sync.h b/src/sync.h index 4e5efca4490..6eb45f29884 100644 --- a/src/sync.h +++ b/src/sync.h @@ -16,7 +16,6 @@ #include #include -#include #include #include @@ -301,86 +300,4 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE //! gcc and the -Wreturn-stack-address flag in clang, both enabled by default. #define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }()) -/** RAII-style semaphore lock */ -template ::max()> -class CountingSemaphoreGrant -{ -private: - std::counting_semaphore* sem; - bool fHaveGrant; - -public: - void Acquire() noexcept - { - if (fHaveGrant) { - return; - } - sem->acquire(); - fHaveGrant = true; - } - - void Release() noexcept - { - if (!fHaveGrant) { - return; - } - sem->release(); - fHaveGrant = false; - } - - bool TryAcquire() noexcept - { - if (!fHaveGrant && sem->try_acquire()) { - fHaveGrant = true; - } - return fHaveGrant; - } - - // Disallow copy. - CountingSemaphoreGrant(const CountingSemaphoreGrant&) = delete; - CountingSemaphoreGrant& operator=(const CountingSemaphoreGrant&) = delete; - - // Allow move. - CountingSemaphoreGrant(CountingSemaphoreGrant&& other) noexcept - { - sem = other.sem; - fHaveGrant = other.fHaveGrant; - other.fHaveGrant = false; - other.sem = nullptr; - } - - CountingSemaphoreGrant& operator=(CountingSemaphoreGrant&& other) noexcept - { - Release(); - sem = other.sem; - fHaveGrant = other.fHaveGrant; - other.fHaveGrant = false; - other.sem = nullptr; - return *this; - } - - CountingSemaphoreGrant() noexcept : sem(nullptr), fHaveGrant(false) {} - - explicit CountingSemaphoreGrant(std::counting_semaphore& sema, bool fTry = false) noexcept : sem(&sema), fHaveGrant(false) - { - if (fTry) { - TryAcquire(); - } else { - Acquire(); - } - } - - ~CountingSemaphoreGrant() - { - Release(); - } - - explicit operator bool() const noexcept - { - return fHaveGrant; - } -}; - -using BinarySemaphoreGrant = CountingSemaphoreGrant<1>; - #endif // BITCOIN_SYNC_H diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index dda1fab204c..14ad38792c4 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -8,6 +8,8 @@ #include #include +#include + struct bilingual_str; struct sqlite3_stmt;