Fix QgsBlockingNetworkRequest race condition issue

qgis · Nov 4, 2021 · 80750d5 · 80750d5
1 parent 2771895
commit 80750d5
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 23 deletions.
diff --git a/.ci/test_blocklist_qt5.txt b/.ci/test_blocklist_qt5.txt
@@ -22,7 +22,3 @@ test_core_layerdefinition
 
 # MSSQL requires the MSSQL docker
 PyQgsProviderConnectionMssql
-
-# Too fragile at the moment
-test_core_networkaccessmanager
-
diff --git a/src/core/network/qgsnetworkaccessmanager.cpp b/src/core/network/qgsnetworkaccessmanager.cpp
@@ -217,6 +217,7 @@ QgsNetworkAccessManager *QgsNetworkAccessManager::instance( Qt::ConnectionType c
 
 QgsNetworkAccessManager::QgsNetworkAccessManager( QObject *parent )
   : QNetworkAccessManager( parent )
+  , mAuthRequestHandlerSemaphore( 1 )
 {
   setProxyFactory( new QgsNetworkProxyFactory() );
   setCookieJar( new QgsNetworkCookieJar( this ) );
@@ -466,24 +467,13 @@ void QgsNetworkAccessManager::afterSslErrorHandled( QNetworkReply *reply )
   }
 }
 
-void QgsNetworkAccessManager::unlockAfterAuthRequestHandled()
-{
-  Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );
-  mAuthRequestWaitCondition.wakeOne();
-}
-
 void QgsNetworkAccessManager::afterAuthRequestHandled( QNetworkReply *reply )
 {
   if ( reply->manager() == this )
   {
     restartTimeout( reply );
     emit authRequestHandled( reply );
   }
-  else if ( this == sMainNAM )
-  {
-    // notify other threads to allow them to handle the reply
-    qobject_cast< QgsNetworkAccessManager *>( reply->manager() )->unlockAfterAuthRequestHandled(); // safe to call directly - the other thread will be stuck waiting for us
-  }
 }
 
 void QgsNetworkAccessManager::pauseTimeout( QNetworkReply *reply )
@@ -534,6 +524,7 @@ void QgsNetworkAccessManager::onAuthRequired( QNetworkReply *reply, QAuthenticat
 
   emit requestRequiresAuth( getRequestId( reply ), auth->realm() );
 
+  mAuthRequestHandlerSemaphore.acquire();
   // in main thread this will trigger auth handler immediately and return once the request is satisfied,
   // while in worker thread the signal will be queued (and return immediately) -- hence the need to lock the thread in the next block
   emit authRequestOccurred( reply, auth );
@@ -542,9 +533,8 @@ void QgsNetworkAccessManager::onAuthRequired( QNetworkReply *reply, QAuthenticat
   {
     // lock thread and wait till error is handled. If we return from this slot now, then the reply will resume
     // without actually giving the main thread the chance to act on the ssl error and possibly ignore it.
-    mAuthRequestHandlerMutex.lock();
-    mAuthRequestWaitCondition.wait( &mAuthRequestHandlerMutex );
-    mAuthRequestHandlerMutex.unlock();
+    mAuthRequestHandlerSemaphore.acquire();
+    mAuthRequestHandlerSemaphore.release();
     afterAuthRequestHandled( reply );
   }
 }
@@ -587,6 +577,7 @@ void QgsNetworkAccessManager::handleAuthRequest( QNetworkReply *reply, QAuthenti
   emit requestAuthDetailsAdded( getRequestId( reply ), auth->realm(), auth->user(), auth->password() );
 
   afterAuthRequestHandled( reply );
+  qobject_cast<QgsNetworkAccessManager *>( reply->manager() )->mAuthRequestHandlerSemaphore.release();
 }
 
 QString QgsNetworkAccessManager::cacheLoadControlName( QNetworkRequest::CacheLoadControl control )

diff --git a/src/core/network/qgsnetworkaccessmanager.h b/src/core/network/qgsnetworkaccessmanager.h
@@ -29,6 +29,7 @@
 #include <QNetworkRequest>
 #include <QMutex>
 #include <QWaitCondition>
+#include <QSemaphore>
 #include <memory>
 
 #include "qgis_core.h"
@@ -800,7 +801,6 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
     void afterSslErrorHandled( QNetworkReply *reply );
 #endif
 
-    void unlockAfterAuthRequestHandled();
     void afterAuthRequestHandled( QNetworkReply *reply );
 
     void pauseTimeout( QNetworkReply *reply );
@@ -824,10 +824,8 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
 
     // auth request handler, will be set for main thread ONLY
     std::unique_ptr< QgsNetworkAuthenticationHandler > mAuthHandler;
-    // only in use by worker threads, unused in main thread
-    QMutex mAuthRequestHandlerMutex;
-    // only in use by worker threads, unused in main thread
-    QWaitCondition mAuthRequestWaitCondition;
+    // Used by worker threads to wait for authentification handler run in main thread
+    QSemaphore mAuthRequestHandlerSemaphore;
 };
 
 #endif // QGSNETWORKACCESSMANAGER_H