MXS-1323: Fix crash on attempted retry of read

When a backend is waiting for a response but no statement is stored for
the session, the buffer where the stored statement is copied is not
modified. This means that it needs to be initialized to a NULL value.

Added a test that checks that the behavior works as expected even with
persistent connections. A second test reproduces the crash by executing
parallel SET commands while slaves are blocked.

There is still a behavioral problem in readwritesplit. If a session
command is being executed and it fails on a slave, an error is sent to the
client. In this case it would not be necessary to close the session if the
master is still alive.

maxscale-system-test/CMakeLists.txt

@@ -302,6 +302,10 @@ add_test_executable(mxs1110_16mb.cpp mxs1110_16mb longblob_filters LABELS readwr
 # Schemarouter implicit database detection
 add_test_executable(mxs1310_implicit_db.cpp mxs1310_implicit_db mxs1310_implicit_db LABELS schemarouter REPL_BACKEND)
 
+# Retry reads with persistent connections
+add_test_executable(mxs1323_retry_read.cpp mxs1323_retry_read mxs1323 LABELS readwritesplit LIGHT REPL_BACKEND)
+add_test_executable(mxs1323_stress.cpp mxs1323_stress mxs1323 LABELS readwritesplit REPL_BACKEND)
+
 # INSERT extremelly big number of rows
 add_test_executable(lots_of_rows.cpp lots_of_rows galera LABELS readwritesplit HEAVY GALERA_BACKEND)
 

maxscale-system-test/cnf/maxscale.cnf.template.mxs1323

@@ -0,0 +1,51 @@
+[maxscale]
+threads=###threads###
+log_info=1
+
+[MySQL Monitor]
+type=monitor
+module=mysqlmon
+###repl51###
+servers= server1,server2
+user=maxskysql
+passwd= skysql
+monitor_interval=500
+
+[RW Split Router]
+type=service
+router= readwritesplit
+servers=server1,server2
+user=maxskysql
+passwd=skysql
+
+[RW Split Listener]
+type=listener
+service=RW Split Router
+protocol=MySQLClient
+port=4006
+
+[CLI]
+type=service
+router=cli
+
+[CLI Listener]
+type=listener
+service=CLI
+protocol=maxscaled
+socket=default
+
+[server1]
+type=server
+address=###node_server_IP_1###
+port=###node_server_port_1###
+protocol=MySQLBackend
+persistpoolmax=10
+persistmaxtime=300
+
+[server2]
+type=server
+address=###node_server_IP_2###
+port=###node_server_port_2###
+protocol=MySQLBackend
+persistpoolmax=10
+persistmaxtime=300

maxscale-system-test/mariadb_nodes.cpp

@@ -14,6 +14,7 @@
 #include "sql_const.h"
 #include <climits>
 #include <string>
+#include <sstream>
 #include <vector>
 
 Mariadb_nodes::Mariadb_nodes(const char *pref, const char *test_cwd, bool verbose):
@@ -918,6 +919,13 @@ int Mariadb_nodes::get_server_id(int index)
     return id;
 }
 
+std::string Mariadb_nodes::get_server_id_str(int index)
+{
+    std::stringstream ss;
+    ss << get_server_id(index);
+    return ss.str();
+}
+
 void Mariadb_nodes::generate_ssh_cmd(char *cmd, int node, const char *ssh, bool sudo)
 {
     if (strcmp(IP[node], "127.0.0.1") == 0)

maxscale-system-test/mariadb_nodes.h

@@ -16,6 +16,7 @@
 
 #include "mariadb_func.h"
 #include <errno.h>
+#include <string>
 
 /**
  * @brief A class to handle backend nodes
@@ -329,6 +330,7 @@ public:
      * @return Node id of the server or -1 on error
      */
     int get_server_id(int index);
+    std::string get_server_id_str(int index);
 
     /**
      * @brief Generate command line to execute command on the node via ssh

maxscale-system-test/mxs1323_retry_read.cpp

@@ -0,0 +1,50 @@
+/**
+ * Test for MXS-1323.
+ * - Check that retried reads work with persistent connections
+ */
+
+#include "testconnections.h"
+
+void* async_block(void* data)
+{
+    TestConnections *test = (TestConnections*)data;
+    sleep(5);
+    test->tprintf("Blocking slave");
+    test->repl->block_node(1);
+    return NULL;
+}
+
+std::string do_query(TestConnections& test)
+{
+    MYSQL* conn = test.open_rwsplit_connection();
+
+    const char* query = "SELECT SLEEP(10), @@server_id";
+    char output[512] = "";
+
+    find_field(conn, query, "@@server_id", output);
+    mysql_close(conn);
+
+    return std::string(output);
+}
+
+int main(int argc, char *argv[])
+{
+    TestConnections test(argc, argv);
+
+    char server_id[2][1024];
+    test.repl->connect();
+    std::string master = test.repl->get_server_id_str(0);
+    std::string slave = test.repl->get_server_id_str(1);
+    test.repl->close_connections();
+
+    test.set_timeout(60);
+    test.add_result(do_query(test) != slave, "The slave should respond to the first query");
+
+    pthread_t thr;
+    pthread_create(&thr, NULL, async_block, &test);
+    test.add_result(do_query(test) != master, "The master should respond to the second query");
+    pthread_join(thr, NULL);
+    test.repl->unblock_node(1);
+
+    return test.global_result;
+}

maxscale-system-test/mxs1323_stress.cpp

@@ -0,0 +1,81 @@
+/**
+ * Test for MXS-1323.
+ * - Check that retried reads work with persistent connections
+ */
+
+#include "testconnections.h"
+#include <sstream>
+
+static bool running = true;
+
+void* async_query(void* data)
+{
+    TestConnections *test = (TestConnections*)data;
+
+    while (running && test->global_result == 0)
+    {
+        MYSQL* conn = test->open_rwsplit_connection();
+
+        for (int i = 0; i < 50; i++)
+        {
+            const char* query = "SET @a = (SELECT SLEEP(1))";
+            test->try_query(conn, query);
+        }
+
+        mysql_close(conn);
+    }
+
+    return NULL;
+}
+
+#define NUM_THR 5
+
+int main(int argc, char *argv[])
+{
+    TestConnections test(argc, argv);
+    pthread_t query_thr[NUM_THR];
+    std::stringstream ss;
+
+    ss << "CREATE OR REPLACE TABLE test.t1 (id INT)";
+    test.connect_maxscale();
+    test.try_query(test.conn_rwsplit, ss.str().c_str());
+
+    ss.str("");
+    ss << "INSERT INTO test.t1 VALUES (0)";
+    for (int i = 1; i <= 10000; i++)
+    {
+        ss << ",(" << i << ")";
+    }
+    test.try_query(test.conn_rwsplit, ss.str().c_str());
+
+    test.close_maxscale_connections();
+
+    if (test.global_result)
+    {
+        return test.global_result;
+    }
+
+    for (int i = 0; i < NUM_THR; i++)
+    {
+        pthread_create(&query_thr[i], NULL, async_query, &test);
+    }
+
+    for (int i = 0; i < 3 && test.global_result == 0; i++)
+    {
+        test.tprintf("Round %d", i + 1);
+        test.repl->block_node(1);
+        sleep(5);
+        test.repl->unblock_node(1);
+        sleep(5);
+    }
+
+    running = false;
+
+    for (int i = 0; i < NUM_THR; i++)
+    {
+        test.set_timeout(10);
+        pthread_join(query_thr[i], NULL);
+    }
+
+    return test.global_result;
+}

server/modules/routing/readwritesplit/readwritesplit.c

@@ -1541,8 +1541,8 @@ static bool handle_error_new_connection(ROUTER_INSTANCE *inst,
      */
     if (BREF_IS_WAITING_RESULT(bref))
     {
-        GWBUF *stored;
-        const SERVER *target;
+        GWBUF *stored = NULL;
+        const SERVER *target = NULL;
 
         if (!session_take_stmt(backend_dcb->session, &stored, &target) ||
             target != bref->ref->server ||
@@ -1554,8 +1554,14 @@ static bool handle_error_new_connection(ROUTER_INSTANCE *inst,
              */
             gwbuf_free(stored);
 
-            DCB *client_dcb = ses->client_dcb;
-            client_dcb->func.write(client_dcb, gwbuf_clone(errmsg));
+            if (!sescmd_cursor_is_active(&bref->bref_sescmd_cur))
+            {
+                /** The client expects a response from this exact backend.
+                 * We need to route an error to the client to let it know
+                 * that the query failed. */
+                DCB *client_dcb = ses->client_dcb;
+                client_dcb->func.write(client_dcb, gwbuf_clone(errmsg));
+            }
         }
     }