From 6783426489e00fd5a1b32f4e6cb6316058e0ec27 Mon Sep 17 00:00:00 2001 From: battlmonstr Date: Tue, 8 Oct 2024 20:47:25 +0200 Subject: [PATCH] code style: scoped_lock (#2407) --- docs/code_style.md | 3 ++- silkworm/db/etl/collector.hpp | 4 ++-- silkworm/db/etl/in_memory_collector.hpp | 4 ++-- silkworm/db/kv/api/state_cache.cpp | 8 ++++---- silkworm/db/kv/grpc/server/state_change_collection.cpp | 8 ++++---- silkworm/db/mdbx/bitmap.cpp | 8 ++++---- silkworm/db/mdbx/bitmap.hpp | 2 +- silkworm/infra/common/log.cpp | 2 +- .../stagedsync/stages/stage_interhashes/trie_loader.cpp | 2 +- .../stagedsync/stages/stage_interhashes/trie_loader.hpp | 2 +- silkworm/rpc/core/filter_storage.cpp | 6 +++--- 11 files changed, 25 insertions(+), 24 deletions(-) diff --git a/docs/code_style.md b/docs/code_style.md index 35403eeaff..fd44f7b75a 100644 --- a/docs/code_style.md +++ b/docs/code_style.md @@ -114,7 +114,8 @@ Bad: ### P4 RAII lock type usage -Use `std::scoped_lock` for a common case where a single mutex needs to be locked immediately and unlocked at the end of the scope. Use other lock types sparingly if other features are needed (e.g. multiple mutexes, deferred locking etc). +Use `std::scoped_lock` for a common case where a single mutex needs to be locked immediately and unlocked at the end of the scope. Do not use `std::lock_guard`. +Use `std::unique_lock` where a manual `unlock()` is required, for working with `std::condition_variable` or if other unique_lock features are needed (e.g. deferred locking, adoption). ### P5 SILKWORM_ASSERT diff --git a/silkworm/db/etl/collector.hpp b/silkworm/db/etl/collector.hpp index 3d7dd331f6..8ea67a0a56 100644 --- a/silkworm/db/etl/collector.hpp +++ b/silkworm/db/etl/collector.hpp @@ -90,7 +90,7 @@ class Collector { //! \brief Returns the hex representation of current load key (for progress tracking) [[nodiscard]] std::string get_load_key() const { - std::unique_lock lock{mutex_}; + std::scoped_lock lock{mutex_}; return loading_key_; } @@ -100,7 +100,7 @@ class Collector { void flush_buffer(); // Write buffer to file void set_loading_key(ByteView key) { - std::unique_lock lock{mutex_}; + std::scoped_lock lock{mutex_}; loading_key_ = to_hex(key, true); } diff --git a/silkworm/db/etl/in_memory_collector.hpp b/silkworm/db/etl/in_memory_collector.hpp index 21a297c341..6cd0c96a9b 100644 --- a/silkworm/db/etl/in_memory_collector.hpp +++ b/silkworm/db/etl/in_memory_collector.hpp @@ -132,7 +132,7 @@ class InMemoryCollector { //! \brief Returns the hex representation of current load key (for progress tracking) [[nodiscard]] std::string get_load_key() const { - std::unique_lock l{mutex_}; + std::scoped_lock l{mutex_}; return loading_key_; } @@ -148,7 +148,7 @@ class InMemoryCollector { // for progress tracking only void set_loading_key(ByteView key) { - std::unique_lock l{mutex_}; + std::scoped_lock l{mutex_}; loading_key_ = to_hex(key, true); } mutable std::mutex mutex_{}; // To sync loading_key_ diff --git a/silkworm/db/kv/api/state_cache.cpp b/silkworm/db/kv/api/state_cache.cpp index 80d6dd0002..8a27c77456 100644 --- a/silkworm/db/kv/api/state_cache.cpp +++ b/silkworm/db/kv/api/state_cache.cpp @@ -48,7 +48,7 @@ CoherentStateCache::CoherentStateCache(CoherentCacheConfig config) : config_(con std::unique_ptr CoherentStateCache::get_view(Transaction& tx) { const auto view_id = tx.view_id(); - std::unique_lock write_lock{rw_mutex_}; + std::scoped_lock write_lock{rw_mutex_}; CoherentStateRoot* root = get_root(view_id); return root->ready ? std::make_unique(tx, this) : nullptr; } @@ -76,7 +76,7 @@ void CoherentStateCache::on_new_block(const api::StateChangeSet& state_changes_s return; } - std::unique_lock write_lock{rw_mutex_}; + std::scoped_lock write_lock{rw_mutex_}; const auto view_id = state_changes_set.state_version_id; CoherentStateRoot* root = advance_root(view_id); @@ -248,7 +248,7 @@ Task> CoherentStateCache::get(ByteView key, Transaction& tx } read_lock.unlock(); - std::unique_lock write_lock{rw_mutex_}; + std::scoped_lock write_lock{rw_mutex_}; kv.value = value; add(std::move(kv), root_it->second.get(), view_id); @@ -290,7 +290,7 @@ Task> CoherentStateCache::get_code(ByteView key, Transactio } read_lock.unlock(); - std::unique_lock write_lock{rw_mutex_}; + std::scoped_lock write_lock{rw_mutex_}; kv.value = value; add_code(std::move(kv), root_it->second.get(), view_id); diff --git a/silkworm/db/kv/grpc/server/state_change_collection.cpp b/silkworm/db/kv/grpc/server/state_change_collection.cpp index a89f3167b2..460d9a2c41 100644 --- a/silkworm/db/kv/grpc/server/state_change_collection.cpp +++ b/silkworm/db/kv/grpc/server/state_change_collection.cpp @@ -27,14 +27,14 @@ namespace silkworm { std::optional StateChangeCollection::subscribe(StateChangeConsumer consumer, StateChangeFilter /*filter*/) { - std::unique_lock consumers_lock{consumers_mutex_}; + std::scoped_lock consumers_lock{consumers_mutex_}; StateChangeToken token = ++next_token_; const auto [_, inserted] = consumers_.insert({token, consumer}); return inserted ? std::make_optional(token) : std::nullopt; } bool StateChangeCollection::unsubscribe(StateChangeToken token) { - std::unique_lock consumers_lock{consumers_mutex_}; + std::scoped_lock consumers_lock{consumers_mutex_}; const auto consumer_it = consumers_.erase(token); return consumer_it != 0; } @@ -204,7 +204,7 @@ void StateChangeCollection::notify_batch(uint64_t pending_base_fee, uint64_t gas state_changes_.set_block_gas_limit(gas_limit); state_changes_.set_state_version_id(tx_id_); - std::unique_lock consumers_lock{consumers_mutex_}; + std::scoped_lock consumers_lock{consumers_mutex_}; for (const auto& [_, batch_callback] : consumers_) { // Make a copy of state change batch for every consumer because lifecycle is independent const std::optional frozen_batch = state_changes_; @@ -218,7 +218,7 @@ void StateChangeCollection::notify_batch(uint64_t pending_base_fee, uint64_t gas } void StateChangeCollection::close() { - std::unique_lock consumers_lock{consumers_mutex_}; + std::scoped_lock consumers_lock{consumers_mutex_}; for (const auto& [_, batch_callback] : consumers_) { SILK_DEBUG << "Notify close to callback=" << &batch_callback; batch_callback(std::nullopt); diff --git a/silkworm/db/mdbx/bitmap.cpp b/silkworm/db/mdbx/bitmap.cpp index 737392257d..db49722996 100644 --- a/silkworm/db/mdbx/bitmap.cpp +++ b/silkworm/db/mdbx/bitmap.cpp @@ -147,7 +147,7 @@ void IndexLoader::unwind_bitmaps_impl(RWTxn& txn, BlockNum to, const std::mapis_open()) { diff --git a/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.cpp b/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.cpp index 44de8a391d..351ab80955 100644 --- a/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.cpp +++ b/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.cpp @@ -95,7 +95,7 @@ evmc::bytes32 TrieLoader::calculate_root() { if (const auto now{std::chrono::steady_clock::now()}; log_time <= now) { SignalHandler::throw_if_signalled(); - std::unique_lock log_lck(log_mtx_); + std::scoped_lock log_lck(log_mtx_); log_key_ = to_hex(hashed_account_data_key_view, true); log_time = now + 2s; } diff --git a/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.hpp b/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.hpp index 242da23cfa..17f3a24e8f 100644 --- a/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.hpp +++ b/silkworm/node/stagedsync/stages/stage_interhashes/trie_loader.hpp @@ -37,7 +37,7 @@ class TrieLoader { //! \brief Returns the hex representation of current load key (for progress tracking) [[nodiscard]] std::string get_log_key() const { - std::unique_lock lock{log_mtx_}; + std::scoped_lock lock{log_mtx_}; return log_key_; } diff --git a/silkworm/rpc/core/filter_storage.cpp b/silkworm/rpc/core/filter_storage.cpp index 3674d3fff2..0d5f9a6b26 100644 --- a/silkworm/rpc/core/filter_storage.cpp +++ b/silkworm/rpc/core/filter_storage.cpp @@ -30,7 +30,7 @@ FilterStorage::FilterStorage(size_t max_size, double max_filter_age) : generator FilterStorage::FilterStorage(Generator& generator, size_t max_size, double max_filter_age) : generator_{generator}, max_size_{max_size}, max_filter_age_{max_filter_age} {} std::optional FilterStorage::add_filter(const StoredFilter& filter) { - std::lock_guard lock(mutex_); + std::scoped_lock lock{mutex_}; if (storage_.size() >= max_size_) { clean_up(); @@ -62,7 +62,7 @@ std::optional FilterStorage::add_filter(const StoredFilter& filter) } bool FilterStorage::remove_filter(const std::string& filter_id) { - std::lock_guard lock(mutex_); + std::scoped_lock lock{mutex_}; const auto itr = storage_.find(filter_id); if (itr == storage_.end()) { @@ -74,7 +74,7 @@ bool FilterStorage::remove_filter(const std::string& filter_id) { } std::optional> FilterStorage::get_filter(const std::string& filter_id) { - std::lock_guard lock(mutex_); + std::scoped_lock lock{mutex_}; clean_up();