Address review comments

* Added unit tests for transaction::xa_detach() and
  transaction::xa_replay()
* Added unit tests for wsrep::xid
* Fixed minor issues pointed out by reviewer

include/wsrep/client_service.hpp

@@ -184,13 +184,13 @@ namespace wsrep
          */
         virtual enum wsrep::provider::status commit_by_xid() = 0;
 
-        /*
+        /**
-          Returns true if the client has an ongoing XA transaction.
+         * Returns true if the client has an ongoing XA transaction.
-          This method is used to determine when to cleanup the
+         * This method is used to determine when to cleanup the
-          corresponding wsrep-lib transaction object.
+         * corresponding wsrep-lib transaction object.
-          This method should return false when the XA transaction
+         * This method should return false when the XA transaction
-          is over, and the wsrep-lib transaction object can be
+         * is over, and the wsrep-lib transaction object can be
-          cleaned up.
+         * cleaned up.
          */
         virtual bool is_explicit_xa() = 0;
 

include/wsrep/transaction.hpp

@@ -128,12 +128,7 @@ namespace wsrep
             return !xid_.is_null();
         }
 
-        void assign_xid(const wsrep::xid& xid)
+        void assign_xid(const wsrep::xid& xid);
-        {
-            assert(active());
-            assert(!is_xa());
-            xid_ = xid;
-        }
 
         const wsrep::xid& xid() const
         {

include/wsrep/xid.hpp

@@ -22,6 +22,7 @@
 
 #include <iosfwd>
 #include "buffer.hpp"
+#include "exception.hpp"
 
 namespace wsrep
 {
@@ -42,6 +43,10 @@ namespace wsrep
             , bqual_len_(bqual_len)
             , data_()
         {
+            if (gtrid_len_ > 64 || bqual_len_ > 64)
+            {
+                throw wsrep::runtime_error("maximum wsrep::xid size exceeded");
+            }
             const long len = gtrid_len_ + bqual_len_;
             if (len > 0)
             {
@@ -64,6 +69,8 @@ namespace wsrep
         void clear()
         {
             format_id_ = -1;
+            gtrid_len_ = 0;
+            bqual_len_ = 0;
             data_.clear();
         }
 

src/server_state.cpp

@@ -1429,9 +1429,10 @@ void wsrep::server_state::recover_streaming_appliers_if_not_recovered(
 class transaction_state_cmp
 {
 public:
-    transaction_state_cmp(const enum wsrep::transaction::state s) : state_(s)
+    transaction_state_cmp(const enum wsrep::transaction::state s)
-    { }
+        : state_(s) { }
-    bool operator()(std::pair<const wsrep::client_id, wsrep::client_state*>& vt) const
+    bool operator()(const std::pair<wsrep::client_id,
+                    wsrep::client_state*>& vt) const
     {
         return vt.second->transaction().state() == state_;
     }

src/transaction.cpp

@@ -1071,6 +1071,13 @@ void wsrep::transaction::clone_for_replay(const wsrep::transaction& other)
     state_ = s_replaying;
 }
 
+void wsrep::transaction::assign_xid(const wsrep::xid& xid)
+{
+    assert(active());
+    assert(!is_xa());
+    xid_ = xid;
+}
+
 int wsrep::transaction::restore_to_prepared_state(const wsrep::xid& xid)
 {
     wsrep::unique_lock<wsrep::mutex> lock(client_state_.mutex_);

test/CMakeLists.txt

@@ -17,6 +17,7 @@ add_executable(wsrep-lib_test
   transaction_test_2pc.cpp
   transaction_test_xa.cpp
   view_test.cpp
+  xid_test.cpp
   wsrep-lib_test.cpp
   )
 

test/client_state_fixture.hpp

@@ -51,6 +51,35 @@ namespace
         const wsrep::transaction& tc;
     };
 
+    struct replicating_two_clients_fixture_sync_rm
+    {
+        replicating_two_clients_fixture_sync_rm()
+            : server_service(sc)
+            , sc("s1", wsrep::server_state::rm_sync, server_service)
+            , cc1(sc, wsrep::client_id(1),
+                  wsrep::client_state::m_local)
+            , cc2(sc, wsrep::client_id(2),
+                  wsrep::client_state::m_local)
+            , tc(cc1.transaction())
+        {
+            sc.mock_connect();
+            cc1.open(cc1.id());
+            BOOST_REQUIRE(cc1.before_command() == 0);
+            BOOST_REQUIRE(cc1.before_statement() == 0);
+            cc2.open(cc2.id());
+            BOOST_REQUIRE(cc2.before_command() == 0);
+            BOOST_REQUIRE(cc2.before_statement() == 0);
+            // Verify initial state
+            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 cc1;
+        wsrep::mock_client cc2;
+        const wsrep::transaction& tc;
+    };
+
     struct replicating_client_fixture_async_rm
     {
         replicating_client_fixture_async_rm()

test/mock_client_state.hpp

@@ -74,6 +74,7 @@ namespace wsrep
             , client_state_(client_state)
             , will_replay_called_()
             , replays_()
+            , unordered_replays_()
             , aborts_()
         { }
 
@@ -108,6 +109,7 @@ namespace wsrep
 
         enum wsrep::provider::status replay_unordered() WSREP_OVERRIDE
         {
+            unordered_replays_++;
             return wsrep::provider::success;
         }
 
@@ -221,11 +223,13 @@ namespace wsrep
         //
         bool will_replay_called() const { return will_replay_called_; }
         size_t replays() const { return replays_; }
+        size_t unordered_replays() const { return unordered_replays_; }
         size_t aborts() const { return aborts_; }
     private:
         wsrep::mock_client_state& client_state_;
         bool will_replay_called_;
         size_t replays_;
+        size_t unordered_replays_;
         size_t aborts_;
     };
 

test/mock_high_priority_service.cpp

@@ -37,6 +37,10 @@ int wsrep::mock_high_priority_service::adopt_transaction(
     const wsrep::transaction& transaction)
 {
     client_state_->adopt_transaction(transaction);
+    if (transaction.state() == wsrep::transaction::s_prepared)
+    {
+        client_state_->restore_xid(transaction.xid());
+    }
     return 0;
 }
 

test/mock_provider.hpp

@@ -104,7 +104,6 @@ namespace wsrep
             }
             if (rolls_back_transaction(flags))
             {
-                assert(0);
                 ++rollback_fragments_;
             }
 

test/transaction_test_xa.cpp

@@ -2,7 +2,7 @@
 #include <iostream>
 
 //
-// Test a succesful XA transaction lifecycle
+// Test a successful XA transaction lifecycle
 //
 BOOST_FIXTURE_TEST_CASE(transaction_xa,
                         replicating_client_fixture_sync_rm)
@@ -49,7 +49,89 @@ BOOST_FIXTURE_TEST_CASE(transaction_xa,
 
 
 //
-// Test a succesful XA transaction lifecycle (applying side)
+// Test detaching of XA transactions
+//
+BOOST_FIXTURE_TEST_CASE(transaction_xa_detach_commit_by_xid,
+                        replicating_two_clients_fixture_sync_rm)
+{
+    wsrep::xid xid(1, 1, 1, "id");
+
+    cc1.start_transaction(wsrep::transaction_id(1));
+    cc1.assign_xid(xid);
+    cc1.before_prepare();
+    cc1.after_prepare();
+    BOOST_REQUIRE(sc.provider().fragments() == 1);
+    BOOST_REQUIRE(tc.streaming_context().fragments_certified() == 1);
+
+    cc1.xa_detach();
+
+    BOOST_REQUIRE(tc.state() == wsrep::transaction::s_aborted);
+    BOOST_REQUIRE(sc.find_streaming_applier(xid));
+    BOOST_REQUIRE(cc1.after_statement() == 0);
+
+    cc2.start_transaction(wsrep::transaction_id(2));
+    cc2.assign_xid(xid);
+    BOOST_REQUIRE(cc2.client_state::commit_by_xid(xid) == 0);
+    BOOST_REQUIRE(cc2.after_statement() == 0);
+    BOOST_REQUIRE(sc.provider().commit_fragments() == 1);
+}
+
+BOOST_FIXTURE_TEST_CASE(transaction_xa_detach_rollback_by_xid,
+                        replicating_two_clients_fixture_sync_rm)
+{
+    wsrep::xid xid(1, 1, 1, "id");
+
+    cc1.start_transaction(wsrep::transaction_id(1));
+    cc1.assign_xid(xid);
+    cc1.before_prepare();
+    cc1.after_prepare();
+    BOOST_REQUIRE(sc.provider().fragments() == 1);
+    BOOST_REQUIRE(tc.streaming_context().fragments_certified() == 1);
+
+    cc1.xa_detach();
+
+    BOOST_REQUIRE(tc.state() == wsrep::transaction::s_aborted);
+    BOOST_REQUIRE(sc.find_streaming_applier(xid));
+    BOOST_REQUIRE(cc1.after_statement() == 0);
+
+    cc2.start_transaction(wsrep::transaction_id(2));
+    cc2.assign_xid(xid);
+    BOOST_REQUIRE(cc2.rollback_by_xid(xid) == 0);
+    BOOST_REQUIRE(cc2.after_statement() == 0);
+    BOOST_REQUIRE(sc.provider().rollback_fragments() == 1);
+}
+
+
+//
+// Test XA replay
+//
+BOOST_FIXTURE_TEST_CASE(transaction_xa_replay,
+                        replicating_client_fixture_sync_rm)
+{
+    wsrep::xid xid(1, 1, 1, "id");
+
+    cc.start_transaction(wsrep::transaction_id(1));
+    cc.assign_xid(xid);
+    cc.before_prepare();
+    cc.after_prepare();
+    cc.after_command_before_result();
+    cc.after_command_after_result();
+    BOOST_REQUIRE(cc.state() == wsrep::client_state::s_idle);
+    BOOST_REQUIRE(tc.state() == wsrep::transaction::s_prepared);
+    wsrep_test::bf_abort_unordered(cc);
+    BOOST_REQUIRE(tc.state() == wsrep::transaction::s_must_replay);
+
+    // this is normally done by rollbacker
+    cc.xa_replay();
+    cc.sync_rollback_complete();
+
+    BOOST_REQUIRE(cc.unordered_replays() == 1);
+    BOOST_REQUIRE(sc.find_streaming_applier(sc.id(),
+                                            wsrep::transaction_id(1)));
+}
+
+//
+// Test a successful XA transaction lifecycle (applying side)
 //
 BOOST_FIXTURE_TEST_CASE(transaction_xa_applying,
                         applying_client_fixture)
@@ -84,7 +166,7 @@ BOOST_FIXTURE_TEST_CASE(transaction_xa_applying,
 ///////////////////////////////////////////////////////////////////////////////
 
 //
-// Test a succesful XA transaction lifecycle
+// Test a successful XA transaction lifecycle
 //
 BOOST_FIXTURE_TEST_CASE(transaction_xa_sr,
                         streaming_client_fixture_byte)

test/xid_test.cpp

@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2020 Codership Oy <info@codership.com>
+ *
+ * This file is part of wsrep-lib.
+ *
+ * Wsrep-lib is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Wsrep-lib is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with wsrep-lib.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#include "wsrep/xid.hpp"
+#include <boost/test/unit_test.hpp>
+
+BOOST_AUTO_TEST_CASE(xid_test_is_null)
+{
+    wsrep::xid null_xid;
+    BOOST_REQUIRE(null_xid.is_null());
+    wsrep::xid test_xid(1,0,0,nullptr);
+    BOOST_REQUIRE(!test_xid.is_null());
+}
+
+BOOST_AUTO_TEST_CASE(xid_test_equal)
+{
+    wsrep::xid a(1,1,1,"ab");
+    wsrep::xid b(1,1,1,"ab");
+    BOOST_REQUIRE(a == b);
+}
+
+BOOST_AUTO_TEST_CASE(xid_test_null_equal)
+{
+    wsrep::xid a;
+    wsrep::xid b;
+    BOOST_REQUIRE(a == b);
+    BOOST_REQUIRE(a.is_null());
+}
+
+BOOST_AUTO_TEST_CASE(xid_test_not_equal)
+{
+    wsrep::xid a(1,1,0,"a");
+    wsrep::xid b(1,0,1,"a");
+    wsrep::xid c(-1,1,0,"a");
+    wsrep::xid d(1,1,0,"b");
+    BOOST_REQUIRE(!(a == b));
+    BOOST_REQUIRE(!(a == c));
+    BOOST_REQUIRE(!(a == d));
+}
+
+BOOST_AUTO_TEST_CASE(xid_clear)
+{
+    wsrep::xid null_xid;
+    wsrep::xid to_clear(1, 1, 0, "a");
+    to_clear.clear();
+    BOOST_REQUIRE(to_clear.is_null());
+    BOOST_REQUIRE(null_xid == to_clear);
+}
+
+BOOST_AUTO_TEST_CASE(xid_to_string)
+{
+    wsrep::xid null_xid;
+    std::stringstream null_xid_str;
+    null_xid_str << null_xid;
+    BOOST_REQUIRE(null_xid_str.str() == "");
+
+    wsrep::xid test_xid(1,4,0,"test");
+    std::string xid_str(to_string(test_xid));
+    BOOST_REQUIRE(xid_str == "test");
+}
+
+static bool exception_check(const wsrep::runtime_error&)
+{
+    return true;
+}
+
+BOOST_AUTO_TEST_CASE(xid_too_big)
+{
+    std::string s(65,'a');
+    BOOST_REQUIRE_EXCEPTION(wsrep::xid a(1, 65, 0, s.c_str()),
+                            wsrep::runtime_error, exception_check);
+    BOOST_REQUIRE_EXCEPTION(wsrep::xid b(1, 0, 65, s.c_str()),
+                            wsrep::runtime_error, exception_check);
+}