Skip to content

Commit

Permalink
GH-2173 Fix deadlock, remove recursive mutex
Browse files Browse the repository at this point in the history
  • Loading branch information
@heifner
heifner committed Feb 1, 2024
1 parent 93ddc22 commit 3137035
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 47 deletions.
36 changes: 30 additions & 6 deletions libraries/chain/fork_database.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <fc/io/fstream.hpp>
#include <fc/io/cfile.hpp>
#include <fstream>
#include <shared_mutex>

namespace eosio::chain {
using boost::multi_index_container;
Expand Down Expand Up @@ -56,6 +57,7 @@ namespace eosio::chain {
BOOST_MULTI_INDEX_CONST_MEM_FUN(bs, const block_id_type&, id)>,
composite_key_compare<std::greater<bool>, std::greater<uint32_t>, std::greater<uint32_t>, sha256_less>>>>;

std::shared_mutex mtx;
fork_multi_index_type index;
bsp root; // Only uses the block_header_state_legacy portion
bsp head;
Expand Down Expand Up @@ -88,6 +90,7 @@ namespace eosio::chain {

template<class bsp>
void fork_database_t<bsp>::open( const std::filesystem::path& fork_db_file, validator_t& validator ) {
std::lock_guard g( my->mtx );
my->open_impl( fork_db_file, validator );
}

Expand Down Expand Up @@ -163,6 +166,7 @@ namespace eosio::chain {

template<class bsp>
void fork_database_t<bsp>::close(const std::filesystem::path& fork_db_file) {
std::lock_guard g( my->mtx );
my->close_impl(fork_db_file);
}

Expand Down Expand Up @@ -234,6 +238,7 @@ namespace eosio::chain {

template<class bsp>
void fork_database_t<bsp>::reset( const bhs& root_bhs ) {
std::lock_guard g( my->mtx );
my->reset_impl(root_bhs);
}

Expand All @@ -248,6 +253,7 @@ namespace eosio::chain {

template<class bsp>
void fork_database_t<bsp>::rollback_head_to_root() {
std::lock_guard g( my->mtx );
my->rollback_head_to_root_impl();
}

Expand All @@ -266,6 +272,7 @@ namespace eosio::chain {

template<class bsp>
void fork_database_t<bsp>::advance_root( const block_id_type& id ) {
std::lock_guard g( my->mtx );
my->advance_root_impl( id );
}

Expand Down Expand Up @@ -306,6 +313,7 @@ namespace eosio::chain {

template<class bsp>
fork_database_t<bsp>::bhsp fork_database_t<bsp>::get_block_header( const block_id_type& id ) const {
std::shared_lock g( my->mtx );
return my->get_block_header_impl( id );
}

Expand Down Expand Up @@ -362,6 +370,7 @@ namespace eosio::chain {

template<class bsp>
void fork_database_t<bsp>::add( const bsp& n, bool ignore_duplicate ) {
std::lock_guard g( my->mtx );
my->add_impl( n, ignore_duplicate, false,
[]( block_timestamp_type timestamp,
const flat_set<digest_type>& cur_features,
Expand All @@ -372,16 +381,19 @@ namespace eosio::chain {

template<class bsp>
bsp fork_database_t<bsp>::root() const {
std::shared_lock g( my->mtx );
return my->root;
}

template<class bsp>
bsp fork_database_t<bsp>::head() const {
std::shared_lock g( my->mtx );
return my->head;
}

template<class bsp>
bsp fork_database_t<bsp>::pending_head() const {
std::shared_lock g( my->mtx );
const auto& indx = my->index.template get<by_lib_block_num>();

auto itr = indx.lower_bound( false );
Expand All @@ -397,6 +409,7 @@ namespace eosio::chain {
fork_database_t<bsp>::branch_type
fork_database_t<bsp>::fetch_branch(const block_id_type& h,
uint32_t trim_after_block_num) const {
std::shared_lock g(my->mtx);
return my->fetch_branch_impl(h, trim_after_block_num);
}

Expand All @@ -414,6 +427,7 @@ namespace eosio::chain {

template<class bsp>
bsp fork_database_t<bsp>::search_on_branch( const block_id_type& h, uint32_t block_num ) const {
std::shared_lock g( my->mtx );
return my->search_on_branch_impl( h, block_num );
}

Expand All @@ -434,6 +448,7 @@ namespace eosio::chain {
template <class bsp>
fork_database_t<bsp>::branch_type_pair
fork_database_t<bsp>::fetch_branch_from(const block_id_type& first, const block_id_type& second) const {
std::shared_lock g(my->mtx);
return my->fetch_branch_from_impl(first, second);
}

Expand Down Expand Up @@ -500,6 +515,7 @@ namespace eosio::chain {
/// remove all of the invalid forks built off of this id including this id
template<class bsp>
void fork_database_t<bsp>::remove( const block_id_type& id ) {
std::lock_guard g( my->mtx );
return my->remove_impl( id );
}

Expand Down Expand Up @@ -527,6 +543,7 @@ namespace eosio::chain {

template<class bsp>
void fork_database_t<bsp>::mark_valid( const bsp& h ) {
std::lock_guard g( my->mtx );
my->mark_valid_impl( h );
}

Expand All @@ -553,6 +570,7 @@ namespace eosio::chain {

template<class bsp>
bsp fork_database_t<bsp>::get_block(const block_id_type& id) const {
std::shared_lock g( my->mtx );
return my->get_block_impl(id);
}

Expand All @@ -567,7 +585,10 @@ namespace eosio::chain {
// ------------------ fork_database -------------------------

fork_database::fork_database(const std::filesystem::path& data_dir)
: data_dir(data_dir) {
: data_dir(data_dir)
// currently needed because chain_head is accessed before fork database open
, fork_db_legacy{std::make_unique<fork_database_legacy_t>(fork_database_legacy_t::legacy_magic_number)}
{
}

fork_database::~fork_database() {
Expand All @@ -579,7 +600,6 @@ namespace eosio::chain {
}

void fork_database::open( validator_t& validator ) {
std::lock_guard g(m);
if (!std::filesystem::is_directory(data_dir))
std::filesystem::create_directories(data_dir);

Expand All @@ -603,12 +623,14 @@ namespace eosio::chain {
);

if (totem == fork_database_legacy_t::legacy_magic_number) {
// fork_db_legacy created in constructor
apply_legacy<void>([&](auto& forkdb) {
forkdb.open(fork_db_file, validator);
});
} else {
// file is instant-finality data, so switch to fork_database_if_t
vforkdb.emplace<fork_database_if_t>(fork_database_if_t::magic_number);
fork_db_if = std::make_unique<fork_database_if_t>(fork_database_if_t::magic_number);
legacy = false;
apply_if<void>([&](auto& forkdb) {
forkdb.open(fork_db_file, validator);
});
Expand All @@ -618,11 +640,13 @@ namespace eosio::chain {
}

void fork_database::switch_from_legacy() {
std::lock_guard g(m);
// no need to close fork_db because we don't want to write anything out, file is removed on open
block_state_legacy_ptr head = std::get<fork_database_legacy_t>(vforkdb).chain_head; // will throw if called after transistion
// threads may be accessing (or locked on mutex about to access legacy forkdb) so don't delete it until program exit
assert(legacy);
block_state_legacy_ptr head = fork_db_legacy->chain_head; // will throw if called after transistion
auto new_head = std::make_shared<block_state>(*head);
vforkdb.emplace<fork_database_if_t>(fork_database_if_t::magic_number);
fork_db_if = std::make_unique<fork_database_if_t>(fork_database_if_t::magic_number);
legacy = false;
apply_if<void>([&](auto& forkdb) {
forkdb.chain_head = new_head;
forkdb.reset(*new_head);
Expand Down
108 changes: 67 additions & 41 deletions libraries/chain/include/eosio/chain/fork_database.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ namespace eosio::chain {
* blocks older than the last irreversible block are freed after emitting the
* irreversible signal.
*
* Not thread safe, thread safety provided by fork_database below.
* An internal mutex is used to provide thread-safety.
*
* fork_database should be used instead of fork_database_t directly as it manages
* the different supported types.
*/
Expand Down Expand Up @@ -108,86 +109,111 @@ namespace eosio::chain {
using fork_database_if_t = fork_database_t<block_state_ptr>;

/**
* Provides thread safety on fork_database_t and provide mechanism for opening the correct type
* Provides mechanism for opening the correct type
* as well as switching from legacy (old dpos) to instant-finality.
*
* All methods assert until open() is closed.
*/
class fork_database {
mutable std::recursive_mutex m;
const std::filesystem::path data_dir;
std::variant<fork_database_t<block_state_legacy_ptr>, fork_database_t<block_state_ptr>> vforkdb;
std::atomic<bool> legacy = true;
std::unique_ptr<fork_database_legacy_t> fork_db_legacy;
std::unique_ptr<fork_database_if_t> fork_db_if;
public:
explicit fork_database(const std::filesystem::path& data_dir);
~fork_database(); // close on destruction

// not thread safe, expected to be called from main thread before allowing concurrent access
void open( validator_t& validator );
void close();

// expected to be called from main thread, accesses chain_head
void switch_from_legacy();

// see fork_database_t::fetch_branch(forkdb->head()->id())
std::vector<signed_block_ptr> fetch_branch_from_head();

template <class R, class F>
R apply(const F& f) {
std::lock_guard g(m);
if constexpr (std::is_same_v<void, R>)
std::visit([&](auto& forkdb) { f(forkdb); }, vforkdb);
else
return std::visit([&](auto& forkdb) -> R { return f(forkdb); }, vforkdb);
if constexpr (std::is_same_v<void, R>) {
if (legacy) {
f(*fork_db_legacy);
} else {
f(*fork_db_if);
}
} else {
if (legacy) {
return f(*fork_db_legacy);
} else {
return f(*fork_db_if);
}
}
}

template <class R, class F>
R apply(const F& f) const {
std::lock_guard g(m);
if constexpr (std::is_same_v<void, R>)
std::visit([&](const auto& forkdb) { f(forkdb); }, vforkdb);
else
return std::visit([&](const auto& forkdb) -> R { return f(forkdb); }, vforkdb);
if constexpr (std::is_same_v<void, R>) {
if (legacy) {
f(*fork_db_legacy);
} else {
f(*fork_db_if);
}
} else {
if (legacy) {
return f(*fork_db_legacy);
} else {
return f(*fork_db_if);
}
}
}

/// Apply for when only need lambda executed when in instant-finality mode
template <class R, class F>
R apply_if(const F& f) {
std::lock_guard g(m);
if constexpr (std::is_same_v<void, R>)
std::visit(overloaded{[&](fork_database_legacy_t&) {},
[&](fork_database_if_t& forkdb) { f(forkdb); }},
vforkdb);
else
return std::visit(overloaded{[&](fork_database_legacy_t&) -> R { return {}; },
[&](fork_database_if_t& forkdb) -> R { return f(forkdb); }},
vforkdb);
if constexpr (std::is_same_v<void, R>) {
if (!legacy) {
f(*fork_db_if);
}
} else {
if (!legacy) {
return f(*fork_db_if);
}
return {};
}
}

/// Apply for when only need lambda executed when in legacy mode
template <class R, class F>
R apply_legacy(const F& f) {
std::lock_guard g(m);
if constexpr (std::is_same_v<void, R>)
std::visit(overloaded{[&](fork_database_legacy_t& forkdb) { f(forkdb); },
[&](fork_database_if_t&) {}},
vforkdb);
else
return std::visit(overloaded{[&](fork_database_legacy_t& forkdb) -> R { return f(forkdb); },
[&](fork_database_if_t&) -> R { return {}; }},
vforkdb);
if constexpr (std::is_same_v<void, R>) {
if (legacy) {
f(*fork_db_legacy);
}
} else {
if (legacy) {
return f(*fork_db_legacy);
}
return {};
}
}

/// @param legacy_f the lambda to execute if in legacy mode
/// @param if_f the lambda to execute if in instant-finality mode
template <class R, class LegacyF, class IfF>
R apply(const LegacyF& legacy_f, const IfF& if_f) {
std::lock_guard g(m);
if constexpr (std::is_same_v<void, R>)
std::visit(overloaded{[&](fork_database_legacy_t& forkdb) { legacy_f(forkdb); },
[&](fork_database_if_t& forkdb) { if_f(forkdb); }},
vforkdb);
else
return std::visit(overloaded{[&](fork_database_legacy_t& forkdb) -> R { return legacy_f(forkdb); },
[&](fork_database_if_t& forkdb) -> R { return if_f(forkdb); }},
vforkdb);
if constexpr (std::is_same_v<void, R>) {
if (legacy) {
legacy_f(*fork_db_legacy);
} else {
if_f(*fork_db_if);
}
} else {
if (legacy) {
return legacy_f(*fork_db_legacy);
} else {
return if_f(*fork_db_if);
}
}
}

// if we ever support more than one version then need to save min/max in fork_database_t
Expand Down

0 comments on commit 3137035

Please sign in to comment.