From a9abb3a80ad55623c1ad8cef9a9028a237d74ca2 Mon Sep 17 00:00:00 2001 From: Teemu Ollakka Date: Wed, 17 Oct 2018 12:07:02 +0300 Subject: [PATCH 1/2] Extracted mock_server_service out of mock_server_state. --- include/wsrep/server_state.hpp | 5 +- test/client_state_fixture.hpp | 40 +++++++++++----- test/mock_server_state.hpp | 88 +++++++++++++++++++++------------- test/mock_storage_service.cpp | 2 +- test/mock_storage_service.hpp | 5 +- test/server_context_test.cpp | 43 +++++++---------- 6 files changed, 108 insertions(+), 75 deletions(-) diff --git a/include/wsrep/server_state.hpp b/include/wsrep/server_state.hpp index cba2c41..f95824d 100644 --- a/include/wsrep/server_state.hpp +++ b/include/wsrep/server_state.hpp @@ -270,7 +270,10 @@ namespace wsrep * * @throw wsrep::runtime_error if provider has not been loaded * - * @todo This should not be virtual. + * @todo This should not be virtual. However, currently there + * is no mechanism for tests and integrations to provide + * their own provider implementations, so this is kept virtual + * for time being. */ virtual wsrep::provider& provider() const { diff --git a/test/client_state_fixture.hpp b/test/client_state_fixture.hpp index dc01428..4ea2309 100644 --- a/test/client_state_fixture.hpp +++ b/test/client_state_fixture.hpp @@ -31,7 +31,8 @@ namespace struct replicating_client_fixture_sync_rm { replicating_client_fixture_sync_rm() - : sc("s1", "s1", wsrep::server_state::rm_sync) + : server_service(sc) + , sc("s1", "s1", wsrep::server_state::rm_sync, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_local) , tc(cc.transaction()) @@ -43,6 +44,7 @@ namespace BOOST_REQUIRE(tc.active() == false); BOOST_REQUIRE(tc.state() == wsrep::transaction::s_executing); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -51,7 +53,8 @@ namespace struct replicating_client_fixture_async_rm { replicating_client_fixture_async_rm() - : sc("s1", "s1", wsrep::server_state::rm_async) + : server_service(sc) + , sc("s1", "s1", wsrep::server_state::rm_async, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_local) , tc(cc.transaction()) @@ -63,6 +66,7 @@ namespace BOOST_REQUIRE(tc.active() == false); BOOST_REQUIRE(tc.state() == wsrep::transaction::s_executing); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -71,7 +75,8 @@ namespace struct replicating_client_fixture_2pc { replicating_client_fixture_2pc() - : sc("s1", "s1", wsrep::server_state::rm_sync) + : server_service(sc) + , sc("s1", "s1", wsrep::server_state::rm_sync, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_local) , tc(cc.transaction()) @@ -84,6 +89,7 @@ namespace BOOST_REQUIRE(tc.active() == false); BOOST_REQUIRE(tc.state() == wsrep::transaction::s_executing); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -92,7 +98,8 @@ namespace struct replicating_client_fixture_autocommit { replicating_client_fixture_autocommit() - : sc("s1", "s1", wsrep::server_state::rm_sync) + : server_service(sc) + , sc("s1", "s1", wsrep::server_state::rm_sync, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_local) , tc(cc.transaction()) @@ -105,6 +112,7 @@ namespace BOOST_REQUIRE(tc.active() == false); BOOST_REQUIRE(tc.state() == wsrep::transaction::s_executing); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -113,8 +121,9 @@ namespace struct applying_client_fixture { applying_client_fixture() - : sc("s1", "s1", - wsrep::server_state::rm_async) + : server_service(sc) + , sc("s1", "s1", + wsrep::server_state::rm_async, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_high_priority) @@ -136,6 +145,7 @@ namespace BOOST_REQUIRE(tc.certified() == true); BOOST_REQUIRE(tc.ordered() == true); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -144,8 +154,9 @@ namespace struct applying_client_fixture_2pc { applying_client_fixture_2pc() - : sc("s1", "s1", - wsrep::server_state::rm_async) + : server_service(sc) + , sc("s1", "s1", + wsrep::server_state::rm_async, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_high_priority) @@ -168,6 +179,7 @@ namespace BOOST_REQUIRE(tc.certified() == true); BOOST_REQUIRE(tc.ordered() == true); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -176,7 +188,8 @@ namespace struct streaming_client_fixture_row { streaming_client_fixture_row() - : sc("s1", "s1", wsrep::server_state::rm_sync) + : server_service(sc) + , sc("s1", "s1", wsrep::server_state::rm_sync, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_local) @@ -190,6 +203,7 @@ namespace BOOST_REQUIRE(tc.state() == wsrep::transaction::s_executing); cc.enable_streaming(wsrep::streaming_context::row, 1); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -198,7 +212,8 @@ namespace struct streaming_client_fixture_byte { streaming_client_fixture_byte() - : sc("s1", "s1", wsrep::server_state::rm_sync) + : server_service(sc) + , sc("s1", "s1", wsrep::server_state::rm_sync, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_local) @@ -212,6 +227,7 @@ namespace BOOST_REQUIRE(tc.state() == wsrep::transaction::s_executing); cc.enable_streaming(wsrep::streaming_context::bytes, 1); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; @@ -220,7 +236,8 @@ namespace struct streaming_client_fixture_statement { streaming_client_fixture_statement() - : sc("s1", "s1", wsrep::server_state::rm_sync) + : server_service(sc) + , sc("s1", "s1", wsrep::server_state::rm_sync, server_service) , cc(sc, wsrep::client_id(1), wsrep::client_state::m_local) @@ -235,6 +252,7 @@ namespace cc.enable_streaming(wsrep::streaming_context::row, 1); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; const wsrep::transaction& tc; diff --git a/test/mock_server_state.hpp b/test/mock_server_state.hpp index ba8088c..a9b14c1 100644 --- a/test/mock_server_state.hpp +++ b/test/mock_server_state.hpp @@ -17,8 +17,8 @@ * along with wsrep-lib. If not, see . */ -#ifndef WSREP_MOCK_SERVER_CONTEXT_HPP -#define WSREP_MOCK_SERVER_CONTEXT_HPP +#ifndef WSREP_MOCK_SERVER_STATE_HPP +#define WSREP_MOCK_SERVER_STATE_HPP #include "wsrep/server_state.hpp" #include "wsrep/server_service.hpp" @@ -31,50 +31,42 @@ namespace wsrep { - class mock_server_state - : public wsrep::server_state - , public wsrep::server_service + class mock_server_service : public wsrep::server_service { public: - mock_server_state(const std::string& name, - const std::string& id, - enum wsrep::server_state::rollback_mode rollback_mode) - : wsrep::server_state(mutex_, cond_, *this, - name, id, "", "", "./", wsrep::gtid::undefined(), 1, rollback_mode) - , sync_point_enabled_() + mock_server_service(wsrep::server_state& server_state) + : sync_point_enabled_() , sync_point_action_() , sst_before_init_() - , mutex_() - , cond_() - , provider_(*this) + , server_state_(server_state) , last_client_id_(0) , last_transaction_id_(0) { } - wsrep::mock_provider& provider() const - { return provider_; } - wsrep::storage_service* storage_service(wsrep::client_service&) + WSREP_OVERRIDE { - return new wsrep::mock_storage_service(*this, + return new wsrep::mock_storage_service(server_state_, wsrep::client_id(++last_client_id_)); } wsrep::storage_service* storage_service(wsrep::high_priority_service&) + WSREP_OVERRIDE { - return new wsrep::mock_storage_service(*this, + return new wsrep::mock_storage_service(server_state_, wsrep::client_id(++last_client_id_)); } void release_storage_service(wsrep::storage_service* storage_service) + WSREP_OVERRIDE { delete storage_service; } - wsrep::client_state* local_client_state() + wsrep::client_state* local_client_state() WSREP_OVERRIDE { wsrep::client_state* ret(new wsrep::mock_client( - *this, + server_state_, wsrep::client_id(++last_client_id_), wsrep::client_state::m_local)); ret->open(ret->id()); @@ -82,33 +74,37 @@ namespace wsrep } void release_client_state(wsrep::client_state* client_state) + WSREP_OVERRIDE { delete client_state; } wsrep::high_priority_service* streaming_applier_service( wsrep::client_service&) + WSREP_OVERRIDE { wsrep::mock_client* cs(new wsrep::mock_client( - *this, + server_state_, wsrep::client_id(++last_client_id_), wsrep::client_state::m_high_priority)); wsrep::mock_high_priority_service* ret( - new wsrep::mock_high_priority_service(*this, cs, false)); + new wsrep::mock_high_priority_service(server_state_, + cs, false)); cs->open(cs->id()); cs->before_command(); return ret; } wsrep::high_priority_service* streaming_applier_service( - wsrep::high_priority_service&) + wsrep::high_priority_service&) WSREP_OVERRIDE { wsrep::mock_client* cs(new wsrep::mock_client( - *this, + server_state_, wsrep::client_id(++last_client_id_), wsrep::client_state::m_high_priority)); wsrep::mock_high_priority_service* ret( - new wsrep::mock_high_priority_service(*this, cs, false)); + new wsrep::mock_high_priority_service(server_state_, + cs, false)); cs->open(cs->id()); cs->before_command(); return ret; @@ -130,15 +126,17 @@ namespace wsrep } void bootstrap() WSREP_OVERRIDE { } void log_message(enum wsrep::log::level level, const char* message) + WSREP_OVERRIDE { - wsrep::log(level, name().c_str()) << message; + wsrep::log(level, server_state_.name().c_str()) << message; } void log_dummy_write_set(wsrep::client_state&, const wsrep::ws_meta&) WSREP_OVERRIDE { } - void log_view(wsrep::high_priority_service*, const wsrep::view&) { } + void log_view(wsrep::high_priority_service*, const wsrep::view&) + WSREP_OVERRIDE { } void log_state_change(enum wsrep::server_state::state, enum wsrep::server_state::state) { } @@ -169,7 +167,7 @@ namespace wsrep switch (sync_point_action_) { case spa_initialize: - initialized(); + server_state_.initialized(); break; } } @@ -183,12 +181,36 @@ namespace wsrep bool sst_before_init_; private: - wsrep::default_mutex mutex_; - wsrep::default_condition_variable cond_; - mutable wsrep::mock_provider provider_; + wsrep::server_state& server_state_; unsigned long long last_client_id_; unsigned long long last_transaction_id_; }; + + + class mock_server_state : public wsrep::server_state + { + public: + mock_server_state(const std::string& name, + const std::string& id, + enum wsrep::server_state::rollback_mode rollback_mode, + wsrep::server_service& server_service) + : wsrep::server_state(mutex_, cond_, server_service, + name, id, "", "", "./", + wsrep::gtid::undefined(), + 1, + rollback_mode) + , mutex_() + , cond_() + , provider_(*this) + { } + + wsrep::mock_provider& provider() const WSREP_OVERRIDE + { return provider_; } + private: + wsrep::default_mutex mutex_; + wsrep::default_condition_variable cond_; + mutable wsrep::mock_provider provider_; + }; } -#endif // WSREP_MOCK_SERVER_CONTEXT_HPP +#endif // WSREP_MOCK_SERVER_STATE_HPP diff --git a/test/mock_storage_service.cpp b/test/mock_storage_service.cpp index 0083159..da5477b 100644 --- a/test/mock_storage_service.cpp +++ b/test/mock_storage_service.cpp @@ -23,7 +23,7 @@ #include "wsrep/client_state.hpp" wsrep::mock_storage_service::mock_storage_service( - wsrep::mock_server_state& server_state, + wsrep::server_state& server_state, wsrep::client_id client_id) : server_state_(server_state) , client_service_(client_state_) diff --git a/test/mock_storage_service.hpp b/test/mock_storage_service.hpp index e9c54db..9771655 100644 --- a/test/mock_storage_service.hpp +++ b/test/mock_storage_service.hpp @@ -29,8 +29,7 @@ class mock_server_state; class mock_storage_service : public wsrep::storage_service { public: - mock_storage_service(wsrep::mock_server_state&, - wsrep::client_id); + mock_storage_service(wsrep::server_state&, wsrep::client_id); ~mock_storage_service(); int start_transaction(const wsrep::ws_handle&) WSREP_OVERRIDE; @@ -56,7 +55,7 @@ class mock_server_state; void store_globals() WSREP_OVERRIDE { } void reset_globals() WSREP_OVERRIDE { } private: - wsrep::mock_server_state& server_state_; + wsrep::server_state& server_state_; wsrep::mock_client_service client_service_; wsrep::mock_client_state client_state_; }; diff --git a/test/server_context_test.cpp b/test/server_context_test.cpp index 68535ed..18e504d 100644 --- a/test/server_context_test.cpp +++ b/test/server_context_test.cpp @@ -26,8 +26,9 @@ namespace struct applying_server_fixture { applying_server_fixture() - : ss("s1", "s1", - wsrep::server_state::rm_sync) + : server_service(ss) + , ss("s1", "s1", + wsrep::server_state::rm_sync, server_service) , cc(ss, wsrep::client_id(1), wsrep::client_state::m_high_priority) @@ -41,8 +42,9 @@ namespace wsrep::provider::flag::commit) { cc.open(cc.id()); - cc.before_command(); + BOOST_REQUIRE(cc.before_command() == 0); } + wsrep::mock_server_service server_service; wsrep::mock_server_state ss; wsrep::mock_client cc; wsrep::mock_high_priority_service hps; @@ -55,7 +57,7 @@ namespace sst_first_server_fixture() : applying_server_fixture() { - ss.sst_before_init_ = true; + server_service.sst_before_init_ = true; } }; @@ -64,10 +66,9 @@ namespace init_first_server_fixture() : applying_server_fixture() { - ss.sst_before_init_ = false; + server_service.sst_before_init_ = false; } }; - } // Test on_apply() method for 1pc @@ -121,24 +122,14 @@ BOOST_FIXTURE_TEST_CASE(server_state_applying_2pc_rollback, BOOST_REQUIRE(txc.state() == wsrep::transaction::s_aborted); } -BOOST_AUTO_TEST_CASE(server_state_streaming) +BOOST_FIXTURE_TEST_CASE(server_state_streaming, applying_server_fixture) { - wsrep::mock_server_state ss("s1", "s1", - wsrep::server_state::rm_sync); - wsrep::mock_client cc(ss, - wsrep::client_id(1), - wsrep::client_state::m_high_priority); - cc.debug_log_level(1); - wsrep::mock_high_priority_service hps(ss, &cc, false); - wsrep::ws_handle ws_handle(wsrep::transaction_id(1), (void*)1); - wsrep::ws_meta ws_meta(wsrep::gtid(wsrep::id("1"), wsrep::seqno(1)), - wsrep::stid(wsrep::id("1"), - wsrep::transaction_id(1), - wsrep::client_id(1)), - wsrep::seqno(0), - wsrep::provider::flag::start_transaction); - cc.open(cc.id()); - BOOST_REQUIRE(cc.before_command() == 0); + ws_meta = wsrep::ws_meta(wsrep::gtid(wsrep::id("1"), wsrep::seqno(1)), + wsrep::stid(wsrep::id("1"), + wsrep::transaction_id(1), + wsrep::client_id(1)), + wsrep::seqno(0), + wsrep::provider::flag::start_transaction); BOOST_REQUIRE(ss.on_apply(hps, ws_handle, ws_meta, wsrep::const_buffer("1", 1)) == 0); BOOST_REQUIRE(ss.find_streaming_applier( @@ -205,10 +196,10 @@ BOOST_FIXTURE_TEST_CASE(server_state_sst_first_boostrap, BOOST_REQUIRE(ss.connect("cluster", "local", "0", false) == 0); ss.on_connect(wsrep::gtid(cluster_id, wsrep::seqno(0))); BOOST_REQUIRE(ss.state() == wsrep::server_state::s_connected); - ss.sync_point_enabled_ = "on_view_wait_initialized"; - ss.sync_point_action_ = ss.spa_initialize; + server_service.sync_point_enabled_ = "on_view_wait_initialized"; + server_service.sync_point_action_ = server_service.spa_initialize; ss.on_view(bootstrap_view, &hps); - ss.sync_point_enabled_ = ""; + server_service.sync_point_enabled_ = ""; BOOST_REQUIRE(ss.state() == wsrep::server_state::s_joined); ss.on_sync(); BOOST_REQUIRE(ss.state() == wsrep::server_state::s_synced); From cb1ca4e66e3d0c6816acc6833c2e078d08b80e0c Mon Sep 17 00:00:00 2001 From: Teemu Ollakka Date: Wed, 17 Oct 2018 12:51:01 +0300 Subject: [PATCH 2/2] Rollback and release streaming appliers in unit tests Unit tests which cause streaming rollback leaked memory because the streaming applier handle which was created for rollback fragment handling was not released. Roll back a streaming transaction and release applier handle appropriately in corresponding tests. --- test/client_state_fixture.hpp | 1 + test/mock_server_state.hpp | 5 +++-- test/transaction_test.cpp | 36 +++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/test/client_state_fixture.hpp b/test/client_state_fixture.hpp index 4ea2309..6073d25 100644 --- a/test/client_state_fixture.hpp +++ b/test/client_state_fixture.hpp @@ -203,6 +203,7 @@ namespace BOOST_REQUIRE(tc.state() == wsrep::transaction::s_executing); cc.enable_streaming(wsrep::streaming_context::row, 1); } + wsrep::mock_server_service server_service; wsrep::mock_server_state sc; wsrep::mock_client cc; diff --git a/test/mock_server_state.hpp b/test/mock_server_state.hpp index a9b14c1..7392d08 100644 --- a/test/mock_server_state.hpp +++ b/test/mock_server_state.hpp @@ -115,8 +115,9 @@ namespace wsrep WSREP_OVERRIDE { mock_high_priority_service* mhps( - reinterpret_cast(high_priority_service)); - wsrep::client_state* cs(mhps->client_state()); + static_cast(high_priority_service)); + wsrep::mock_client* cs(static_cast( + mhps->client_state())); cs->after_command_before_result(); cs->after_command_after_result(); cs->close(); diff --git a/test/transaction_test.cpp b/test/transaction_test.cpp index 46ec414..3b78e7d 100644 --- a/test/transaction_test.cpp +++ b/test/transaction_test.cpp @@ -1067,6 +1067,15 @@ BOOST_FIXTURE_TEST_CASE(transaction_row_streaming_rollback, BOOST_REQUIRE(sc.provider().fragments() == 2); BOOST_REQUIRE(sc.provider().start_fragments() == 1); BOOST_REQUIRE(sc.provider().rollback_fragments() == 1); + + wsrep::high_priority_service* hps( + sc.find_streaming_applier( + sc.id(), wsrep::transaction_id(1))); + BOOST_REQUIRE(hps); + hps->rollback(wsrep::ws_handle(), wsrep::ws_meta()); + hps->after_apply(); + sc.stop_streaming_applier(sc.id(), wsrep::transaction_id(1)); + server_service.release_high_priority_service(hps); } // @@ -1087,6 +1096,15 @@ BOOST_FIXTURE_TEST_CASE(transaction_row_streaming_cert_fail_non_commit, BOOST_REQUIRE(sc.provider().fragments() == 2); BOOST_REQUIRE(sc.provider().start_fragments() == 1); BOOST_REQUIRE(sc.provider().rollback_fragments() == 1); + + wsrep::high_priority_service* hps( + sc.find_streaming_applier( + sc.id(), wsrep::transaction_id(1))); + BOOST_REQUIRE(hps); + hps->rollback(wsrep::ws_handle(), wsrep::ws_meta()); + hps->after_apply(); + sc.stop_streaming_applier(sc.id(), wsrep::transaction_id(1)); + server_service.release_high_priority_service(hps); } // @@ -1109,6 +1127,15 @@ BOOST_FIXTURE_TEST_CASE(transaction_row_streaming_cert_fail_commit, BOOST_REQUIRE(sc.provider().fragments() == 2); BOOST_REQUIRE(sc.provider().start_fragments() == 1); BOOST_REQUIRE(sc.provider().rollback_fragments() == 1); + + wsrep::high_priority_service* hps( + sc.find_streaming_applier( + sc.id(), wsrep::transaction_id(1))); + BOOST_REQUIRE(hps); + hps->rollback(wsrep::ws_handle(), wsrep::ws_meta()); + hps->after_apply(); + sc.stop_streaming_applier(sc.id(), wsrep::transaction_id(1)); + server_service.release_high_priority_service(hps); } // @@ -1243,6 +1270,15 @@ BOOST_FIXTURE_TEST_CASE(transaction_statement_streaming_cert_fail, BOOST_REQUIRE(sc.provider().fragments() == 1); BOOST_REQUIRE(sc.provider().start_fragments() == 0); BOOST_REQUIRE(sc.provider().rollback_fragments() == 1); + + wsrep::high_priority_service* hps( + sc.find_streaming_applier( + sc.id(), wsrep::transaction_id(1))); + BOOST_REQUIRE(hps); + hps->rollback(wsrep::ws_handle(), wsrep::ws_meta()); + hps->after_apply(); + sc.stop_streaming_applier(sc.id(), wsrep::transaction_id(1)); + server_service.release_high_priority_service(hps); } ///////////////////////////////////////////////////////////////////////////////