From 7c6ee3f61fec6bc7ec3537274b18f2324c83d392 Mon Sep 17 00:00:00 2001 From: Teemu Ollakka Date: Mon, 15 Oct 2018 10:14:04 +0300 Subject: [PATCH] In order to avoid potential deadlocks, release client_state lock when calling server state methods which may acquire server_state mutex. Fixed compilation errors in release mode. --- include/wsrep/server_state.hpp | 4 +++- include/wsrep/transaction.hpp | 8 ++++---- src/client_state.cpp | 3 +-- src/server_state.cpp | 3 ++- src/transaction.cpp | 27 ++++++++++++++++++--------- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/include/wsrep/server_state.hpp b/include/wsrep/server_state.hpp index cba2c41..4924f2d 100644 --- a/include/wsrep/server_state.hpp +++ b/include/wsrep/server_state.hpp @@ -88,6 +88,7 @@ #include "view.hpp" #include "transaction_id.hpp" #include "provider.hpp" +#include "compiler.hpp" #include #include @@ -488,7 +489,8 @@ namespace wsrep return state(lock); } - enum state state(wsrep::unique_lock& lock) const + enum state state(wsrep::unique_lock& + lock WSREP_UNUSED) const { assert(lock.owns_lock()); return state_; diff --git a/include/wsrep/transaction.hpp b/include/wsrep/transaction.hpp index f7d9f01..7ebd1dd 100644 --- a/include/wsrep/transaction.hpp +++ b/include/wsrep/transaction.hpp @@ -18,8 +18,8 @@ */ /** @file transaction.hpp */ -#ifndef WSREP_TRANSACTION_CONTEXT_HPP -#define WSREP_TRANSACTION_CONTEXT_HPP +#ifndef WSREP_TRANSACTION_HPP +#define WSREP_TRANSACTION_HPP #include "provider.hpp" #include "server_state.hpp" @@ -175,7 +175,7 @@ namespace wsrep int certify_fragment(wsrep::unique_lock&); int certify_commit(wsrep::unique_lock&); int append_sr_keys_for_commit(); - void streaming_rollback(); + void streaming_rollback(wsrep::unique_lock&); void clear_fragments(); void cleanup(); void debug_log_state(const char*) const; @@ -227,4 +227,4 @@ namespace wsrep } -#endif // WSREP_TRANSACTION_CONTEXT_HPP +#endif // WSREP_TRANSACTION_HPP diff --git a/src/client_state.cpp b/src/client_state.cpp index b25ced8..b308ee9 100644 --- a/src/client_state.cpp +++ b/src/client_state.cpp @@ -314,13 +314,12 @@ int wsrep::client_state::enter_toi(const wsrep::ws_meta& ws_meta) int wsrep::client_state::leave_toi() { - int ret; + int ret(0); if (toi_mode_ == m_local) { switch (provider().leave_toi(id_)) { case wsrep::provider::success: - ret = 0; break; default: assert(0); diff --git a/src/server_state.cpp b/src/server_state.cpp index 2c7c181..8aafbaf 100644 --- a/src/server_state.cpp +++ b/src/server_state.cpp @@ -929,7 +929,8 @@ int wsrep::server_state::desync(wsrep::unique_lock& lock) return ret; } -void wsrep::server_state::resync(wsrep::unique_lock& lock) +void wsrep::server_state::resync(wsrep::unique_lock& + lock WSREP_UNUSED) { assert(lock.owns_lock()); assert(desync_count_ > 0); diff --git a/src/transaction.cpp b/src/transaction.cpp index e916fb5..d835b07 100644 --- a/src/transaction.cpp +++ b/src/transaction.cpp @@ -520,7 +520,9 @@ int wsrep::transaction::after_commit() client_state_.mode() == wsrep::client_state::m_high_priority); if (client_state_.mode() == wsrep::client_state::m_local) { + lock.unlock(); client_state_.server_state_.stop_streaming_client(&client_state_); + lock.lock(); } clear_fragments(); } @@ -573,7 +575,7 @@ int wsrep::transaction::before_rollback() // Voluntary rollback if (is_streaming()) { - streaming_rollback(); + streaming_rollback(lock); } state(lock, s_aborting); break; @@ -586,7 +588,7 @@ int wsrep::transaction::before_rollback() { if (is_streaming()) { - streaming_rollback(); + streaming_rollback(lock); } state(lock, s_aborting); } @@ -594,14 +596,14 @@ int wsrep::transaction::before_rollback() case s_cert_failed: if (is_streaming()) { - streaming_rollback(); + streaming_rollback(lock); } state(lock, s_aborting); break; case s_aborting: if (is_streaming()) { - streaming_rollback(); + streaming_rollback(lock); } break; case s_must_replay: @@ -899,7 +901,7 @@ bool wsrep::transaction::bf_abort( if (client_state_.mode() == wsrep::client_state::m_local && is_streaming() && state() == s_executing) { - streaming_rollback(); + streaming_rollback(lock); } if ((client_state_.state() == wsrep::client_state::s_idle && @@ -918,8 +920,10 @@ bool wsrep::transaction::bf_abort( state(lock, wsrep::transaction::s_aborting); if (client_state_.mode() == wsrep::client_state::m_high_priority) { + lock.unlock(); client_state_.server_state().stop_streaming_applier( server_id_, id_); + lock.lock(); } lock.unlock(); @@ -1170,11 +1174,13 @@ int wsrep::transaction::certify_fragment( { if (is_streaming() == false) { + lock.unlock(); client_state_.server_state_.stop_streaming_client(&client_state_); + lock.lock(); } else { - streaming_rollback(); + streaming_rollback(lock); } if (state_ != s_must_abort) { @@ -1186,7 +1192,7 @@ int wsrep::transaction::certify_fragment( { if (is_streaming()) { - streaming_rollback(); + streaming_rollback(lock); } client_state_.override_error(wsrep::e_deadlock_error, cert_ret); ret = 1; @@ -1389,7 +1395,7 @@ int wsrep::transaction::append_sr_keys_for_commit() return ret; } -void wsrep::transaction::streaming_rollback() +void wsrep::transaction::streaming_rollback(wsrep::unique_lock& lock) { debug_log_state("streaming_rollback enter"); assert(state_ != s_must_replay); @@ -1399,7 +1405,9 @@ void wsrep::transaction::streaming_rollback() { if (bf_aborted_in_total_order_) { + lock.unlock(); client_state_.server_state_.stop_streaming_client(&client_state_); + lock.lock(); streaming_context_.rolled_back(id_); } else @@ -1409,9 +1417,10 @@ void wsrep::transaction::streaming_rollback() // Adopt transaction will copy fragment set and appropriate // meta data. Mark current transaction streaming context // rolled back. + lock.unlock(); client_state_.server_state_.convert_streaming_client_to_applier( &client_state_); - + lock.lock(); streaming_context_.cleanup(); streaming_context_.rolled_back(id_); enum wsrep::provider::status ret;