Skip to content
Permalink
Browse files

[WFS provider] Remove event loop in iterator causing crashes

Fixes #32913
  • Loading branch information
rouault authored and nyalldawson committed Jan 27, 2020
1 parent ad98af9 commit cc9126c3a4a6e6669b1736f71b87daa278f3218e
@@ -1130,18 +1130,16 @@ void QgsWFSFeatureIterator::connectSignals( QgsWFSFeatureDownloader *downloader
connect( downloader, static_cast<void ( QgsWFSFeatureDownloader::* )( QVector<QgsWFSFeatureGmlIdPair> )>( &QgsWFSFeatureDownloader::featureReceived ),
this, &QgsWFSFeatureIterator::featureReceivedSynchronous, Qt::DirectConnection );

connect( downloader, static_cast<void ( QgsWFSFeatureDownloader::* )( int )>( &QgsWFSFeatureDownloader::featureReceived ),
this, &QgsWFSFeatureIterator::featureReceived );

connect( downloader, &QgsWFSFeatureDownloader::endOfDownload,
this, &QgsWFSFeatureIterator::endOfDownload );
this, &QgsWFSFeatureIterator::endOfDownloadSynchronous, Qt::DirectConnection );
}

void QgsWFSFeatureIterator::endOfDownload( bool )
void QgsWFSFeatureIterator::endOfDownloadSynchronous( bool )
{
// Wake up waiting loop
QMutexLocker locker( &mMutex );
mDownloadFinished = true;
if ( mLoop )
mLoop->quit();
mWaitCond.wakeOne();
}

void QgsWFSFeatureIterator::setInterruptionChecker( QgsFeedback *interruptionChecker )
@@ -1155,6 +1153,11 @@ void QgsWFSFeatureIterator::featureReceivedSynchronous( const QVector<QgsWFSFeat
{
QgsDebugMsgLevel( QStringLiteral( "QgsWFSFeatureIterator::featureReceivedSynchronous %1 features" ).arg( list.size() ), 4 );
QMutexLocker locker( &mMutex );

// Wake up waiting loop
mNewFeaturesReceived = true;
mWaitCond.wakeOne();

if ( !mWriterStream )
{
mWriterStream = new QDataStream( &mWriterByteArray, QIODevice::WriteOnly );
@@ -1185,35 +1188,6 @@ void QgsWFSFeatureIterator::featureReceivedSynchronous( const QVector<QgsWFSFeat
}
}

// This will invoked asynchronously, in the thread of QgsWFSFeatureIterator
// hence it is safe to quite the loop
void QgsWFSFeatureIterator::featureReceived( int /*featureCount*/ )
{
//QgsDebugMsg( QStringLiteral("QgsWFSFeatureIterator::featureReceived %1 features").arg(featureCount) );
if ( mLoop )
mLoop->quit();
}

void QgsWFSFeatureIterator::checkInterruption()
{
//QgsDebugMsg("QgsWFSFeatureIterator::checkInterruption()");

if ( mInterruptionChecker && mInterruptionChecker->isCanceled() )
{
mDownloadFinished = true;
if ( mLoop )
mLoop->quit();
}
}

void QgsWFSFeatureIterator::timeout()
{
mTimeoutOccurred = true;
mDownloadFinished = true;
if ( mLoop )
mLoop->quit();
}

bool QgsWFSFeatureIterator::fetchFeature( QgsFeature &f )
{
f.setValid( false );
@@ -1228,7 +1202,7 @@ bool QgsWFSFeatureIterator::fetchFeature( QgsFeature &f )
if ( mInterruptionChecker && mInterruptionChecker->isCanceled() )
return false;

if ( mTimeoutOccurred )
if ( mTimeoutOrInterruptionOccurred )
return false;

//QgsDebugMsg(QString("QgsWFSSharedData::fetchFeature() : mCacheIterator.nextFeature(cachedFeature)") );
@@ -1296,6 +1270,11 @@ bool QgsWFSFeatureIterator::fetchFeature( QgsFeature &f )
// Second step is to wait for features to be notified by the downloader
while ( true )
{
{
QMutexLocker locker( &mMutex );
mNewFeaturesReceived = false;
}

// Initialize a reader stream if there's a writer stream available
if ( !mReaderStream )
{
@@ -1394,22 +1373,40 @@ bool QgsWFSFeatureIterator::fetchFeature( QgsFeature &f )
if ( mInterruptionChecker && mInterruptionChecker->isCanceled() )
return false;

//QgsDebugMsg("fetchFeature before loop");
QEventLoop loop;
mLoop = &loop;
QTimer timer( this );
timer.start( 50 );
QTimer requestTimeout( this );
if ( mRequest.timeout() > 0 )
// Wait for:
// - mRequest.timeout to fire
// - or new features to be notified
// - or end of download being notified
// - or interruption checker to notify cancellation
QTime timeRequestTimeout;
const int requestTimeout = mRequest.timeout();
if ( requestTimeout > 0 )
timeRequestTimeout.start();
while ( true )
{
connect( &requestTimeout, &QTimer::timeout, this, &QgsWFSFeatureIterator::timeout );
requestTimeout.start( mRequest.timeout() );
QMutexLocker locker( &mMutex );
if ( mNewFeaturesReceived || mDownloadFinished )
{
break;
}
const int delayCheckInterruption = 50;
const int timeout = ( requestTimeout > 0 ) ?
std::min( requestTimeout - timeRequestTimeout.elapsed(), delayCheckInterruption ) :
delayCheckInterruption;
if ( timeout < 0 )
{
mTimeoutOrInterruptionOccurred = true;
mDownloadFinished = true;
break;
}
mWaitCond.wait( &mMutex, timeout );
if ( mInterruptionChecker && mInterruptionChecker->isCanceled() )
{
mTimeoutOrInterruptionOccurred = true;
mDownloadFinished = true;
break;
}
}
if ( mInterruptionChecker )
connect( &timer, &QTimer::timeout, this, &QgsWFSFeatureIterator::checkInterruption );
loop.exec( QEventLoop::ExcludeUserInputEvents );
mLoop = nullptr;
//QgsDebugMsg("fetchFeature after loop");
}
}

@@ -214,11 +214,8 @@ class QgsWFSFeatureIterator : public QObject,
void connectSignals( QgsWFSFeatureDownloader *downloader );

private slots:
void featureReceived( int featureCount );
void featureReceivedSynchronous( const QVector<QgsWFSFeatureGmlIdPair> &list );
void endOfDownload( bool success );
void checkInterruption();
void timeout();
void endOfDownloadSynchronous( bool success );

private:

@@ -235,14 +232,15 @@ class QgsWFSFeatureIterator : public QObject,
//! Subset of attributes (relatives to mShared->mFields) to fetch. Only valid if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
QgsAttributeList mSubSetAttributes;

bool mDownloadFinished;
QEventLoop *mLoop = nullptr;
bool mNewFeaturesReceived = false;
bool mDownloadFinished = false;
QgsFeatureIterator mCacheIterator;
QgsFeedback *mInterruptionChecker = nullptr;
bool mTimeoutOccurred = false;
bool mTimeoutOrInterruptionOccurred = false;

//! this mutex synchronizes the mWriterXXXX variables between featureReceivedSynchronous() and fetchFeature()
QMutex mMutex;
QWaitCondition mWaitCond;
//! used to forger mWriterFilename
int mCounter;
//! maximum size in bytes of mWriterByteArray before flushing it to disk
@@ -22,7 +22,7 @@
# Needed on Qt 5 so that the serialization of XML is consistent among all executions
os.environ['QT_HASH_SEED'] = '1'

from qgis.PyQt.QtCore import QCoreApplication, Qt, QObject, QDateTime
from qgis.PyQt.QtCore import QCoreApplication, Qt, QObject, QDateTime, QEventLoop

from qgis.core import (
QgsWkbTypes,
@@ -1651,6 +1651,11 @@ def testWFS20TruncatedResponse(self):
# Check that we get a log message
with MessageLogger('WFS') as logger:
[f for f in vl.getFeatures()]

# Let signals to be notified to QgsVectorDataProvider
loop = QEventLoop()
loop.processEvents()

self.assertEqual(len(logger.messages()), 1, logger.messages())
self.assertTrue(logger.messages()[0].decode('UTF-8').find('The download limit has been reached') >= 0, logger.messages())

@@ -1696,6 +1701,11 @@ def testRetryLogic(self):

# Failed download: test that error is propagated to the data provider, so as to get application notification
[f['INTFIELD'] for f in vl.getFeatures()]

# Let signals to be notified to QgsVectorDataProvider
loop = QEventLoop()
loop.processEvents()

errors = vl.dataProvider().errors()
self.assertEqual(len(errors), 1, errors)

@@ -2585,6 +2595,9 @@ def testGeomedia(self):
self.assertEqual(len(features), 2)

reference = QgsGeometry.fromRect(QgsRectangle(500000, 4500000, 510000, 4510000))
# Let signals to be notified to QgsVectorLayer
loop = QEventLoop()
loop.processEvents()
vl_extent = QgsGeometry.fromRect(vl.extent())
assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001), 'Expected {}, got {}'.format(reference.asWkt(), vl_extent.asWkt())
self.assertEqual(features[0]['intfield'], 1)

0 comments on commit cc9126c

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