From 7dde0edb5406cd7ffc5efad16225f24257213b5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= <markus.makela@mariadb.com>
Date: Fri, 14 Jun 2019 10:06:38 +0300
Subject: [PATCH] Clean up unexpected error handling in readwritesplit

By using the Error class, the code can be cleaned up and simplified.
---
 include/maxscale/protocol/rwbackend.hh        | 18 +++++++-
 server/modules/protocol/MySQL/rwbackend.cc    |  2 +-
 .../routing/readwritesplit/rwsplitsession.cc  | 43 +++++++++----------
 3 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/include/maxscale/protocol/rwbackend.hh b/include/maxscale/protocol/rwbackend.hh
index 492398757..c5310e664 100644
--- a/include/maxscale/protocol/rwbackend.hh
+++ b/include/maxscale/protocol/rwbackend.hh
@@ -19,6 +19,7 @@
 #include <maxscale/backend.hh>
 #include <maxscale/modutil.hh>
 #include <maxscale/response_stat.hh>
+#include <maxscale/protocol/mysql.hh>
 
 namespace maxscale
 {
@@ -87,6 +88,21 @@ public:
             return rv;
         }
 
+        bool is_unexpected_error() const
+        {
+            switch (m_code)
+            {
+            case ER_CONNECTION_KILLED:
+            case ER_SERVER_SHUTDOWN:
+            case ER_NORMAL_SHUTDOWN:
+            case ER_SHUTDOWN_COMPLETE:
+                return true;
+
+            default:
+                return false;
+            }
+        }
+
         uint32_t code() const
         {
             return m_code;
@@ -121,7 +137,7 @@ public:
         }
 
     private:
-        uint16_t    m_code { 0 };
+        uint16_t    m_code {0};
         std::string m_sql_state;
         std::string m_message;
     };
diff --git a/server/modules/protocol/MySQL/rwbackend.cc b/server/modules/protocol/MySQL/rwbackend.cc
index 1d96f4967..5e11df444 100644
--- a/server/modules/protocol/MySQL/rwbackend.cc
+++ b/server/modules/protocol/MySQL/rwbackend.cc
@@ -410,7 +410,7 @@ void RWBackend::process_reply(GWBUF* buffer)
         process_packets(buffer);
     }
 
-    if (get_reply_state() == REPLY_STATE_DONE)
+    if (get_reply_state() == REPLY_STATE_DONE && is_waiting_result())
     {
         ack_write();
     }
diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc
index 50a12f18e..96a6c8452 100644
--- a/server/modules/routing/readwritesplit/rwsplitsession.cc
+++ b/server/modules/routing/readwritesplit/rwsplitsession.cc
@@ -580,29 +580,15 @@ void RWSplitSession::clientReply(GWBUF* writebuf, DCB* backend_dcb)
     DCB* client_dcb = backend_dcb->session->client_dcb;
     RWBackend* backend = get_backend_from_dcb(backend_dcb);
 
-    if (backend->get_reply_state() == REPLY_STATE_DONE)
+    if (backend->get_reply_state() == REPLY_STATE_DONE
+        && !connection_was_killed(writebuf)
+        && !server_is_shutting_down(writebuf))
     {
-        if (connection_was_killed(writebuf))
-        {
-            // The connection was killed, we can safely ignore it. When the TCP connection is
-            // closed, the router's error handling will sort it out.
-            gwbuf_free(writebuf);
-        }
-        else
-        {
-            /** If we receive an unexpected response from the server, the internal
-             * logic cannot handle this situation. Routing the reply straight to
-             * the client should be the safest thing to do at this point. */
-            log_unexpected_response(backend, writebuf, m_current_query.get());
-            MXS_SESSION_ROUTE_REPLY(backend_dcb->session, writebuf);
-        }
-        return;
-    }
-    else if (backend->get_reply_state() == REPLY_STATE_START && server_is_shutting_down(writebuf))
-    {
-        // The server is shutting down, ignore this error and wait for the TCP connection to die.
-        // This allows the query to be retried on another server without the client noticing it.
-        gwbuf_free(writebuf);
+        /** If we receive an unexpected response from the server, the internal
+         * logic cannot handle this situation. Routing the reply straight to
+         * the client should be the safest thing to do at this point. */
+        log_unexpected_response(backend, writebuf, m_current_query.get());
+        MXS_SESSION_ROUTE_REPLY(backend_dcb->session, writebuf);
         return;
     }
 
@@ -615,6 +601,19 @@ void RWSplitSession::clientReply(GWBUF* writebuf, DCB* backend_dcb)
 
     const RWBackend::Error& error = backend->error();
 
+    if (error.is_unexpected_error())
+    {
+        // The server sent an error that we didn't expect: treat it as if the connection was closed. The
+        // client shouldn't see this error as we can replace the closed connection.
+
+        // TODO: Erase trailing packets
+        if (mxs_mysql_is_err_packet(writebuf))
+        {
+            gwbuf_free(writebuf);
+            return;
+        }
+    }
+
     if (error && m_config.transaction_replay && error.is_rollback())
     {
         MXS_INFO("A retryable error: %s", error.message().c_str());