Skip to content
Permalink
Browse files

[WFS/OAPIF providers] Fix freeze when feature requests issued from ma…

…in thread and authentication manager triggers (fixes #37224)
  • Loading branch information
rouault authored and nyalldawson committed Sep 22, 2020
1 parent 36a7d1b commit daf78c64e9e5342a7f289366a070db46dbcacc36
@@ -101,6 +101,11 @@ void QgsFeatureDownloaderImpl::emitEndOfDownload( bool success )
emit mDownloader->endOfDownload( success );
}

void QgsFeatureDownloaderImpl::emitResumeMainThread()
{
emit mDownloader->resumeMainThread();
}

void QgsFeatureDownloaderImpl::stop()
{
QgsDebugMsgLevel( QStringLiteral( "QgsFeatureDownloaderImpl::stop()" ), 4 );
@@ -212,6 +217,7 @@ void QgsFeatureDownloader::stop()

QgsThreadedFeatureDownloader::QgsThreadedFeatureDownloader( QgsBackgroundCachedSharedData *shared )
: mShared( shared )
, mRequestMadeFromMainThread( QThread::currentThread() == QApplication::instance()->thread() )
{
}

@@ -246,7 +252,7 @@ void QgsThreadedFeatureDownloader::run()
{
// We need to construct it in the run() method (i.e. in the new thread)
mDownloader = new QgsFeatureDownloader();
mDownloader->setImpl( mShared->newFeatureDownloaderImpl( mDownloader ) );
mDownloader->setImpl( mShared->newFeatureDownloaderImpl( mDownloader, mRequestMadeFromMainThread ) );
{
QMutexLocker locker( &mWaitMutex );
mWaitCond.wakeOne();
@@ -487,6 +493,9 @@ void QgsBackgroundCachedFeatureIterator::connectSignals( QgsFeatureDownloader *d

connect( downloader, &QgsFeatureDownloader::endOfDownload,
this, &QgsBackgroundCachedFeatureIterator::endOfDownloadSynchronous, Qt::DirectConnection );

connect( downloader, &QgsFeatureDownloader::resumeMainThread,
this, &QgsBackgroundCachedFeatureIterator::resumeMainThreadSynchronous, Qt::DirectConnection );
}

void QgsBackgroundCachedFeatureIterator::endOfDownloadSynchronous( bool )
@@ -497,6 +506,14 @@ void QgsBackgroundCachedFeatureIterator::endOfDownloadSynchronous( bool )
mWaitCond.wakeOne();
}

void QgsBackgroundCachedFeatureIterator::resumeMainThreadSynchronous()
{
// Wake up waiting loop
QMutexLocker locker( &mMutex );
mProcessEvents = true;
mWaitCond.wakeOne();
}

void QgsBackgroundCachedFeatureIterator::setInterruptionChecker( QgsFeedback *interruptionChecker )
{
mInterruptionChecker = interruptionChecker;
@@ -619,6 +636,8 @@ bool QgsBackgroundCachedFeatureIterator::fetchFeature( QgsFeature &f )
return true;
}

const bool requestMadeFromMainThread = QThread::currentThread() == QApplication::instance()->thread();

// Second step is to wait for features to be notified by the downloader
while ( true )
{
@@ -745,6 +764,11 @@ bool QgsBackgroundCachedFeatureIterator::fetchFeature( QgsFeature &f )
break;
}
mWaitCond.wait( &mMutex, timeout );
if ( requestMadeFromMainThread && mProcessEvents )
{
QgsApplication::instance()->processEvents();
mProcessEvents = false;
}
if ( mInterruptionChecker && mInterruptionChecker->isCanceled() )
{
mTimeoutOrInterruptionOccurred = true;
@@ -17,6 +17,7 @@

#include "qgsfeatureiterator.h"
#include "qgscoordinatereferencesystem.h"
#include "qgsnetworkaccessmanager.h"
#include "qgsspatialindex.h"
#include "qgsvectordataprovider.h"

@@ -107,6 +108,11 @@ class QgsFeatureDownloaderImpl
// To be used when the download is finished (successful or not)
void emitEndOfDownload( bool success );

// To be used when QgsNetworkAccessManager emit signals that require
// QgsBackgroundCachedFeatureIterator to process (authentication) events,
// if it was started from the main thread.
void emitResumeMainThread();

#if 0
// NOTE: implementations should copy & paste the below block
signals:
@@ -142,13 +148,50 @@ class QgsFeatureDownloaderImpl
bool truncatedResponse, bool interrupted,
const QString &errorMessage );

void connectSignals( QObject *obj, bool requestMadeFromMainThread );

private:
QgsBackgroundCachedSharedData *mSharedBase;
QgsFeatureDownloader *mDownloader;
QWidget *mMainWindow = nullptr;
QMutex mMutexCreateProgressDialog;
};

// Sorry for ugliness. Due to QgsFeatureDownloaderImpl that cannot derive from QObject
#define QGS_FEATURE_DOWNLOADER_IMPL_CONNECT_SIGNALS_BASE(requestMadeFromMainThread) \
do { \
if ( requestMadeFromMainThread ) \
{ \
auto resumeMainThread = [this]() \
{ \
emitResumeMainThread(); \
}; \
QObject::connect( QgsNetworkAccessManager::instance(), &QgsNetworkAccessManager::authRequestOccurred, \
this, resumeMainThread, Qt::DirectConnection ); \
QObject::connect( QgsNetworkAccessManager::instance(), &QgsNetworkAccessManager::proxyAuthenticationRequired, \
this, resumeMainThread, Qt::DirectConnection ); \
} \
} while(false)

#ifndef QT_NO_SSL
#define QGS_FEATURE_DOWNLOADER_IMPL_CONNECT_SIGNALS(requestMadeFromMainThread) \
do { \
QGS_FEATURE_DOWNLOADER_IMPL_CONNECT_SIGNALS_BASE(requestMadeFromMainThread); \
if ( requestMadeFromMainThread ) \
{ \
auto resumeMainThread = [this]() \
{ \
emitResumeMainThread(); \
}; \
QObject::connect( QgsNetworkAccessManager::instance(), &QgsNetworkAccessManager::sslErrorsOccurred, \
this, resumeMainThread, Qt::DirectConnection ); \
} \
} while(false)
#else
#define QGS_FEATURE_DOWNLOADER_IMPL_CONNECT_SIGNALS(requestMadeFromMainThread) \
QGS_FEATURE_DOWNLOADER_IMPL_CONNECT_SIGNALS_BASE(requestMadeFromMainThread)
#endif

// Sorry for ugliness. Due to QgsFeatureDownloaderImpl that cannot derive from QObject
#define CONNECT_PROGRESS_DIALOG(actual_downloader_impl_class) do { \
connect( mProgressDialog, &QProgressDialog::canceled, this, &actual_downloader_impl_class::setStopFlag, Qt::DirectConnection ); \
@@ -221,6 +264,11 @@ class QgsFeatureDownloader: public QObject
//! Emitted when the download is finished (successful or not)
void endOfDownload( bool success );

// Emitted when QgsNetworkAccessManager emit signals that require
// QgsBackgroundCachedFeatureIterator to process (authentication) events,
// if it was started from the main thread.
void resumeMainThread();

private:
std::unique_ptr<QgsFeatureDownloaderImpl> mImpl;
};
@@ -253,6 +301,7 @@ class QgsThreadedFeatureDownloader: public QThread
QgsFeatureDownloader *mDownloader = nullptr;
QWaitCondition mWaitCond;
QMutex mWaitMutex;
bool mRequestMadeFromMainThread = false;
};


@@ -287,6 +336,7 @@ class QgsBackgroundCachedFeatureIterator final: public QObject,
private slots:
void featureReceivedSynchronous( const QVector<QgsFeatureUniqueIdPair> &list );
void endOfDownloadSynchronous( bool success );
void resumeMainThreadSynchronous();

private:

@@ -297,6 +347,7 @@ class QgsBackgroundCachedFeatureIterator final: public QObject,

bool mNewFeaturesReceived = false;
bool mDownloadFinished = false;
bool mProcessEvents = false;
QgsFeatureIterator mCacheIterator;
QgsFeedback *mInterruptionChecker = nullptr;
bool mTimeoutOrInterruptionOccurred = false;
@@ -168,7 +168,7 @@ class QgsBackgroundCachedSharedData
//////// Pure virtual methods

//! Instantiate a new feature downloader implementation.
virtual std::unique_ptr<QgsFeatureDownloaderImpl> newFeatureDownloaderImpl( QgsFeatureDownloader * ) = 0;
virtual std::unique_ptr<QgsFeatureDownloaderImpl> newFeatureDownloaderImpl( QgsFeatureDownloader *, bool requestMadeFromMainThread ) = 0;

//! Return whether the GetFeature request should include the request bounding box.
virtual bool isRestrictedToRequestBBOX() const = 0;
@@ -377,9 +377,9 @@ bool QgsOapifSharedData::isRestrictedToRequestBBOX() const
}


std::unique_ptr<QgsFeatureDownloaderImpl> QgsOapifSharedData::newFeatureDownloaderImpl( QgsFeatureDownloader *downloader )
std::unique_ptr<QgsFeatureDownloaderImpl> QgsOapifSharedData::newFeatureDownloaderImpl( QgsFeatureDownloader *downloader, bool requestMadeFromMainThread )
{
return std::unique_ptr<QgsFeatureDownloaderImpl>( new QgsOapifFeatureDownloaderImpl( this, downloader ) );
return std::unique_ptr<QgsFeatureDownloaderImpl>( new QgsOapifFeatureDownloaderImpl( this, downloader, requestMadeFromMainThread ) );
}


@@ -595,10 +595,11 @@ void QgsOapifSharedData::pushError( const QString &errorMsg )

// ---------------------------------

QgsOapifFeatureDownloaderImpl::QgsOapifFeatureDownloaderImpl( QgsOapifSharedData *shared, QgsFeatureDownloader *downloader ):
QgsOapifFeatureDownloaderImpl::QgsOapifFeatureDownloaderImpl( QgsOapifSharedData *shared, QgsFeatureDownloader *downloader, bool requestMadeFromMainThread ):
QgsFeatureDownloaderImpl( shared, downloader ),
mShared( shared )
{
QGS_FEATURE_DOWNLOADER_IMPL_CONNECT_SIGNALS( requestMadeFromMainThread );
}

QgsOapifFeatureDownloaderImpl::~QgsOapifFeatureDownloaderImpl()
@@ -135,7 +135,7 @@ class QgsOapifSharedData final: public QObject, public QgsBackgroundCachedShared

bool hasGeometry() const override { return mWKBType != QgsWkbTypes::Unknown; }

std::unique_ptr<QgsFeatureDownloaderImpl> newFeatureDownloaderImpl( QgsFeatureDownloader * ) override;
std::unique_ptr<QgsFeatureDownloaderImpl> newFeatureDownloaderImpl( QgsFeatureDownloader *, bool requestFromMainThread ) override;

bool isRestrictedToRequestBBOX() const override;

@@ -220,7 +220,7 @@ class QgsOapifFeatureDownloaderImpl final: public QObject, public QgsFeatureDown
void updateProgress( int totalFeatureCount );

public:
QgsOapifFeatureDownloaderImpl( QgsOapifSharedData *shared, QgsFeatureDownloader *downloader );
QgsOapifFeatureDownloaderImpl( QgsOapifSharedData *shared, QgsFeatureDownloader *downloader, bool requestMadeFromMainThread );
~QgsOapifFeatureDownloaderImpl() override;

void run( bool serializeFeatures, int maxFeatures ) override;
@@ -79,13 +79,14 @@ QString QgsWFSFeatureHitsAsyncRequest::errorMessageWithReason( const QString &re

// -------------------------

QgsWFSFeatureDownloaderImpl::QgsWFSFeatureDownloaderImpl( QgsWFSSharedData *shared, QgsFeatureDownloader *downloader ):
QgsWFSFeatureDownloaderImpl::QgsWFSFeatureDownloaderImpl( QgsWFSSharedData *shared, QgsFeatureDownloader *downloader, bool requestMadeFromMainThread ):
QgsWfsRequest( shared->mURI ),
QgsFeatureDownloaderImpl( shared, downloader ),
mShared( shared ),
mPageSize( shared->mPageSize ),
mFeatureHitsAsyncRequest( shared->mURI )
{
QGS_FEATURE_DOWNLOADER_IMPL_CONNECT_SIGNALS( requestMadeFromMainThread );
}

QgsWFSFeatureDownloaderImpl::~QgsWFSFeatureDownloaderImpl()
@@ -78,7 +78,7 @@ class QgsWFSFeatureDownloaderImpl final: public QgsWfsRequest, public QgsFeature
void updateProgress( int totalFeatureCount );

public:
QgsWFSFeatureDownloaderImpl( QgsWFSSharedData *shared, QgsFeatureDownloader *downloader );
QgsWFSFeatureDownloaderImpl( QgsWFSSharedData *shared, QgsFeatureDownloader *downloader, bool requestMadeFromMainThread );
~QgsWFSFeatureDownloaderImpl() override;

void run( bool serializeFeatures, int maxFeatures ) override;
@@ -121,11 +121,21 @@ QgsWFSProvider::QgsWFSProvider( const QString &uri, const ProviderOptions &optio
//Failed to detect feature type from describeFeatureType -> get first feature from layer to detect type
if ( mShared->mWKBType == QgsWkbTypes::Unknown )
{
const bool requestMadeFromMainThread = QThread::currentThread() == QApplication::instance()->thread();
auto downloader = qgis::make_unique<QgsFeatureDownloader>();
downloader->setImpl( qgis::make_unique<QgsWFSFeatureDownloaderImpl>( mShared.get(), downloader.get() ) );
downloader->setImpl( qgis::make_unique<QgsWFSFeatureDownloaderImpl>( mShared.get(), downloader.get(), requestMadeFromMainThread ) );
connect( downloader.get(),
qgis::overload < QVector<QgsFeatureUniqueIdPair> >::of( &QgsFeatureDownloader::featureReceived ),
this, &QgsWFSProvider::featureReceivedAnalyzeOneFeature );
if ( requestMadeFromMainThread )
{
auto processEvents = []()
{
QgsApplication::instance()->processEvents();
};
connect( downloader.get(), &QgsFeatureDownloader::resumeMainThread,
this, processEvents );
}
downloader->run( false, /* serialize features */
1 /* maxfeatures */ );
}
@@ -36,9 +36,9 @@ QgsWFSSharedData::~QgsWFSSharedData()
cleanup();
}

std::unique_ptr<QgsFeatureDownloaderImpl> QgsWFSSharedData::newFeatureDownloaderImpl( QgsFeatureDownloader *downloader )
std::unique_ptr<QgsFeatureDownloaderImpl> QgsWFSSharedData::newFeatureDownloaderImpl( QgsFeatureDownloader *downloader, bool requestMadeFromMainThread )
{
return std::unique_ptr<QgsFeatureDownloaderImpl>( new QgsWFSFeatureDownloaderImpl( this, downloader ) );
return std::unique_ptr<QgsFeatureDownloaderImpl>( new QgsWFSFeatureDownloaderImpl( this, downloader, requestMadeFromMainThread ) );
}

bool QgsWFSSharedData::isRestrictedToRequestBBOX() const
@@ -40,7 +40,7 @@ class QgsWFSSharedData : public QObject, public QgsBackgroundCachedSharedData
//! Return provider geometry attribute name
const QString &geometryAttribute() const { return mGeometryAttribute; }

std::unique_ptr<QgsFeatureDownloaderImpl> newFeatureDownloaderImpl( QgsFeatureDownloader * ) override;
std::unique_ptr<QgsFeatureDownloaderImpl> newFeatureDownloaderImpl( QgsFeatureDownloader *, bool requestFromMainThread ) override;

bool isRestrictedToRequestBBOX() const override;

0 comments on commit daf78c6

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