diff --git a/include/wsrep/client_context.hpp b/include/wsrep/client_context.hpp index c598a6e..205b20c 100644 --- a/include/wsrep/client_context.hpp +++ b/include/wsrep/client_context.hpp @@ -44,6 +44,7 @@ #include "mutex.hpp" #include "lock.hpp" #include "data.hpp" +#include "thread.hpp" namespace wsrep { @@ -359,7 +360,8 @@ namespace wsrep wsrep::server_context& server_context, const client_id& id, enum mode mode) - : mutex_(mutex) + : thread_id_(wsrep::this_thread::get_id()) + , mutex_(mutex) , server_context_(server_context) , id_(id) , mode_(mode) @@ -473,10 +475,15 @@ namespace wsrep */ virtual void abort() const = 0; + public: /*! * Set up thread global variables for client connection. */ - virtual void store_globals() = 0; + virtual void store_globals() + { + thread_id_ = wsrep::this_thread::get_id(); + } + private: /*! * Enter debug synchronization point. @@ -495,16 +502,9 @@ namespace wsrep /*! * */ - void override_error(enum wsrep::client_error error) - { - if (current_error_ != wsrep::e_success && - error == wsrep::e_success) - { - throw wsrep::runtime_error("Overriding error with success"); - } - current_error_ = error; - } + void override_error(enum wsrep::client_error error); + wsrep::thread::id thread_id_; wsrep::mutex& mutex_; wsrep::server_context& server_context_; client_id id_; diff --git a/include/wsrep/thread.hpp b/include/wsrep/thread.hpp new file mode 100644 index 0000000..f76e9dc --- /dev/null +++ b/include/wsrep/thread.hpp @@ -0,0 +1,36 @@ +// +// Copyright (C) 2018 Codership Oy +// + +#include + +namespace wsrep +{ + class thread + { + public: + class id + { + public: + id() : thread_() { } + explicit id(pthread_t thread) : thread_(thread) { } + private: + friend bool operator==(thread::id left, thread::id right) + { + return (pthread_equal(left.thread_, right.thread_)); + } + pthread_t thread_; + }; + + thread() + : id_(pthread_self()) + { } + private: + id id_; + }; + + namespace this_thread + { + static inline thread::id get_id() { return thread::id(pthread_self()); } + } +}; diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 680df7d..039a2be 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -15,7 +15,7 @@ target_link_libraries(wsrep-lib wsrep_api_v26 pthread dl) add_executable(wsrep-lib_test mock_client_context.cpp - mock_utils.cpp + test_utils.cpp client_context_test.cpp server_context_test.cpp transaction_context_test.cpp diff --git a/src/client_context.cpp b/src/client_context.cpp index 943bc40..58c1b5e 100644 --- a/src/client_context.cpp +++ b/src/client_context.cpp @@ -14,13 +14,24 @@ wsrep::provider& wsrep::client_context::provider() const return server_context_.provider(); } +void wsrep::client_context::override_error(enum wsrep::client_error error) +{ + assert(wsrep::this_thread::get_id() == thread_id_); + if (current_error_ != wsrep::e_success && + error == wsrep::e_success) + { + throw wsrep::runtime_error("Overriding error with success"); + } + current_error_ = error; +} + + int wsrep::client_context::before_command() { wsrep::unique_lock lock(mutex_); assert(state_ == s_idle); if (server_context_.rollback_mode() == wsrep::server_context::rm_sync) { - /*! * \todo Wait until the possible synchronous rollback * has been finished. @@ -31,11 +42,20 @@ int wsrep::client_context::before_command() } } state(lock, s_exec); + assert(transaction_.active() == false || + (transaction_.state() == wsrep::transaction_context::s_executing || + transaction_.state() == wsrep::transaction_context::s_aborted)); + + // Transaction was rolled back either just before sending result + // to the client, or after client_context become idle. + // Clean up the transaction and return error. if (transaction_.active() && - (transaction_.state() == wsrep::transaction_context::s_must_abort || - transaction_.state() == wsrep::transaction_context::s_aborted)) + transaction_.state() == wsrep::transaction_context::s_aborted) { override_error(wsrep::e_deadlock_error); + lock.unlock(); + (void)transaction_.after_statement(); + lock.lock(); return 1; } return 0; @@ -51,7 +71,7 @@ void wsrep::client_context::after_command_before_result() override_error(wsrep::e_deadlock_error); lock.unlock(); rollback(); - transaction_.after_statement(); + (void)transaction_.after_statement(); lock.lock(); assert(transaction_.state() == wsrep::transaction_context::s_aborted); assert(current_error() != wsrep::e_success); @@ -63,27 +83,18 @@ void wsrep::client_context::after_command_after_result() { wsrep::unique_lock lock(mutex_); assert(state() == s_result); + assert(transaction_.state() != wsrep::transaction_context::s_aborting); if (transaction_.active() && transaction_.state() == wsrep::transaction_context::s_must_abort) { - // Note: Error is not overridden here as the result has already - // been sent to client. The error should be set in before_command() - // when the client issues next command and finds the transaction - // in aborted state. lock.unlock(); rollback(); - transaction_.after_statement(); lock.lock(); assert(transaction_.state() == wsrep::transaction_context::s_aborted); - assert(current_error() == wsrep::e_success); + override_error(wsrep::e_deadlock_error); } - else if (transaction_.active() && - transaction_.state() == wsrep::transaction_context::s_aborted) + else if (transaction_.active() == false) { - // Will clean up the transaction - lock.unlock(); - (void)transaction_.after_statement(); - lock.lock(); current_error_ = wsrep::e_success; } state(lock, s_idle); @@ -147,6 +158,7 @@ void wsrep::client_context::state( wsrep::unique_lock& lock WSREP_UNUSED, enum wsrep::client_context::state state) { + assert(wsrep::this_thread::get_id() == thread_id_); assert(lock.owns_lock()); static const char allowed[state_max_][state_max_] = { diff --git a/src/client_context_test.cpp b/src/client_context_test.cpp index 36c07f3..2e73547 100644 --- a/src/client_context_test.cpp +++ b/src/client_context_test.cpp @@ -5,7 +5,7 @@ #include "mock_client_context.hpp" #include "mock_server_context.hpp" -#include "mock_utils.hpp" +#include "test_utils.hpp" #include @@ -22,7 +22,7 @@ BOOST_AUTO_TEST_CASE(client_context_test_error_codes) BOOST_REQUIRE(txc.active() == false); cc.start_transaction(1); - wsrep_mock::bf_abort_unordered(cc); + wsrep_test::bf_abort_unordered(cc); cc.after_statement(); cc.after_command_before_result(); diff --git a/src/dbms_simulator.cpp b/src/dbms_simulator.cpp index 2714aa3..cde8a8b 100644 --- a/src/dbms_simulator.cpp +++ b/src/dbms_simulator.cpp @@ -380,7 +380,12 @@ private: { } bool killed() const override { return false; } void abort() const override { ::abort(); } - void store_globals() override { } +public: + void store_globals() override + { + wsrep::client_context::store_globals(); + } +private: void debug_sync(const char*) override { } void debug_suicide(const char*) override { } void on_error(enum wsrep::client_error) override { } @@ -528,6 +533,7 @@ void dbms_server::stop_clients() void dbms_server::client_thread(const std::shared_ptr& client) { + client->store_globals(); client->start(); } diff --git a/src/mock_utils.cpp b/src/mock_utils.cpp deleted file mode 100644 index 1566321..0000000 --- a/src/mock_utils.cpp +++ /dev/null @@ -1,44 +0,0 @@ -// -// Copyright (C) 2018 Codership Oy -// - -#include "mock_utils.hpp" -#include "wsrep/client_context.hpp" -#include "mock_server_context.hpp" - - -// Simple BF abort method to BF abort unordered transasctions -void wsrep_mock::bf_abort_unordered(wsrep::client_context& cc) -{ - wsrep::unique_lock lock(cc.mutex()); - assert(cc.transaction().seqno().nil()); - cc.bf_abort(lock, 1); -} - - // BF abort method to abort transactions via provider -void wsrep_mock::bf_abort_provider(wsrep::mock_server_context& sc, - const wsrep::transaction_context& tc, - wsrep::seqno bf_seqno) -{ - wsrep::seqno victim_seqno; - sc.provider().bf_abort(bf_seqno, tc.id(), victim_seqno); - (void)victim_seqno; -} - -void wsrep_mock::start_applying_transaction( - wsrep::client_context& cc, - const wsrep::transaction_id& id, - wsrep::seqno seqno, - int flags) -{ - wsrep::ws_handle ws_handle(id, 0); - wsrep::ws_meta ws_meta(wsrep::gtid(wsrep::id("1"), seqno), - wsrep::stid(wsrep::id("1"), id, cc.id()), - seqno.get() - 1, flags); - assert(ws_meta.flags()); - int ret(cc.start_transaction(ws_handle, ws_meta)); - if (ret != 0) - { - throw wsrep::runtime_error("failed to start applying transaction"); - } -} diff --git a/src/server_context_test.cpp b/src/server_context_test.cpp index f8f0edf..1bbb20a 100644 --- a/src/server_context_test.cpp +++ b/src/server_context_test.cpp @@ -3,7 +3,6 @@ // #include "mock_server_context.hpp" -#include "mock_utils.hpp" #include @@ -25,10 +24,6 @@ namespace wsrep::provider::flag::start_transaction | wsrep::provider::flag::commit) { - // wsrep_mock::start_applying_transaction( - // cc, 1, 1, - // wsrep::provider::flag::start_transaction | - // wsrep::provider::flag::commit); cc.start_transaction(ws_handle, ws_meta); } wsrep::mock_server_context sc; diff --git a/src/test_utils.cpp b/src/test_utils.cpp new file mode 100644 index 0000000..47e7089 --- /dev/null +++ b/src/test_utils.cpp @@ -0,0 +1,26 @@ +// +// Copyright (C) 2018 Codership Oy +// + +#include "test_utils.hpp" +#include "wsrep/client_context.hpp" +#include "mock_server_context.hpp" + + +// Simple BF abort method to BF abort unordered transasctions +void wsrep_test::bf_abort_unordered(wsrep::client_context& cc) +{ + wsrep::unique_lock lock(cc.mutex()); + assert(cc.transaction().seqno().nil()); + cc.bf_abort(lock, 1); +} + + // BF abort method to abort transactions via provider +void wsrep_test::bf_abort_provider(wsrep::mock_server_context& sc, + const wsrep::transaction_context& tc, + wsrep::seqno bf_seqno) +{ + wsrep::seqno victim_seqno; + sc.provider().bf_abort(bf_seqno, tc.id(), victim_seqno); + (void)victim_seqno; +} diff --git a/src/mock_utils.hpp b/src/test_utils.hpp similarity index 77% rename from src/mock_utils.hpp rename to src/test_utils.hpp index 273c7f3..00af57d 100644 --- a/src/mock_utils.hpp +++ b/src/test_utils.hpp @@ -15,7 +15,7 @@ namespace wsrep // // Utility functions // -namespace wsrep_mock +namespace wsrep_test { // Simple BF abort method to BF abort unordered transasctions @@ -26,9 +26,4 @@ namespace wsrep_mock const wsrep::transaction_context& tc, wsrep::seqno bf_seqno); - void start_applying_transaction( - wsrep::client_context& cc, - const wsrep::transaction_id& id, - wsrep::seqno seqno, - int flags); } diff --git a/src/transaction_context_test.cpp b/src/transaction_context_test.cpp index 20e390b..8a9ad26 100644 --- a/src/transaction_context_test.cpp +++ b/src/transaction_context_test.cpp @@ -8,7 +8,7 @@ #include "mock_client_context.hpp" #include "mock_server_context.hpp" -#include "mock_utils.hpp" +#include "test_utils.hpp" #include @@ -45,10 +45,13 @@ namespace { BOOST_REQUIRE(cc.before_command() == 0); BOOST_REQUIRE(cc.before_statement() == 0); - wsrep_mock::start_applying_transaction( - cc, 1, 1, - wsrep::provider::flag::start_transaction | - wsrep::provider::flag::commit); + wsrep::ws_handle ws_handle(1, (void*)1); + wsrep::ws_meta ws_meta(wsrep::gtid(wsrep::id("1"), 1), + wsrep::stid(sc.id(), 1, cc.id()), + 0, + wsrep::provider::flag::start_transaction | + wsrep::provider::flag::commit); + BOOST_REQUIRE(cc.start_transaction(ws_handle, ws_meta) == 0); BOOST_REQUIRE(tc.active() == false); BOOST_REQUIRE(cc.start_transaction() == 0); BOOST_REQUIRE(tc.active() == true); @@ -173,7 +176,7 @@ BOOST_FIXTURE_TEST_CASE(transaction_context_1pc_bf_before_before_commit, BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - wsrep_mock::bf_abort_unordered(cc); + wsrep_test::bf_abort_unordered(cc); // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -207,7 +210,7 @@ BOOST_FIXTURE_TEST_CASE(transaction_context_2pc_bf_before_before_prepare, BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - wsrep_mock::bf_abort_unordered(cc); + wsrep_test::bf_abort_unordered(cc); // Run before commit BOOST_REQUIRE(cc.before_prepare()); @@ -245,7 +248,7 @@ BOOST_FIXTURE_TEST_CASE(transaction_context_2pc_bf_before_after_prepare, BOOST_REQUIRE(cc.before_prepare() == 0); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_preparing); - wsrep_mock::bf_abort_unordered(cc); + wsrep_test::bf_abort_unordered(cc); // Run before commit BOOST_REQUIRE(cc.after_prepare()); @@ -281,7 +284,7 @@ BOOST_FIXTURE_TEST_CASE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - wsrep_mock::bf_abort_provider(sc, tc, wsrep::seqno::undefined()); + wsrep_test::bf_abort_provider(sc, tc, wsrep::seqno::undefined()); // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -316,7 +319,7 @@ BOOST_FIXTURE_TEST_CASE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - wsrep_mock::bf_abort_unordered(cc); + wsrep_test::bf_abort_unordered(cc); cc.after_statement(); BOOST_REQUIRE(tc.active() == false); @@ -339,7 +342,7 @@ BOOST_FIXTURE_TEST_CASE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - wsrep_mock::bf_abort_provider(sc, tc, 1); + wsrep_test::bf_abort_provider(sc, tc, 1); // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -361,8 +364,42 @@ BOOST_FIXTURE_TEST_CASE( BOOST_REQUIRE(cc.current_error() == wsrep::e_success); } -BOOST_FIXTURE_TEST_CASE(transaction_context_1pc_bf_abort_idle_client, - replicating_client_fixture) +BOOST_FIXTURE_TEST_CASE( + transaction_context_1pc_bf_abort_after_after_command_before_result, + replicating_client_fixture) +{ + cc.start_transaction(1); + BOOST_REQUIRE(tc.active()); + cc.after_statement(); + BOOST_REQUIRE(cc.state() == wsrep::client_context::s_exec); + cc.after_command_before_result(); + BOOST_REQUIRE(cc.state() == wsrep::client_context::s_result); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); + wsrep_test::bf_abort_unordered(cc); + // The result is being sent to client. We need to mark transaction + // as must_abort but not override error yet as this might cause + // a race condition resulting incorrect result returned to the DBMS client. + BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_must_abort); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); + // After the result has been sent to the DBMS client, the after result + // processing should roll back the transaction and set the error. + cc.after_command_after_result(); + BOOST_REQUIRE(cc.state() == wsrep::client_context::s_idle); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + BOOST_REQUIRE(tc.active() == true); + BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_aborted); + BOOST_REQUIRE(cc.before_command() == 1); + BOOST_REQUIRE(tc.active() == false); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_before_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_after_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); +} + +BOOST_FIXTURE_TEST_CASE( + transaction_context_1pc_bf_abort_after_after_command_after_result, + replicating_client_fixture) { cc.start_transaction(1); BOOST_REQUIRE(tc.active()); @@ -372,9 +409,11 @@ BOOST_FIXTURE_TEST_CASE(transaction_context_1pc_bf_abort_idle_client, BOOST_REQUIRE(cc.state() == wsrep::client_context::s_result); cc.after_command_after_result(); BOOST_REQUIRE(cc.state() == wsrep::client_context::s_idle); - wsrep_mock::bf_abort_unordered(cc); + wsrep_test::bf_abort_unordered(cc); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_aborted); + BOOST_REQUIRE(tc.active()); BOOST_REQUIRE(cc.before_command() == 1); + BOOST_REQUIRE(tc.active() == false); BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); cc.after_command_before_result(); BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error);