From b98b8f9d4a5219cd193a61fe7869eda73d2b89cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= <markus.makela@mariadb.com>
Date: Wed, 18 Mar 2020 18:19:06 +0200
Subject: [PATCH] MXS-2939: Fix reconnection with session commands

Session commands did not trigger a reconnection process which caused
sessions to be closed in cases where recovery was possible.

Added a test case that verifies the patch fixes the problem.
---
 maxscale-system-test/CMakeLists.txt           |  3 ++
 ...cale.cnf.template.mxs2939_sescmd_reconnect | 26 +++++++++++++++
 .../mxs2939_sescmd_reconnect.cpp              | 33 +++++++++++++++++++
 .../readwritesplit/rwsplit_route_stmt.cc      | 27 ++++++++-------
 4 files changed, 78 insertions(+), 11 deletions(-)
 create mode 100755 maxscale-system-test/cnf/maxscale.cnf.template.mxs2939_sescmd_reconnect
 create mode 100644 maxscale-system-test/mxs2939_sescmd_reconnect.cpp

diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt
index 151eda326..dde0ea387 100644
--- a/maxscale-system-test/CMakeLists.txt
+++ b/maxscale-system-test/CMakeLists.txt
@@ -1015,6 +1015,9 @@ add_test_executable(mxs2631_ignore_system_tables.cpp mxs2631_ignore_system_table
 # MXS-2878: Verify that TLS is required
 add_test_executable(mxs2878_monitor_ssl.cpp mxs2878_monitor_ssl mxs2878_monitor_ssl LABELS REPL_BACKEND)
 
+# MXS-2939: Test that session commands trigger a reconnection
+add_test_executable(mxs2939_sescmd_reconnect.cpp mxs2939_sescmd_reconnect mxs2939_sescmd_reconnect LABELS REPL_BACKEND readwritesplit)
+
 ############################################
 # END: Normal tests                        #
 ############################################
diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.mxs2939_sescmd_reconnect b/maxscale-system-test/cnf/maxscale.cnf.template.mxs2939_sescmd_reconnect
new file mode 100755
index 000000000..4babea381
--- /dev/null
+++ b/maxscale-system-test/cnf/maxscale.cnf.template.mxs2939_sescmd_reconnect
@@ -0,0 +1,26 @@
+[maxscale]
+threads=###threads###
+
+###server###
+
+[MySQL-Monitor]
+type=monitor
+module=mysqlmon
+servers=###server_line###
+user=maxskysql
+password=skysql
+monitor_interval=1000
+
+[RW-Split-Router]
+type=service
+router=readwritesplit
+servers=###server_line###
+user=maxskysql
+password=skysql
+transaction_replay=true
+
+[RW-Split-Listener]
+type=listener
+service=RW-Split-Router
+protocol=MySQLClient
+port=4006
diff --git a/maxscale-system-test/mxs2939_sescmd_reconnect.cpp b/maxscale-system-test/mxs2939_sescmd_reconnect.cpp
new file mode 100644
index 000000000..97686228e
--- /dev/null
+++ b/maxscale-system-test/mxs2939_sescmd_reconnect.cpp
@@ -0,0 +1,33 @@
+/**
+ * MXS-2939: Test that session commands trigger a reconnection
+ */
+
+#include "testconnections.h"
+
+int main(int argc, char** argv)
+{
+    TestConnections test(argc, argv);
+
+    test.maxscales->connect_rwsplit();
+
+    // Make sure we have at least one fully opened connection
+    test.try_query(test.maxscales->conn_rwsplit[0], "select 1");
+
+    // Block and unblock all nodes to sever all connections
+    for (int i = 0; i < test.repl->N; i++)
+    {
+        test.repl->block_node(i);
+    }
+
+    test.maxscales->wait_for_monitor();
+
+    test.repl->unblock_all_nodes();
+    test.maxscales->wait_for_monitor();
+
+    // Make sure that session commands trigger a reconnection if there are no open connections
+    test.set_timeout(20);
+    test.try_query(test.maxscales->conn_rwsplit[0], "set @a = 1");
+    test.maxscales->disconnect();
+
+    return test.global_result;
+}
diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc
index 16995a962..48a1946f3 100644
--- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc
+++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc
@@ -105,22 +105,25 @@ bool RWSplitSession::prepare_target(RWBackend* target, route_target_t route_targ
 
 bool RWSplitSession::create_one_connection()
 {
-    mxb_assert(m_config.lazy_connect);
+    mxb_assert(can_recover_servers());
 
-    // Try to first find a master
-    for (auto backend : m_raw_backends)
+    // Try to first find a master if we are allowed to connect to one
+    if (m_config.lazy_connect || m_config.master_reconnection)
     {
-        if (backend->can_connect() && backend->is_master())
+        for (auto backend : m_raw_backends)
         {
-            if (prepare_target(backend, TARGET_MASTER))
+            if (backend->can_connect() && backend->is_master())
             {
-                if (!m_current_master)
+                if (prepare_target(backend, TARGET_MASTER))
                 {
-                    MXS_INFO("Chose '%s' as master due to session write", backend->name());
-                    m_current_master = backend;
-                }
+                    if (!m_current_master)
+                    {
+                        MXS_INFO("Chose '%s' as master due to session write", backend->name());
+                        m_current_master = backend;
+                    }
 
-                return true;
+                    return true;
+                }
             }
         }
     }
@@ -598,8 +601,10 @@ bool RWSplitSession::route_session_write(GWBUF* querybuf, uint8_t command, uint3
         m_sescmd_list.push_back(sescmd);
     }
 
-    if (m_config.lazy_connect && !attempted_write && nsucc == 0)
+    if (!attempted_write && can_recover_servers())
     {
+        mxb_assert(nsucc == 0);
+
         // If no connections are open, create one and execute the session command on it
         if (create_one_connection())
         {