[Backport 3.10] [WFS provider] Remove event loop in iterator causing crashes

src/providers/wfs/qgswfsfeatureiterator.cpp

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

src/providers/wfs/qgswfsfeatureiterator.h

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

tests/src/python/test_provider_wfs.py

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