Skip to content
Permalink
Browse files

Merge pull request #7055 from m-kuhn/wfsNoMainThreadEventLoopb

Avoid running QEventLoop on main thread (in WFS provider)
  • Loading branch information
m-kuhn committed Jun 15, 2018
2 parents 6b08eff + 478d6d0 commit 66ad9d99ddf58ef8564a95ffe42ab1a13bae46f4
@@ -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"

@@ -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;
@@ -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
}

@@ -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; }
@@ -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 )
@@ -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 )
@@ -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

6 comments on commit 66ad9d9

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

@rduivenvoorde rduivenvoorde replied Jun 15, 2018

@m-kuhn Hi Matthias, seeing this commit. My eye was pulled yesterday to this log msg which you get when you draw an xyz (eg OSM):

../src/providers/wms/qgswmsprovider.cpp: 617: (draw) [0ms] [thread:0x558840d62910] Trying to draw a WMS image on the main thread. Stop it!

Confirmed with a just compiled version now. Is that a call that a thread is stopped? Or is it asking devs to stop drawing in the main thread?

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Jun 15, 2018

@rduivenvoorde in an idea world yes: there should be no blocking downloads done on the main thread.
The message is an advice for developers, no effect at the moment (except for an increased risk of crash in such a scenario).
This pull request is the first brick of a code refactoring to make things more stable (i.e. still not bullet proof, but the risk surface becomes smaller and is limited to cases when authentication is involved).

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jun 15, 2018

@m-kuhn what's the grand plan here?

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Jun 15, 2018

  • Stop using blocking downloads wherever possible
  • Have a class to abstract away those threading details which are too hard to get right
  • Find a way to properly deal with things that require user interaction (foremost authentication)
@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

@rduivenvoorde rduivenvoorde replied Jun 15, 2018

I think indeed the online datasources will become more and more used. That is why I hope we can make QgsNetworkManager more user/python friendly (see qgis/QGIS-Enhancement-Proposals#123).
Also: what is https://qgis.org/pyqgis/master/core/Network/QgsNetworkContentFetcher.html becoming then? Looks to me as a second http-fetch class (though does not do POST?), isn't that a little redundant then?

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Jun 15, 2018

The right thing to use in 90% of the cases when you are able and not so lazy to use blocking downloads.

Please sign in to comment.
You can’t perform that action at this time.