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 23, 2020
1 parent 307c497 commit 531ba0d6f7b897a21eacdf06b1bcf6a5f28b8250
@@ -483,18 +483,16 @@ void QgsBackgroundCachedFeatureIterator::connectSignals( QgsFeatureDownloader *d
connect( downloader, static_cast<void ( QgsFeatureDownloader::* )( QVector<QgsFeatureUniqueIdPair> )>( &QgsFeatureDownloader::featureReceived ),
this, &QgsBackgroundCachedFeatureIterator::featureReceivedSynchronous, Qt::DirectConnection );

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

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

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

void QgsBackgroundCachedFeatureIterator::setInterruptionChecker( QgsFeedback *interruptionChecker )
@@ -508,6 +506,11 @@ void QgsBackgroundCachedFeatureIterator::featureReceivedSynchronous( const QVect
{
QgsDebugMsgLevel( QStringLiteral( "QgsBackgroundCachedFeatureIterator::featureReceivedSynchronous %1 features" ).arg( list.size() ), 4 );
QMutexLocker locker( &mMutex );

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

if ( !mWriterStream )
{
mWriterStream.reset( new QDataStream( &mWriterByteArray, QIODevice::WriteOnly ) );
@@ -539,34 +542,6 @@ void QgsBackgroundCachedFeatureIterator::featureReceivedSynchronous( const QVect
}
}

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

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

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

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

bool QgsBackgroundCachedFeatureIterator::fetchFeature( QgsFeature &f )
{
@@ -582,7 +557,7 @@ bool QgsBackgroundCachedFeatureIterator::fetchFeature( QgsFeature &f )
if ( mInterruptionChecker && mInterruptionChecker->isCanceled() )
return false;

if ( mTimeoutOccurred )
if ( mTimeoutOrInterruptionOccurred )
return false;

//QgsDebugMsg(QString("QgsBackgroundCachedSharedData::fetchFeature() : mCacheIterator.nextFeature(cachedFeature)") );
@@ -641,6 +616,11 @@ bool QgsBackgroundCachedFeatureIterator::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 )
{
@@ -727,22 +707,41 @@ bool QgsBackgroundCachedFeatureIterator::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, &QgsBackgroundCachedFeatureIterator::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, &QgsBackgroundCachedFeatureIterator::checkInterruption );
loop.exec( QEventLoop::ExcludeUserInputEvents );
mLoop = nullptr;
//QgsDebugMsg("fetchFeature after loop");

}
}

@@ -283,11 +283,8 @@ class QgsBackgroundCachedFeatureIterator : public QObject,
void connectSignals( QgsFeatureDownloader *downloader );

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

private:

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

bool mNewFeaturesReceived = false;
bool mDownloadFinished = false;
QEventLoop *mLoop = nullptr;
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 = 0;
//! 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 531ba0d

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