[Bug Fix] Fix for undefined MySQL library behavior. (#2834)

* MYSQL objects cannot be copied in a well defined way, this removes the copy and replaces it with another connection

* Change to share underlying pointers.

* Push up mutex changes

* Post rebase

* Formatting

---------

Co-authored-by: KimLS <KimLS@peqtgc.com>
Co-authored-by: Akkadius <akkadius1@gmail.com>

client_files/export/main.cpp

@@ -86,7 +86,7 @@ int main(int argc, char **argv)
 			return 1;
 		}
 	} else {
-		content_db.SetMysql(database.getMySQL());
+		content_db.SetMySQL(database);
 	}
 
 	LogSys.SetDatabase(&database)

client_files/import/main.cpp

@@ -83,7 +83,7 @@ int main(int argc, char **argv) {
 			return 1;
 		}
 	} else {
-		content_db.SetMysql(database.getMySQL());
+		content_db.SetMySQL(database);
 	}
 
 	LogSys.SetDatabase(&database)

common/dbcore.cpp

@@ -34,14 +34,16 @@
 
 DBcore::DBcore()
 {
-	mysql_init(&mysql);
+	mysql      = mysql_init(nullptr);
-	pHost           = nullptr;
+	mysqlOwner = true;
-	pUser           = nullptr;
+	pHost      = nullptr;
-	pPassword       = nullptr;
+	pUser      = nullptr;
-	pDatabase       = nullptr;
+	pPassword  = nullptr;
-	pCompress       = false;
+	pDatabase  = nullptr;
-	pSSL            = false;
+	pCompress  = false;
-	pStatus         = Closed;
+	pSSL       = false;
+	pStatus    = Closed;
+	m_mutex    = new Mutex;
 }
 
 DBcore::~DBcore()
@@ -51,16 +53,10 @@ DBcore::~DBcore()
 	 * are re-using the default database connection pointer when we dont have an
 	 * external configuration setup ex: (content_database)
 	 */
-	std::string mysql_connection_host;
+	if (mysqlOwner) {
-	if (mysql.host) {
+		mysql_close(mysql);
-		mysql_connection_host = mysql.host;
 	}
 
-	if (GetOriginHost() != mysql_connection_host) {
-		return;
-	}
-
-	mysql_close(&mysql);
 	safe_delete_array(pHost);
 	safe_delete_array(pUser);
 	safe_delete_array(pPassword);
@@ -70,12 +66,12 @@ DBcore::~DBcore()
 // Sends the MySQL server a keepalive
 void DBcore::ping()
 {
-	if (!MDatabase.trylock()) {
+	if (!m_mutex->trylock()) {
 		// well, if's it's locked, someone's using it. If someone's using it, it doesnt need a keepalive
 		return;
 	}
-	mysql_ping(&mysql);
+	mysql_ping(mysql);
-	MDatabase.unlock();
+	m_mutex->unlock();
 }
 
 MySQLRequestResult DBcore::QueryDatabase(std::string query, bool retryOnFailureOnce)
@@ -96,7 +92,7 @@ MySQLRequestResult DBcore::QueryDatabase(const char *query, uint32 querylen, boo
 	BenchTimer timer;
 	timer.reset();
 
-	LockMutex lock(&MDatabase);
+	LockMutex lock(m_mutex);
 
 	// Reconnect if we are not connected before hand.
 	if (pStatus != Connected) {
@@ -104,8 +100,8 @@ MySQLRequestResult DBcore::QueryDatabase(const char *query, uint32 querylen, boo
 	}
 
 	// request query. != 0 indicates some kind of error.
-	if (mysql_real_query(&mysql, query, querylen) != 0) {
+	if (mysql_real_query(mysql, query, querylen) != 0) {
-		unsigned int errorNumber = mysql_errno(&mysql);
+		unsigned int errorNumber = mysql_errno(mysql);
 
 		if (errorNumber == CR_SERVER_GONE_ERROR) {
 			pStatus = Error;
@@ -129,26 +125,26 @@ MySQLRequestResult DBcore::QueryDatabase(const char *query, uint32 querylen, boo
 
 			auto errorBuffer = new char[MYSQL_ERRMSG_SIZE];
 
-			snprintf(errorBuffer, MYSQL_ERRMSG_SIZE, "#%i: %s", mysql_errno(&mysql), mysql_error(&mysql));
+			snprintf(errorBuffer, MYSQL_ERRMSG_SIZE, "#%i: %s", mysql_errno(mysql), mysql_error(mysql));
 
-			return MySQLRequestResult(nullptr, 0, 0, 0, 0, (uint32) mysql_errno(&mysql), errorBuffer);
+			return MySQLRequestResult(nullptr, 0, 0, 0, 0, (uint32) mysql_errno(mysql), errorBuffer);
 		}
 
 		auto errorBuffer = new char[MYSQL_ERRMSG_SIZE];
-		snprintf(errorBuffer, MYSQL_ERRMSG_SIZE, "#%i: %s", mysql_errno(&mysql), mysql_error(&mysql));
+		snprintf(errorBuffer, MYSQL_ERRMSG_SIZE, "#%i: %s", mysql_errno(mysql), mysql_error(mysql));
 
 		/**
 		 * Error logging
 		 */
-		if (mysql_errno(&mysql) > 0 && strlen(query) > 0) {
+		if (mysql_errno(mysql) > 0 && strlen(query) > 0) {
-			LogMySQLError("[{}] [{}]\n[{}]", mysql_errno(&mysql), mysql_error(&mysql), query);
+			LogMySQLError("[{}] [{}]\n[{}]", mysql_errno(mysql), mysql_error(mysql), query);
 		}
 
-		return MySQLRequestResult(nullptr, 0, 0, 0, 0, mysql_errno(&mysql), errorBuffer);
+		return MySQLRequestResult(nullptr, 0, 0, 0, 0, mysql_errno(mysql), errorBuffer);
 	}
 
 	// successful query. get results.
-	MYSQL_RES *res     = mysql_store_result(&mysql);
+	MYSQL_RES *res     = mysql_store_result(mysql);
 	uint32    rowCount = 0;
 
 	if (res != nullptr) {
@@ -157,10 +153,10 @@ MySQLRequestResult DBcore::QueryDatabase(const char *query, uint32 querylen, boo
 
 	MySQLRequestResult requestResult(
 		res,
-		(uint32) mysql_affected_rows(&mysql),
+		(uint32) mysql_affected_rows(mysql),
 		rowCount,
-		(uint32) mysql_field_count(&mysql),
+		(uint32) mysql_field_count(mysql),
-		(uint32) mysql_insert_id(&mysql)
+		(uint32) mysql_insert_id(mysql)
 	);
 
 	if (LogSys.log_settings[Logs::MySQLQuery].is_category_enabled == 1) {
@@ -206,7 +202,7 @@ uint32 DBcore::DoEscapeString(char *tobuf, const char *frombuf, uint32 fromlen)
 {
 //	No good reason to lock the DB, we only need it in the first place to check char encoding.
 //	LockMutex lock(&MDatabase);
-	return mysql_real_escape_string(&mysql, tobuf, frombuf, fromlen);
+	return mysql_real_escape_string(mysql, tobuf, frombuf, fromlen);
 }
 
 bool DBcore::Open(
@@ -221,7 +217,7 @@ bool DBcore::Open(
 	bool iSSL
 )
 {
-	LockMutex lock(&MDatabase);
+	LockMutex lock(m_mutex);
 	safe_delete_array(pHost);
 	safe_delete_array(pUser);
 	safe_delete_array(pPassword);
@@ -241,13 +237,13 @@ bool DBcore::Open(uint32 *errnum, char *errbuf)
 	if (errbuf) {
 		errbuf[0] = 0;
 	}
-	LockMutex lock(&MDatabase);
+	LockMutex lock(m_mutex);
 	if (GetStatus() == Connected) {
 		return true;
 	}
 	if (GetStatus() == Error) {
-		mysql_close(&mysql);
+		mysql_close(mysql);
-		mysql_init(&mysql);        // Initialize structure again
+		mysql_init(mysql);        // Initialize structure again
 	}
 	if (!pHost) {
 		return false;
@@ -264,7 +260,7 @@ bool DBcore::Open(uint32 *errnum, char *errbuf)
 	if (pSSL) {
 		flags |= CLIENT_SSL;
 	}
-	if (mysql_real_connect(&mysql, pHost, pUser, pPassword, pDatabase, pPort, 0, flags)) {
+	if (mysql_real_connect(mysql, pHost, pUser, pPassword, pDatabase, pPort, 0, flags)) {
 		pStatus = Connected;
 
 		std::string connected_origin_host = pHost;
@@ -274,21 +270,16 @@ bool DBcore::Open(uint32 *errnum, char *errbuf)
 	}
 	else {
 		if (errnum) {
-			*errnum = mysql_errno(&mysql);
+			*errnum = mysql_errno(mysql);
 		}
 		if (errbuf) {
-			snprintf(errbuf, MYSQL_ERRMSG_SIZE, "#%i: %s", mysql_errno(&mysql), mysql_error(&mysql));
+			snprintf(errbuf, MYSQL_ERRMSG_SIZE, "#%i: %s", mysql_errno(mysql), mysql_error(mysql));
 		}
 		pStatus = Error;
 		return false;
 	}
 }
 
-void DBcore::SetMysql(MYSQL *mysql)
-{
-	DBcore::mysql = *mysql;
-}
-
 const std::string &DBcore::GetOriginHost() const
 {
 	return origin_host;
@@ -303,7 +294,14 @@ std::string DBcore::Escape(const std::string& s)
 {
 	const std::size_t s_len = s.length();
 	std::vector<char> temp((s_len * 2) + 1, '\0');
-	mysql_real_escape_string(&mysql, temp.data(), s.c_str(), s_len);
+	mysql_real_escape_string(mysql, temp.data(), s.c_str(), s_len);
 
 	return temp.data();
 }
+
+void DBcore::SetMutex(Mutex *mutex)
+{
+	safe_delete(m_mutex);
+
+	DBcore::m_mutex = mutex;
+}

common/dbcore.h

@@ -31,14 +31,19 @@ public:
 	std::string Escape(const std::string& s);
 	uint32 DoEscapeString(char *tobuf, const char *frombuf, uint32 fromlen);
 	void ping();
-	MYSQL *getMySQL() { return &mysql; }
-	void SetMysql(MYSQL *mysql);
 
 	const std::string &GetOriginHost() const;
 	void SetOriginHost(const std::string &origin_host);
 
 	bool DoesTableExist(std::string table_name);
 
+	void SetMySQL(const DBcore &o)
+	{
+		mysql      = o.mysql;
+		mysqlOwner = false;
+	}
+	void SetMutex(Mutex *mutex);
+
 protected:
 	bool Open(
 		const char *iHost,
@@ -55,8 +60,9 @@ protected:
 private:
 	bool Open(uint32 *errnum = nullptr, char *errbuf = nullptr);
 
-	MYSQL   mysql;
+	MYSQL*  mysql;
-	Mutex   MDatabase;
+	bool    mysqlOwner;
+	Mutex   *m_mutex;
 	eStatus pStatus;
 
 	std::mutex m_query_lock{};

shared_memory/main.cpp

@@ -124,7 +124,7 @@ int main(int argc, char **argv)
 			return 1;
 		}
 	} else {
-		content_db.SetMysql(database.getMySQL());
+		content_db.SetMySQL(database);
 	}
 
 	LogSys.SetDatabase(&database)

world/cli/database_concurrency.cpp

@@ -0,0 +1,73 @@
+#include <thread>
+#include "../../common/repositories/zone_repository.h"
+#include "../../common/eqemu_config.h"
+#include <signal.h>
+
+Database db;
+Database db2;
+
+volatile sig_atomic_t stop;
+void inthand(int signum) {
+	stop = 1;
+}
+
+[[noreturn]] void DatabaseTest()
+{
+	while (true) {
+		LogInfo("DatabaseTest Query");
+		db.QueryDatabase("SELECT 1");
+	}
+}
+
+[[noreturn]] void DatabaseTestSecondConnection()
+{
+	while (true) {
+		LogInfo("DatabaseTest Query");
+		db2.QueryDatabase("SELECT 1");
+	}
+}
+
+
+void WorldserverCLI::TestDatabaseConcurrency(int argc, char **argv, argh::parser &cmd, std::string &description)
+{
+	description = "Test command to test database concurrency";
+
+	if (cmd[{"-h", "--help"}]) {
+		return;
+	}
+
+	signal(SIGINT, inthand);
+
+	LogInfo("Database test");
+
+	auto mutex = new Mutex;
+
+	auto c = EQEmuConfig::get();
+	LogInfo("Connecting to MySQL");
+	if (!db.Connect(
+		c->DatabaseHost.c_str(),
+		c->DatabaseUsername.c_str(),
+		c->DatabasePassword.c_str(),
+		c->DatabaseDB.c_str(),
+		c->DatabasePort
+	)) {
+		LogError("Cannot continue without a database connection");
+		return;
+	}
+
+	db.SetMutex(mutex);
+
+	db2.SetMySQL(db);
+
+	db2.SetMutex(mutex);
+
+	std::thread(DatabaseTest).detach();
+	std::thread(DatabaseTest).detach();
+	std::thread(DatabaseTestSecondConnection).detach();
+
+	while (!stop) {
+
+	}
+
+	safe_delete(mutex);
+}

world/main.cpp

@@ -478,6 +478,8 @@ int main(int argc, char **argv)
 	LogInfo("Signaling HTTP service to stop");
 	LogSys.CloseFileLogs();
 
+	WorldBoot::Shutdown();
+
 	return 0;
 }
 

world/world_boot.cpp

@@ -30,6 +30,8 @@
 extern ZSList      zoneserver_list;
 extern WorldConfig Config;
 
+auto mutex = new Mutex;
+
 void WorldBoot::GMSayHookCallBackProcessWorld(uint16 log_category, const char *func, std::string message)
 {
 	// we don't want to loop up with chat messages
@@ -136,9 +138,7 @@ bool WorldBoot::LoadDatabaseConnections()
 		return false;
 	}
 
-	/**
+	// Multi-tenancy - content database
-	 * Multi-tenancy: Content database
-	 */
 	if (!c->ContentDbHost.empty()) {
 		if (!content_db.Connect(
 			c->ContentDbHost.c_str(),
@@ -153,7 +153,12 @@ bool WorldBoot::LoadDatabaseConnections()
 		}
 	}
 	else {
-		content_db.SetMysql(database.getMySQL());
+		content_db.SetMySQL(database);
+		// when database and content_db share the same underlying mysql connection
+		// it needs to be protected by a shared mutex otherwise we produce concurrency issues
+		// when database actions are occurring in different threads
+		database.SetMutex(mutex);
+		content_db.SetMutex(mutex);
 	}
 
 	return true;
@@ -652,3 +657,8 @@ void WorldBoot::CheckForPossibleConfigurationIssues()
 	}
 }
 
+void WorldBoot::Shutdown()
+{
+	safe_delete(mutex);
+}
+

world/world_boot.h

@@ -15,6 +15,7 @@ public:
 	static void RegisterLoginservers();
 	static bool DatabaseLoadRoutines(int argc, char **argv);
 	static void CheckForPossibleConfigurationIssues();
+	static void Shutdown();
 };
 
 

world/world_server_cli.cpp

@@ -31,10 +31,12 @@ void WorldserverCLI::CommandHandler(int argc, char **argv)
 	function_map["test:expansion"]              = &WorldserverCLI::ExpansionTestCommand;
 	function_map["test:repository"]             = &WorldserverCLI::TestRepository;
 	function_map["test:repository2"]            = &WorldserverCLI::TestRepository2;
+	function_map["test:db-concurrency"]         = &WorldserverCLI::TestDatabaseConcurrency;
 
 	EQEmuCommand::HandleMenu(function_map, cmd, argc, argv);
 }
 
+#include "cli/database_concurrency.cpp"
 #include "cli/copy_character.cpp"
 #include "cli/database_dump.cpp"
 #include "cli/database_get_schema.cpp"

world/world_server_cli.h

@@ -18,6 +18,7 @@ public:
 	static void ExpansionTestCommand(int argc, char **argv, argh::parser &cmd, std::string &description);
 	static void TestRepository(int argc, char **argv, argh::parser &cmd, std::string &description);
 	static void TestRepository2(int argc, char **argv, argh::parser &cmd, std::string &description);
+	static void TestDatabaseConcurrency(int argc, char **argv, argh::parser &cmd, std::string &description);
 };
 
 

zone/main.cpp

@@ -231,6 +231,8 @@ int main(int argc, char** argv) {
 		worldserver.SetLauncherName("NONE");
 	}
 
+	auto mutex = new Mutex;
+
 	LogInfo("Connecting to MySQL");
 	if (!database.Connect(
 		Config->DatabaseHost.c_str(),
@@ -242,9 +244,7 @@ int main(int argc, char** argv) {
 		return 1;
 	}
 
-	/**
+	// Multi-tenancy: Content Database
-	 * Multi-tenancy: Content Database
-	 */
 	if (!Config->ContentDbHost.empty()) {
 		if (!content_db.Connect(
 			Config->ContentDbHost.c_str() ,
@@ -258,7 +258,12 @@ int main(int argc, char** argv) {
 			return 1;
 		}
 	} else {
-		content_db.SetMysql(database.getMySQL());
+		content_db.SetMySQL(database);
+		// when database and content_db share the same underlying mysql connection
+		// it needs to be protected by a shared mutex otherwise we produce concurrency issues
+		// when database actions are occurring in different threads
+		database.SetMutex(mutex);
+		content_db.SetMutex(mutex);
 	}
 
 	/* Register Log System and Settings */
@@ -613,6 +618,9 @@ int main(int argc, char** argv) {
 	safe_delete(parse);
 	LogInfo("Proper zone shutdown complete.");
 	LogSys.CloseFileLogs();
+
+	safe_delete(mutex);
+
 	return 0;
 }