QxSqlDatabase::clearAllDatabases()

You find a bug using QxOrm library and you know how to fix it : submit a patch on this forum

QxSqlDatabase::clearAllDatabases()

Postby mdw » Wed Jun 14, 2017 12:21 pm

Wouldn't it be better to call QSqlDatabase::removeDatabase() on each database key before clearing the hash map? I mean to change clearAllDatabases() like this:

Code: Select all
void QxSqlDatabase::clearAllDatabases()
{
   qx::QxSqlDatabase::closeAllDatabases();
   qx::QxSqlDatabase * pSingleton = qx::QxSqlDatabase::getSingleton();
   if (! pSingleton) { qAssert(false); return; }
   Q_FOREACH(QString sDbKey, pSingleton->m_lstDbByThread)
   { QSqlDatabase::removeDatabase(sDbKey); }

   pSingleton->m_lstDbByThread.clear();
   pSingleton->m_sDriverName = "";
   pSingleton->m_sConnectOptions = "";
   pSingleton->m_sDatabaseName = "";
   pSingleton->m_sUserName = "";
   pSingleton->m_sPassword = "";
   pSingleton->m_sHostName = "";
   pSingleton->m_iPort = -1;
}
mdw
 
Posts: 34
Joined: Mon Feb 15, 2016 2:45 pm

Re: QxSqlDatabase::clearAllDatabases()

Postby mdw » Wed Jun 14, 2017 2:34 pm

Okay, I have reworked the patch a bit further.
  • The assertions after getSingleton() are not needed, since C++'s new operator always returns a valid instance, otherwise an exception (compare with QxSqlDatabase::getDatabase(QSqlError & dbError))
  • Fixed some missing mutexes
  • I need some way to tell if a connection pool is in use or not, for now I called the routine isEmpty() (I didn't want to call it isClosed() to not cause confusion with closeAllDatabases())

Code: Select all
diff --git a/QxSqlDatabase.h b/QxSqlDatabase.h.new
index 52824f2..4ed0b2d 100644
--- a/QxSqlDatabase.h
+++ b/QxSqlDatabase.h.new
@@ -162,6 +162,7 @@ public:
    static QSqlDatabase getDatabaseCloned();
    static void closeAllDatabases();
    static void clearAllDatabases();
+   static bool isEmpty();
 
    qx::dao::detail::IxSqlGenerator * getSqlGenerator();
 


Code: Select all
diff --git a/QxSqlDatabase.cpp b/QxSqlDatabase.cpp.new
index c3b4a7d..e217d07 100644
--- a/QxSqlDatabase.cpp
+++ b/QxSqlDatabase.cpp.new
@@ -169,7 +169,8 @@ qx::dao::detail::IxSqlGenerator * QxSqlDatabase::getSqlGenerator()
 void QxSqlDatabase::closeAllDatabases()
 {
    qx::QxSqlDatabase * pSingleton = qx::QxSqlDatabase::getSingleton();
-   if (! pSingleton) { qAssert(false); return; }
+   QMutexLocker locker(& pSingleton->m_oDbMutex);
+
    Q_FOREACH(QString sDbKey, pSingleton->m_lstDbByThread)
    { QSqlDatabase::database(sDbKey).close(); }
 }
@@ -177,8 +178,13 @@ void QxSqlDatabase::closeAllDatabases()
 void QxSqlDatabase::clearAllDatabases()
 {
    qx::QxSqlDatabase::closeAllDatabases();
+
    qx::QxSqlDatabase * pSingleton = qx::QxSqlDatabase::getSingleton();
-   if (! pSingleton) { qAssert(false); return; }
+   QMutexLocker locker(& pSingleton->m_oDbMutex);
+
+   Q_FOREACH(QString sDbKey, pSingleton->m_lstDbByThread)
+   { QSqlDatabase::removeDatabase(sDbKey); }
+
    pSingleton->m_lstDbByThread.clear();
    pSingleton->m_sDriverName = "";
    pSingleton->m_sConnectOptions = "";
@@ -189,4 +195,12 @@ void QxSqlDatabase::clearAllDatabases()
    pSingleton->m_iPort = -1;
 }
 
+bool QxSqlDatabase::isEmpty()
+{
+    qx::QxSqlDatabase * pSingleton = qx::QxSqlDatabase::getSingleton();
+    QMutexLocker locker(& pSingleton->m_oDbMutex);
+
+    return pSingleton->m_lstDbByThread.isEmpty();
+}
+
 } // namespace qx

mdw
 
Posts: 34
Joined: Mon Feb 15, 2016 2:45 pm

Re: QxSqlDatabase::clearAllDatabases()

Postby qxorm » Wed Jun 21, 2017 8:11 am

Thx for your patch !
qxorm
Site Admin
 
Posts: 481
Joined: Mon Apr 12, 2010 7:45 am

Re: QxSqlDatabase::clearAllDatabases()

Postby mdw » Wed Jun 21, 2017 9:47 am

Could you please make me a pre-release available, so that I can fix the final nomenclature of the functions?
mdw
 
Posts: 34
Joined: Mon Feb 15, 2016 2:45 pm

Re: QxSqlDatabase::clearAllDatabases()

Postby mdw » Wed Feb 14, 2018 4:51 pm

Update to this issue: Unfortunately I found a problem with the patch which I have posted half a year ago. Please find attached the latest version which I encourage you to include in the upcoming QxORM releases:

  • I need a recursive mutex since clearAllDatabases() is based on closeAllDatabases() and I want to lock immediately to have the whole clear-operation atomic (once or wait). This requires the recursive mutex so that the mutex locker in closeAllDatabases() does not block on the same thread (in Java this is already the default, in Qt unfortunately not).
  • closeAllDatabases() previously did not delete the database references (no call to QSqlDatabase::removeDatabase(sDbKey);). This was wrong when re-opening the pool, since there old connections could have been reused which was not the intention (the threads could have been changed, but the hash code remained the same which caused invalid connection objects to be returned). Now the patch performs a full-cleanup and even empties m_lstDbByThread.

QxSqlDatabase.h
Code: Select all
diff --git a/include/QxDao/QxSqlDatabase.h.old b/include/QxDao/QxSqlDatabase.h
index 52824f2..c94cd04 100644
--- a/include/QxDao/QxSqlDatabase.h.old
+++ b/include/QxDao/QxSqlDatabase.h
@@ -57,6 +57,7 @@
 #include <QxDao/QxSqlGenerator/IxSqlGenerator.h>
 
 #define QX_CONSTRUCT_QX_SQL_DATABASE() \
+m_oDbMutex(QMutex::Recursive), \
 m_iPort(-1), m_bTraceSqlQuery(true), m_bTraceSqlRecord(false), \
 m_bTraceSqlBoundValues(false), m_bTraceSqlBoundValuesOnError(true), \
 m_ePlaceHolderStyle(ph_style_2_point_name), m_bSessionThrowable(false), \
@@ -162,6 +163,7 @@ public:
    static QSqlDatabase getDatabaseCloned();
    static void closeAllDatabases();
    static void clearAllDatabases();
+   static bool isEmpty();
 
    qx::dao::detail::IxSqlGenerator * getSqlGenerator();
 


QxSqlDatabase.cpp
Code: Select all
diff --git a/src/QxDao/QxSqlDatabase.cpp.old b/src/QxDao/QxSqlDatabase.cpp
index c3b4a7d..8d444dc 100644
--- a/src/QxDao/QxSqlDatabase.cpp.old
+++ b/src/QxDao/QxSqlDatabase.cpp
@@ -169,17 +169,23 @@ qx::dao::detail::IxSqlGenerator * QxSqlDatabase::getSqlGenerator()
 void QxSqlDatabase::closeAllDatabases()
 {
    qx::QxSqlDatabase * pSingleton = qx::QxSqlDatabase::getSingleton();
-   if (! pSingleton) { qAssert(false); return; }
+   QMutexLocker locker(& pSingleton->m_oDbMutex);
+
    Q_FOREACH(QString sDbKey, pSingleton->m_lstDbByThread)
-   { QSqlDatabase::database(sDbKey).close(); }
+   {
+       QSqlDatabase::database(sDbKey).close();
+       QSqlDatabase::removeDatabase(sDbKey);
+   }
+   pSingleton->m_lstDbByThread.clear();
 }
 
 void QxSqlDatabase::clearAllDatabases()
 {
-   qx::QxSqlDatabase::closeAllDatabases();
    qx::QxSqlDatabase * pSingleton = qx::QxSqlDatabase::getSingleton();
-   if (! pSingleton) { qAssert(false); return; }
-   pSingleton->m_lstDbByThread.clear();
+   QMutexLocker locker(& pSingleton->m_oDbMutex);
+
+   pSingleton->closeAllDatabases();
+
    pSingleton->m_sDriverName = "";
    pSingleton->m_sConnectOptions = "";
    pSingleton->m_sDatabaseName = "";
@@ -189,4 +195,12 @@ void QxSqlDatabase::clearAllDatabases()
    pSingleton->m_iPort = -1;
 }
 
+bool QxSqlDatabase::isEmpty()
+{
+    qx::QxSqlDatabase * pSingleton = qx::QxSqlDatabase::getSingleton();
+    QMutexLocker locker(& pSingleton->m_oDbMutex);
+
+    return pSingleton->m_lstDbByThread.isEmpty();
+}
+
 } // namespace qx

mdw
 
Posts: 34
Joined: Mon Feb 15, 2016 2:45 pm

Re: QxSqlDatabase::clearAllDatabases()

Postby qxorm » Mon Mar 05, 2018 11:18 am

Hello,
Here is a version which includes your patch : https://www.qxorm.com/version/QxOrm_1.4.5_BETA_04.zip
qxorm
Site Admin
 
Posts: 481
Joined: Mon Apr 12, 2010 7:45 am

Re: QxSqlDatabase::clearAllDatabases()

Postby mdw » Thu Mar 22, 2018 8:22 am

Yes fine, this should work for us :) . I am looking forward for the final 1.4.5 release.
mdw
 
Posts: 34
Joined: Mon Feb 15, 2016 2:45 pm


Return to QxOrm - Submit a patch

Who is online

Users browsing this forum: No registered users and 2 guests