Merge pull request #7055 from m-kuhn/wfsNoMainThreadEventLoopb

m-kuhn · web-flow · commit 66ad9d99ddf5 · 2018-06-15T08:57:50.000+02:00
Avoid running QEventLoop on main thread (in WFS provider)
diff --git a/.ci/travis/linux/docker-build-test.sh b/.ci/travis/linux/docker-build-test.sh
@@ -55,6 +55,7 @@ cmake \
  -DWITH_BINDINGS=ON \
  -DWITH_SERVER=ON \
  -DDISABLE_DEPRECATED=ON \
+ -DPYTHON_TEST_WRAPPER="timeout -sSIGSEGV 55s"\
  -DCXX_EXTRA_FLAGS=${CLANG_WARNINGS} ..
 echo "travis_fold:end:cmake"
 
diff --git a/python/core/auto_generated/qgsnetworkaccessmanager.sip.in b/python/core/auto_generated/qgsnetworkaccessmanager.sip.in
@@ -32,7 +32,26 @@ that the fallback proxy should not be used for, then no proxy will be used.
 #include "qgsnetworkaccessmanager.h"
 %End
   public:
-    static QgsNetworkAccessManager *instance();
+
+    static QgsNetworkAccessManager *instance( Qt::ConnectionType connectionType = Qt::BlockingQueuedConnection );
+%Docstring
+Returns a pointer to the active QgsNetworkAccessManager
+for the current thread.
+
+With the ``connectionType`` parameter it is possible to setup the default connection
+type that is used to handle signals that might require user interaction and therefore
+need to be handled on the main thread. See in-depth discussion below.
+
+:param connectionType: In most cases the default of using a ``Qt.BlockingQueuedConnection``
+                       is ok, to make a background thread wait for the main thread to answer such a request is
+                       fine and anything else is dangerous.
+                       However, in case the request was started on the main thread, one should execute a
+                       local event loop in a helper thread and freeze the main thread for the duration of the
+                       download. In this case, if an authentication request is sent from the background thread
+                       network access manager, the background thread should be blocked, the main thread be woken
+                       up, processEvents() executed once, the main thread frozen again and the background thread
+                       continued.
+%End
 
     QgsNetworkAccessManager( QObject *parent = 0 );
 
@@ -76,9 +95,13 @@ Gets name for QNetworkRequest.CacheLoadControl
 Gets QNetworkRequest.CacheLoadControl from name
 %End
 
-    void setupDefaultProxyAndCache();
+    void setupDefaultProxyAndCache( Qt::ConnectionType connectionType = Qt::BlockingQueuedConnection );
 %Docstring
-Setup the NAM according to the user's settings
+Setup the QgsNetworkAccessManager (NAM) according to the user's settings.
+The ``connectionType`` sets up the default connection type that is used to
+handle signals that might require user interaction and therefore
+need to be handled on the main thread. See in-depth discussion in the documentation
+for the constructor of this class.
 %End
 
     bool useSystemProxy() const;
diff --git a/src/core/qgsnetworkaccessmanager.cpp b/src/core/qgsnetworkaccessmanager.cpp
@@ -102,7 +102,7 @@ class QgsNetworkProxyFactory : public QNetworkProxyFactory
 //
 // Static calls to enforce singleton behavior
 //
-QgsNetworkAccessManager *QgsNetworkAccessManager::instance()
+QgsNetworkAccessManager *QgsNetworkAccessManager::instance( Qt::ConnectionType connectionType )
 {
   static QThreadStorage<QgsNetworkAccessManager> sInstances;
   QgsNetworkAccessManager *nam = &sInstances.localData();
@@ -111,7 +111,7 @@ QgsNetworkAccessManager *QgsNetworkAccessManager::instance()
     sMainNAM = nam;
 
   if ( !nam->mInitialized )
-    nam->setupDefaultProxyAndCache();
+    nam->setupDefaultProxyAndCache( connectionType );
 
   return nam;
 }
@@ -281,7 +281,7 @@ QNetworkRequest::CacheLoadControl QgsNetworkAccessManager::cacheLoadControlFromN
   return QNetworkRequest::PreferNetwork;
 }
 
-void QgsNetworkAccessManager::setupDefaultProxyAndCache()
+void QgsNetworkAccessManager::setupDefaultProxyAndCache( Qt::ConnectionType connectionType )
 {
   mInitialized = true;
   mUseSystemProxy = false;
@@ -292,19 +292,19 @@ void QgsNetworkAccessManager::setupDefaultProxyAndCache()
   {
     connect( this, &QNetworkAccessManager::authenticationRequired,
              sMainNAM, &QNetworkAccessManager::authenticationRequired,
-             Qt::BlockingQueuedConnection );
+             connectionType );
 
     connect( this, &QNetworkAccessManager::proxyAuthenticationRequired,
              sMainNAM, &QNetworkAccessManager::proxyAuthenticationRequired,
-             Qt::BlockingQueuedConnection );
+             connectionType );
 
     connect( this, &QgsNetworkAccessManager::requestTimedOut,
              sMainNAM, &QgsNetworkAccessManager::requestTimedOut );
 
 #ifndef QT_NO_SSL
     connect( this, &QNetworkAccessManager::sslErrors,
              sMainNAM, &QNetworkAccessManager::sslErrors,
-             Qt::BlockingQueuedConnection );
+             connectionType );
 #endif
   }
 
diff --git a/src/core/qgsnetworkaccessmanager.h b/src/core/qgsnetworkaccessmanager.h
@@ -49,9 +49,26 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
     Q_OBJECT
 
   public:
-    //! returns a pointer to the single instance
-    // and creates that instance on the first call.
-    static QgsNetworkAccessManager *instance();
+
+    /**
+     * Returns a pointer to the active QgsNetworkAccessManager
+     * for the current thread.
+     *
+     * With the \a connectionType parameter it is possible to setup the default connection
+     * type that is used to handle signals that might require user interaction and therefore
+     * need to be handled on the main thread. See in-depth discussion below.
+     *
+     * \param connectionType In most cases the default of using a ``Qt::BlockingQueuedConnection``
+     * is ok, to make a background thread wait for the main thread to answer such a request is
+     * fine and anything else is dangerous.
+     * However, in case the request was started on the main thread, one should execute a
+     * local event loop in a helper thread and freeze the main thread for the duration of the
+     * download. In this case, if an authentication request is sent from the background thread
+     * network access manager, the background thread should be blocked, the main thread be woken
+     * up, processEvents() executed once, the main thread frozen again and the background thread
+     * continued.
+     */
+    static QgsNetworkAccessManager *instance( Qt::ConnectionType connectionType = Qt::BlockingQueuedConnection );
 
     QgsNetworkAccessManager( QObject *parent = nullptr );
 
@@ -79,8 +96,14 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
     //! Gets QNetworkRequest::CacheLoadControl from name
     static QNetworkRequest::CacheLoadControl cacheLoadControlFromName( const QString &name );
 
-    //! Setup the NAM according to the user's settings
-    void setupDefaultProxyAndCache();
+    /**
+     * Setup the QgsNetworkAccessManager (NAM) according to the user's settings.
+     * The \a connectionType sets up the default connection type that is used to
+     * handle signals that might require user interaction and therefore
+     * need to be handled on the main thread. See in-depth discussion in the documentation
+     * for the constructor of this class.
+     */
+    void setupDefaultProxyAndCache( Qt::ConnectionType connectionType = Qt::BlockingQueuedConnection );
 
     //! Returns whether the system proxy should be used
     bool useSystemProxy() const { return mUseSystemProxy; }
diff --git a/src/providers/wfs/qgswfscapabilities.cpp b/src/providers/wfs/qgswfscapabilities.cpp
@@ -30,7 +30,11 @@
 QgsWfsCapabilities::QgsWfsCapabilities( const QString &uri )
   : QgsWfsRequest( QgsWFSDataSourceURI( uri ) )
 {
-  connect( this, &QgsWfsRequest::downloadFinished, this, &QgsWfsCapabilities::capabilitiesReplyFinished );
+  // Using Qt::DirectConnection since the download might be running on a different thread.
+  // In this case, the request was sent from the main thread and is executed with the main
+  // thread being blocked in future.waitForFinished() so we can run code on this object which
+  // lives in the main thread without risking havoc.
+  connect( this, &QgsWfsRequest::downloadFinished, this, &QgsWfsCapabilities::capabilitiesReplyFinished, Qt::DirectConnection );
 }
 
 bool QgsWfsCapabilities::requestCapabilities( bool synchronous, bool forceRefresh )
diff --git a/src/providers/wfs/qgswfsrequest.cpp b/src/providers/wfs/qgswfsrequest.cpp
@@ -24,6 +24,8 @@
 #include <QEventLoop>
 #include <QNetworkCacheMetaData>
 #include <QCryptographicHash> // just for testin file:// fake_qgis_http_endpoint hack
+#include <QFuture>
+#include <QtConcurrent>
 
 const qint64 READ_BUFFER_SIZE_HINT = 1024 * 1024;
 
@@ -126,26 +128,95 @@ bool QgsWfsRequest::sendGET( const QUrl &url, bool synchronous, bool forceRefres
     request.setAttribute( QNetworkRequest::CacheSaveControlAttribute, true );
   }
 
-  mReply = QgsNetworkAccessManager::instance()->get( request );
-  mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
-  if ( !mUri.auth().setAuthorizationReply( mReply ) )
+  QWaitCondition waitCondition;
+  QMutex waitConditionMutex;
+
+  std::function<bool()> downloaderFunction = [ this, request, synchronous, &waitConditionMutex, &waitCondition ]()
   {
-    mErrorCode = QgsWfsRequest::NetworkError;
-    mErrorMessage = errorMessageFailedAuth();
-    QgsMessageLog::logMessage( mErrorMessage, tr( "WFS" ) );
-    return false;
-  }
-  connect( mReply, &QNetworkReply::finished, this, &QgsWfsRequest::replyFinished );
-  connect( mReply, &QNetworkReply::downloadProgress, this, &QgsWfsRequest::replyProgress );
+    if ( QThread::currentThread() != QgsApplication::instance()->thread() )
+      QgsNetworkAccessManager::instance( Qt::DirectConnection );
 
-  if ( !synchronous )
-    return true;
+    bool success = true;
+    mReply = QgsNetworkAccessManager::instance()->get( request );
 
-  QEventLoop loop;
-  connect( this, &QgsWfsRequest::downloadFinished, &loop, &QEventLoop::quit );
-  loop.exec( QEventLoop::ExcludeUserInputEvents );
+    mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
+    if ( !mUri.auth().setAuthorizationReply( mReply ) )
+    {
+      mErrorCode = QgsWfsRequest::NetworkError;
+      mErrorMessage = errorMessageFailedAuth();
+      QgsMessageLog::logMessage( mErrorMessage, tr( "WFS" ) );
+      waitCondition.wakeAll();
+      success = false;
+    }
+    else
+    {
+      // We are able to use direct connection here, because we
+      // * either run on the thread mReply lives in, so DirectConnection is standard and safe anyway
+      // * or the owner thread of mReply is currently not doing anything because it's blocked in future.waitForFinished() (if it is the main thread)
+      connect( mReply, &QNetworkReply::finished, this, &QgsWfsRequest::replyFinished, Qt::DirectConnection );
+      connect( mReply, &QNetworkReply::downloadProgress, this, &QgsWfsRequest::replyProgress, Qt::DirectConnection );
 
-  return mErrorMessage.isEmpty();
+      if ( synchronous )
+      {
+        auto resumeMainThread = [&waitConditionMutex, &waitCondition]()
+        {
+          waitConditionMutex.lock();
+          waitCondition.wakeAll();
+          waitConditionMutex.unlock();
+
+          waitConditionMutex.lock();
+          waitCondition.wait( &waitConditionMutex );
+          waitConditionMutex.unlock();
+        };
+
+        connect( QgsNetworkAccessManager::instance(), &QgsNetworkAccessManager::authenticationRequired, this, resumeMainThread, Qt::DirectConnection );
+        connect( QgsNetworkAccessManager::instance(), &QgsNetworkAccessManager::proxyAuthenticationRequired, this, resumeMainThread, Qt::DirectConnection );
+
+#ifndef QT_NO_SSL
+        connect( QgsNetworkAccessManager::instance(), &QgsNetworkAccessManager::sslErrors, this, resumeMainThread, Qt::DirectConnection );
+#endif
+        QEventLoop loop;
+        connect( this, &QgsWfsRequest::downloadFinished, &loop, &QEventLoop::quit, Qt::DirectConnection );
+        loop.exec();
+      }
+    }
+    waitCondition.wakeAll();
+    return success;
+  };
+
+  bool success;
+
+  if ( synchronous && QThread::currentThread() == QApplication::instance()->thread() )
+  {
+    std::unique_ptr<DownloaderThread> downloaderThread = qgis::make_unique<DownloaderThread>( downloaderFunction );
+    downloaderThread->start();
+
+    while ( !downloaderThread->isFinished() )
+    {
+      waitConditionMutex.lock();
+      waitCondition.wait( &waitConditionMutex );
+      waitConditionMutex.unlock();
+
+      // If the downloader thread wakes us (the main thread) up and is not yet finished
+      // he needs the authentication to run.
+      // The processEvents() call gives the auth manager the chance to show a dialog and
+      // once done with that, we can wake the downloaderThread again and continue the download.
+      if ( !downloaderThread->isFinished() )
+      {
+        QgsApplication::instance()->processEvents();
+        waitConditionMutex.lock();
+        waitCondition.wakeAll();
+        waitConditionMutex.unlock();
+      }
+    }
+
+    success = downloaderThread->success();
+  }
+  else
+  {
+    success = downloaderFunction();
+  }
+  return success && mErrorMessage.isEmpty();
 }
 
 bool QgsWfsRequest::sendPOST( const QUrl &url, const QString &contentTypeHeader, const QByteArray &data )
diff --git a/src/providers/wfs/qgswfsrequest.h b/src/providers/wfs/qgswfsrequest.h
@@ -19,6 +19,7 @@
 #include <QNetworkRequest>
 #include <QNetworkReply>
 #include <QUrl>
+#include <QAuthenticator>
 
 #include "qgswfsdatasourceuri.h"
 
@@ -117,4 +118,31 @@ class QgsWfsRequest : public QObject
 
 };
 
+
+class DownloaderThread : public QThread
+{
+    Q_OBJECT
+
+  public:
+    DownloaderThread( std::function<bool()> function, QObject *parent = nullptr )
+      : QThread( parent )
+      , mFunction( function )
+    {
+    }
+
+    void run() override
+    {
+      mSuccess = mFunction();
+    }
+
+    bool success() const
+    {
+      return mSuccess;
+    }
+
+  private:
+    std::function<bool()> mFunction;
+    bool mSuccess = false;
+};
+
 #endif // QGSWFSREQUEST_H

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,11 @@`
`30`	`30`	`QgsWfsCapabilities::QgsWfsCapabilities( const QString &uri )`
`31`	`31`	`: QgsWfsRequest( QgsWFSDataSourceURI( uri ) )`
`32`	`32`	`{`
`33`		`- connect( this, &QgsWfsRequest::downloadFinished, this, &QgsWfsCapabilities::capabilitiesReplyFinished );`
	`33`	`+ // Using Qt::DirectConnection since the download might be running on a different thread.`
	`34`	`+ // In this case, the request was sent from the main thread and is executed with the main`
	`35`	`+ // thread being blocked in future.waitForFinished() so we can run code on this object which`
	`36`	`+ // lives in the main thread without risking havoc.`
	`37`	`+ connect( this, &QgsWfsRequest::downloadFinished, this, &QgsWfsCapabilities::capabilitiesReplyFinished, Qt::DirectConnection );`
`34`	`38`	`}`
`35`	`39`
`36`	`40`	`bool QgsWfsCapabilities::requestCapabilities( bool synchronous, bool forceRefresh )`