Core/DBLayer: Fixed memory leaks with existing SQLQueryHolder uses and eliminated the possibilty of adding more in future (replaced manual memory management with smart pointers)

6 files changed, 28 insertions, 40 deletions

diff --git a/src/server/database/Database/DatabaseEnvFwd.h b/src/server/database/Database/DatabaseEnvFwd.h
index ba41655e213..c92c8092775 100644
--- a/src/server/database/Database/DatabaseEnvFwd.h
+++ b/src/server/database/Database/DatabaseEnvFwd.h
@@ -72,8 +72,8 @@ using LoginDatabaseTransaction = SQLTransaction<LoginDatabaseConnection>;
 using WorldDatabaseTransaction = SQLTransaction<WorldDatabaseConnection>;
 
 class SQLQueryHolderBase;
-using QueryResultHolderFuture = std::future<SQLQueryHolderBase*>;
-using QueryResultHolderPromise = std::promise<SQLQueryHolderBase*>;
+using QueryResultHolderFuture = std::future<void>;
+using QueryResultHolderPromise = std::promise<void>;
 
 template<typename T>
 class SQLQueryHolder;
diff --git a/src/server/database/Database/DatabaseWorkerPool.cpp b/src/server/database/Database/DatabaseWorkerPool.cpp
index 8ef1e28d70f..c09f7c3e4b6 100644
--- a/src/server/database/Database/DatabaseWorkerPool.cpp
+++ b/src/server/database/Database/DatabaseWorkerPool.cpp
@@ -226,13 +226,13 @@ QueryCallback DatabaseWorkerPool<T>::AsyncQuery(PreparedStatement<T>* stmt)
 }
 
 template <class T>
-SQLQueryHolderCallback DatabaseWorkerPool<T>::DelayQueryHolder(SQLQueryHolder<T>* holder)
+SQLQueryHolderCallback DatabaseWorkerPool<T>::DelayQueryHolder(std::shared_ptr<SQLQueryHolder<T>> holder)
 {
     SQLQueryHolderTask* task = new SQLQueryHolderTask(holder);
     // Store future result before enqueueing - task might get already processed and deleted before returning from this method
     QueryResultHolderFuture result = task->GetFuture();
     Enqueue(task);
-    return result;
+    return { std::move(holder), std::move(result) };
 }
 
 template <class T>
diff --git a/src/server/database/Database/DatabaseWorkerPool.h b/src/server/database/Database/DatabaseWorkerPool.h
index f46c0e29c04..9852ded374e 100644
--- a/src/server/database/Database/DatabaseWorkerPool.h
+++ b/src/server/database/Database/DatabaseWorkerPool.h
@@ -160,7 +160,7 @@ class DatabaseWorkerPool
         //! return object as soon as the query is executed.
         //! The return value is then processed in ProcessQueryCallback methods.
         //! Any prepared statements added to this holder need to be prepared with the CONNECTION_ASYNC flag.
-        SQLQueryHolderCallback DelayQueryHolder(SQLQueryHolder<T>* holder);
+        SQLQueryHolderCallback DelayQueryHolder(std::shared_ptr<SQLQueryHolder<T>> holder);
 
         /**
             Transaction context methods.
diff --git a/src/server/database/Database/QueryHolder.cpp b/src/server/database/Database/QueryHolder.cpp
index eb9dbdcca22..a5c43b67c8a 100644
--- a/src/server/database/Database/QueryHolder.cpp
+++ b/src/server/database/Database/QueryHolder.cpp
@@ -15,10 +15,11 @@
  * with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "MySQLConnection.h"
 #include "QueryHolder.h"
-#include "PreparedStatement.h"
+#include "Errors.h"
 #include "Log.h"
+#include "MySQLConnection.h"
+#include "PreparedStatement.h"
 #include "QueryResult.h"
 
 bool SQLQueryHolderBase::SetPreparedQueryImpl(size_t index, PreparedStatementBase* stmt)
@@ -33,13 +34,13 @@ bool SQLQueryHolderBase::SetPreparedQueryImpl(size_t index, PreparedStatementBas
     return true;
 }
 
-PreparedQueryResult SQLQueryHolderBase::GetPreparedResult(size_t index)
+PreparedQueryResult SQLQueryHolderBase::GetPreparedResult(size_t index) const
 {
     // Don't call to this function if the index is of a prepared statement
-    if (index < m_queries.size())
-        return m_queries[index].second;
-    else
-        return PreparedQueryResult(nullptr);
+    ASSERT(index < m_queries.size(), "Query holder result index out of range, tried to access index " SZFMTD " but there are only " SZFMTD " results",
+        index, m_queries.size());
+
+    return m_queries[index].second;
 }
 
 void SQLQueryHolderBase::SetPreparedResult(size_t index, PreparedResultSet* result)
@@ -71,25 +72,16 @@ void SQLQueryHolderBase::SetSize(size_t size)
     m_queries.resize(size);
 }
 
-SQLQueryHolderTask::~SQLQueryHolderTask()
-{
-    if (!m_executed)
-        delete m_holder;
-}
+SQLQueryHolderTask::~SQLQueryHolderTask() = default;
 
 bool SQLQueryHolderTask::Execute()
 {
-    m_executed = true;
-
-    if (!m_holder)
-        return false;
-
     /// execute all queries in the holder and pass the results
     for (size_t i = 0; i < m_holder->m_queries.size(); ++i)
         if (PreparedStatementBase* stmt = m_holder->m_queries[i].first)
             m_holder->SetPreparedResult(i, m_conn->Query(stmt));
 
-    m_result.set_value(m_holder);
+    m_result.set_value();
     return true;
 }
 
@@ -97,7 +89,7 @@ bool SQLQueryHolderCallback::InvokeIfReady()
 {
     if (m_future.valid() && m_future.wait_for(std::chrono::seconds(0)) == std::future_status::ready)
     {
-        m_callback(m_future.get());
+        m_callback(*m_holder);
         return true;
     }
 
diff --git a/src/server/database/Database/QueryHolder.h b/src/server/database/Database/QueryHolder.h
index f94eff49d2c..25687fedd81 100644
--- a/src/server/database/Database/QueryHolder.h
+++ b/src/server/database/Database/QueryHolder.h
@@ -19,6 +19,7 @@
 #define _QUERYHOLDER_H
 
 #include "SQLOperation.h"
+#include <vector>
 
 class TC_DATABASE_API SQLQueryHolderBase
 {
@@ -26,10 +27,10 @@ class TC_DATABASE_API SQLQueryHolderBase
     private:
         std::vector<std::pair<PreparedStatementBase*, PreparedQueryResult>> m_queries;
     public:
-        SQLQueryHolderBase() { }
+        SQLQueryHolderBase() = default;
         virtual ~SQLQueryHolderBase();
         void SetSize(size_t size);
-        PreparedQueryResult GetPreparedResult(size_t index);
+        PreparedQueryResult GetPreparedResult(size_t index) const;
         void SetPreparedResult(size_t index, PreparedResultSet* result);
 
     protected:
@@ -49,13 +50,12 @@ public:
 class TC_DATABASE_API SQLQueryHolderTask : public SQLOperation
 {
     private:
-        SQLQueryHolderBase* m_holder;
+        std::shared_ptr<SQLQueryHolderBase> m_holder;
         QueryResultHolderPromise m_result;
-        bool m_executed;
 
     public:
-        SQLQueryHolderTask(SQLQueryHolderBase* holder)
-            : m_holder(holder), m_executed(false) { }
+        explicit SQLQueryHolderTask(std::shared_ptr<SQLQueryHolderBase> holder)
+            : m_holder(std::move(holder)) { }
 
         ~SQLQueryHolderTask();
 
@@ -66,20 +66,23 @@ class TC_DATABASE_API SQLQueryHolderTask : public SQLOperation
 class TC_DATABASE_API SQLQueryHolderCallback
 {
 public:
-    SQLQueryHolderCallback(QueryResultHolderFuture&& future) : m_future(std::move(future)) { }
+    SQLQueryHolderCallback(std::shared_ptr<SQLQueryHolderBase>&& holder, QueryResultHolderFuture&& future)
+        : m_holder(std::move(holder)), m_future(std::move(future)) { }
+
     SQLQueryHolderCallback(SQLQueryHolderCallback&&) = default;
 
     SQLQueryHolderCallback& operator=(SQLQueryHolderCallback&&) = default;
 
-    void AfterComplete(std::function<void(SQLQueryHolderBase*)> callback) &
+    void AfterComplete(std::function<void(SQLQueryHolderBase const&)> callback) &
     {
         m_callback = std::move(callback);
     }
 
     bool InvokeIfReady();
 
+    std::shared_ptr<SQLQueryHolderBase> m_holder;
     QueryResultHolderFuture m_future;
-    std::function<void(SQLQueryHolderBase*)> m_callback;
+    std::function<void(SQLQueryHolderBase const&)> m_callback;
 };
 
 #endif
diff --git a/src/server/database/Database/SQLOperation.h b/src/server/database/Database/SQLOperation.h
index 97f1b396cea..2e85f72c17c 100644
--- a/src/server/database/Database/SQLOperation.h
+++ b/src/server/database/Database/SQLOperation.h
@@ -42,13 +42,6 @@ struct SQLElementData
     SQLElementDataType type;
 };
 
-//- For ambigious resultsets
-union SQLResultSetUnion
-{
-    PreparedResultSet* presult;
-    ResultSet* qresult;
-};
-
 class MySQLConnection;
 
 class TC_DATABASE_API SQLOperation