* Unit test for BF abort after after_command_before_result()

* Revised logic for handling BF abort around after command operations
* Added lighweight thread class for runtime thread id checking

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)
+        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;
-        }
 
+        wsrep::thread::id thread_id_;
         wsrep::mutex& mutex_;
         wsrep::server_context& server_context_;
         client_id id_;

include/wsrep/thread.hpp

@@ -0,0 +1,36 @@
+//
+// Copyright (C) 2018 Codership Oy <info@codership.com>
+//
+
+#include <pthread.h>
+
+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()); }
+    }
+};

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

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<wsrep::mutex> 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<wsrep::mutex> 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() &&
+    else if (transaction_.active() == false)
-             transaction_.state() == wsrep::transaction_context::s_aborted)
     {
-        // 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<wsrep::mutex>& 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_] =
         {

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 <boost/test/unit_test.hpp>
 
@@ -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();

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<dbms_client>& client)
 {
+    client->store_globals();
     client->start();
 }
 

src/mock_utils.cpp

@@ -1,44 +0,0 @@
-//
-// Copyright (C) 2018 Codership Oy <info@codership.com>
-//
-
-#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<wsrep::mutex> 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");
-    }
-}

src/server_context_test.cpp

@@ -3,7 +3,6 @@
 //
 
 #include "mock_server_context.hpp"
-#include "mock_utils.hpp"
 
 #include <boost/test/unit_test.hpp>
 
@@ -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;

src/test_utils.cpp

@@ -0,0 +1,26 @@
+//
+// Copyright (C) 2018 Codership Oy <info@codership.com>
+//
+
+#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<wsrep::mutex> 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;
+}

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);
 }

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 <boost/test/unit_test.hpp>
 
@@ -45,10 +45,13 @@ namespace
         {
             BOOST_REQUIRE(cc.before_command() == 0);
             BOOST_REQUIRE(cc.before_statement() == 0);
-            wsrep_mock::start_applying_transaction(
+            wsrep::ws_handle ws_handle(1, (void*)1);
-                cc, 1, 1,
+            wsrep::ws_meta ws_meta(wsrep::gtid(wsrep::id("1"), 1),
-                wsrep::provider::flag::start_transaction |
+                                   wsrep::stid(sc.id(), 1, cc.id()),
-                wsrep::provider::flag::commit);
+                                   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,
+BOOST_FIXTURE_TEST_CASE(
-                        replicating_client_fixture)
+    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);