Fixed prepared statements within transactions not being re-prepared a…

…fter a reconnect.
FredyH · Nov 21, 2021 · b9d5b74 · b9d5b74
1 parent f5dc813
commit b9d5b74
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 118 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -3,7 +3,7 @@ project(mysqloo)
 add_subdirectory(GmodLUA)
 
 file(GLOB_RECURSE MYSQLOO_SRC "src/*.h" "src/*.cpp")
-set(SOURCE_FILES ${MYSQLOO_SRC} src/Main.cpp)
+set(SOURCE_FILES ${MYSQLOO_SRC})
 set(CMAKE_BUILD_TYPE RelWithDebInfo)
 set (CMAKE_CXX_STANDARD 14)
 

diff --git a/src/Main.cpp b/src/Main.cpp
diff --git a/src/mysql/Database.cpp b/src/mysql/Database.cpp
@@ -299,7 +299,7 @@ void Database::setSQLAutoReconnect(bool shouldReconnect) {
 }
 
 //Should only be called from the db thread
-bool Database::getAutoReconnect() {
+bool Database::getSQLAutoReconnect() {
     my_bool autoReconnect;
     mysql_get_optionv(m_sql, MYSQL_OPT_RECONNECT, &autoReconnect);
     return (bool) autoReconnect;

diff --git a/src/mysql/Database.h b/src/mysql/Database.h
@@ -55,8 +55,6 @@ class Database : public std::enable_shared_from_this<Database> {
 
     void setShouldAutoReconnect(bool autoReconnect);
 
-    bool getAutoReconnect();
-
     bool shouldCachePreparedStatements() {
         return cachePreparedStatements;
     }
@@ -107,6 +105,8 @@ class Database : public std::enable_shared_from_this<Database> {
 
     void setSQLAutoReconnect(bool autoReconnect);
 
+    bool getSQLAutoReconnect();
+
 private:
     Database(std::string host, std::string username, std::string pw, std::string database, unsigned int port,
              std::string unixSocket);

diff --git a/src/mysql/PingQuery.cpp b/src/mysql/PingQuery.cpp
@@ -16,7 +16,7 @@ PingQuery::~PingQuery() = default;
 /* Executes the ping query
 */
 void PingQuery::executeQuery(Database &database, MYSQL *connection, const std::shared_ptr<IQueryData> &data) {
-    bool oldAutoReconnect = database.getAutoReconnect();
+    bool oldAutoReconnect = database.getSQLAutoReconnect();
     database.setSQLAutoReconnect(true);
     this->pingSuccess = mysql_ping(connection) == 0;
     database.setSQLAutoReconnect(oldAutoReconnect);

diff --git a/src/mysql/PreparedQuery.cpp b/src/mysql/PreparedQuery.cpp
@@ -181,7 +181,7 @@ void PreparedQuery::generateMysqlBinds(MYSQL_BIND *binds,
 */
 void PreparedQuery::executeQuery(Database &database, MYSQL *connection, const std::shared_ptr<IQueryData> &ptr) {
     std::shared_ptr<PreparedQueryData> data = std::dynamic_pointer_cast<PreparedQueryData>(ptr);
-    bool shouldReconnect = database.getAutoReconnect();
+    bool shouldReconnect = database.getSQLAutoReconnect();
     //Autoreconnect has to be disabled for prepared statement since prepared statements
     //get reset on the server if the connection fails and auto reconnects
     try {
@@ -235,9 +235,20 @@ void PreparedQuery::executeQuery(Database &database, MYSQL *connection, const st
         }
     } catch (const MySQLException &error) {
         unsigned int errorCode = error.getErrorCode();
-        if (errorCode == CR_SERVER_LOST || errorCode == CR_SERVER_GONE_ERROR ||
-            errorCode == ER_MAX_PREPARED_STMT_COUNT_REACHED || errorCode == CR_NO_PREPARE_STMT ||
-            errorCode == ER_UNKNOWN_STMT_HANDLER) {
+        if (errorCode == ER_UNKNOWN_STMT_HANDLER || errorCode == CR_NO_PREPARE_STMT) {
+            //In this case, the statement is lost on the server (usually after a reconnect).
+            //Since the statement is unknown, nothing has been executed yet (i.e. no side effects),
+            //and we are perfectly fine to re-prepare the statement and try again, even if auto-reconnect
+            //is disabled.
+            database.freeStatement(this->cachedStatement);
+            this->cachedStatement = nullptr;
+            if (data->firstAttempt) {
+                data->firstAttempt = false;
+                executeQuery(database, connection, ptr);
+                return;
+            }
+        } else if (errorCode == CR_SERVER_LOST || errorCode == CR_SERVER_GONE_ERROR ||
+                   errorCode == ER_MAX_PREPARED_STMT_COUNT_REACHED) {
             database.freeStatement(this->cachedStatement);
             this->cachedStatement = nullptr;
             //Because autoreconnect is disabled we want to try and explicitly execute the prepared query once more

diff --git a/src/mysql/Transaction.cpp b/src/mysql/Transaction.cpp
@@ -3,76 +3,78 @@
 #include <utility>
 #include "errmsg.h"
 #include "Database.h"
+#include "mysqld_error.h"
 
-bool Transaction::executeStatement(Database &database, MYSQL* connection, std::shared_ptr<IQueryData> ptr) {
+bool Transaction::executeStatement(Database &database, MYSQL *connection, std::shared_ptr<IQueryData> ptr) {
     std::shared_ptr<TransactionData> data = std::dynamic_pointer_cast<TransactionData>(ptr);
-	data->setStatus(QUERY_RUNNING);
-	//This temporarily disables reconnect, since a reconnect
-	//would rollback (and cancel) a transaction
-	//Which could lead to parts of the transaction being executed outside of a transaction
-	//If they are being executed after the reconnect
-	bool oldReconnectStatus = database.getAutoReconnect();
+    data->setStatus(QUERY_RUNNING);
+    //This temporarily disables reconnect, since a reconnect
+    //would rollback (and cancel) a transaction
+    //Which could lead to parts of the transaction being executed outside of a transaction
+    //If they are being executed after the reconnect
+    bool oldReconnectStatus = database.getSQLAutoReconnect();
     database.setSQLAutoReconnect(false);
-	auto resetReconnectStatus = finally([&] { database.setSQLAutoReconnect(oldReconnectStatus); });
-	try {
-		Transaction::mysqlAutocommit(connection, false);
-		{
-			for (auto& query : data->m_queries) {
-				try {
-					//Errors are cleared in case this is retrying after losing connection
-					query.second->setResultStatus(QUERY_NONE);
-					query.second->setError("");
-					query.first->executeQuery(database, connection, query.second);
-				} catch (const MySQLException& error) {
-					query.second->setError(error.what());
-					query.second->setResultStatus(QUERY_ERROR);
-					throw error;
-				}
-			}
-		}
-		mysql_commit(connection);
-		data->setResultStatus(QUERY_SUCCESS);
-		Transaction::mysqlAutocommit(connection, true);
-	} catch (const MySQLException& error) {
-		//This check makes sure that setting mysqlAutocommit back to true doesn't cause the transaction to fail
-		//Even though the transaction was executed successfully
-		if (data->getResultStatus() != QUERY_SUCCESS) {
-			unsigned int errorCode = error.getErrorCode();
-			if (oldReconnectStatus && !data->retried &&
-				(errorCode == CR_SERVER_LOST || errorCode == CR_SERVER_GONE_ERROR)) {
-				//Because autoreconnect is disabled we want to try and explicitly execute the transaction once more
-				//if we can get the client to reconnect (reconnect is caused by mysql_ping)
-				//If this fails we just go ahead and error
+    auto resetReconnectStatus = finally([&] { database.setSQLAutoReconnect(oldReconnectStatus); });
+    try {
+        Transaction::mysqlAutocommit(connection, false);
+        {
+            for (auto &query: data->m_queries) {
+                try {
+                    //Errors are cleared in case this is retrying after losing connection
+                    query.second->setResultStatus(QUERY_NONE);
+                    query.second->setError("");
+                    query.first->executeQuery(database, connection, query.second);
+                } catch (const MySQLException &error) {
+                    query.second->setError(error.what());
+                    query.second->setResultStatus(QUERY_ERROR);
+                    throw error;
+                }
+            }
+        }
+        mysql_commit(connection);
+        data->setResultStatus(QUERY_SUCCESS);
+        Transaction::mysqlAutocommit(connection, true);
+    } catch (const MySQLException &error) {
+        //This check makes sure that setting mysqlAutocommit back to true doesn't cause the transaction to fail
+        //Even though the transaction was executed successfully
+        if (data->getResultStatus() != QUERY_SUCCESS) {
+            unsigned int errorCode = error.getErrorCode();
+            if (oldReconnectStatus && !data->retried &&
+                (errorCode == CR_SERVER_LOST || errorCode == CR_SERVER_GONE_ERROR)) {
+                //Because autoreconnect is disabled we want to try and explicitly execute the transaction once more
+                //if we can get the client to reconnect (reconnect is caused by mysql_ping)
+                //If this fails we just go ahead and error
                 database.setSQLAutoReconnect(true);
-				if (mysql_ping(connection) == 0) {
-					data->retried = true;
-					return executeStatement(database, connection, ptr);
-				}
-			}
-			//If this call fails it means that the connection was (probably) lost
-			//In that case the mysql server rolls back any transaction anyways so it doesn't
-			//matter if it fails
-			mysql_rollback(connection);
-			data->setResultStatus(QUERY_ERROR);
-		}
-		//If this fails it probably means that the connection was lost
-		//In that case autocommit is turned back on anyways (once the connection is reestablished)
-		//See: https://dev.mysql.com/doc/refman/5.7/en/auto-reconnect.html
-		mysql_autocommit(connection, true);
-		data->setError(error.what());
-	}
-	for (auto& pair : data->m_queries) {
-		pair.second->setResultStatus(data->getResultStatus());
-		pair.second->setStatus(QUERY_COMPLETE);
-	}
-	data->setStatus(QUERY_COMPLETE);
-	return true;
+                if (mysql_ping(connection) == 0) {
+                    data->retried = true;
+                    return executeStatement(database, connection, ptr);
+                }
+            }
+            //If this call fails it means that the connection was (probably) lost
+            //In that case the mysql server rolls back any transaction anyways so it doesn't
+            //matter if it fails
+            mysql_rollback(connection);
+            data->setResultStatus(QUERY_ERROR);
+        }
+        //If this fails it probably means that the connection was lost
+        //In that case autocommit is turned back on anyways (once the connection is reestablished)
+        //See: https://dev.mysql.com/doc/refman/5.7/en/auto-reconnect.html
+        mysql_autocommit(connection, true);
+        data->setError(error.what());
+    }
+    for (auto &pair: data->m_queries) {
+        pair.second->setResultStatus(data->getResultStatus());
+        pair.second->setStatus(QUERY_COMPLETE);
+    }
+    data->setStatus(QUERY_COMPLETE);
+    return true;
 }
 
 
-std::shared_ptr<TransactionData> Transaction::buildQueryData(const std::deque<std::pair<std::shared_ptr<Query>, std::shared_ptr<IQueryData>>>& queries) {
-	//At this point the transaction is guaranteed to have a referenced table
-	//since this is always called shortly after transaction:start()
+std::shared_ptr<TransactionData>
+Transaction::buildQueryData(const std::deque<std::pair<std::shared_ptr<Query>, std::shared_ptr<IQueryData>>> &queries) {
+    //At this point the transaction is guaranteed to have a referenced table
+    //since this is always called shortly after transaction:start()
     return std::shared_ptr<TransactionData>(new TransactionData(queries));
 }