Merge pull request #9493 from m-kuhn/fix-geomvalidator-freeze

Fix freeze in geometry validator
qgis · Mar 18, 2019 · 1f38b44 · 1f38b44
2 parents 128477d + 87686cd
commit 1f38b44
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 39 deletions.
diff --git a/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in b/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
@@ -10,7 +10,6 @@
 
 
 
-
 class QgsFeaturePool : QgsFeatureSink /Abstract/
 {
 %Docstring
@@ -34,12 +33,11 @@ Creates a new feature pool for ``layer``.
 %End
     virtual ~QgsFeaturePool();
 
-    bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = 0 );
+    bool getFeature( QgsFeatureId id, QgsFeature &feature );
 %Docstring
 Retrieves the feature with the specified ``id`` into ``feature``.
-It will be retrieved from the cache or from the underlying layer if unavailable.
-If the feature is neither available from the cache nor from the layer it will return ``False``.
-If ``feedback`` is specified, the call may return if the feedback is canceled.
+It will be retrieved from the cache or from the underlying feature source if unavailable.
+If the feature is neither available from the cache nor from the source it will return ``False``.
 %End
 
 
@@ -78,11 +76,19 @@ The geometry type of this layer.
     QgsCoordinateReferenceSystem crs() const;
 %Docstring
 The coordinate reference system of this layer.
+%End
+
+    QString layerName() const;
+%Docstring
+Returns the name of the layer.
+
+Should be preferred over layer().name() because it can directly be run on
+the background thread.
 %End
 
   protected:
 
-    void insertFeature( const QgsFeature &feature );
+    void insertFeature( const QgsFeature &feature, bool skipLock = false );
 %Docstring
 Inserts a feature into the cache and the spatial index.
 To be used by implementations of ``addFeature``.

diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
@@ -29,14 +29,14 @@
 QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer )
   : mFeatureCache( CACHE_SIZE )
   , mLayer( layer )
-  , mLayerId( layer->id() )
   , mGeometryType( layer->geometryType() )
-  , mCrs( layer->crs() )
+  , mFeatureSource( qgis::make_unique<QgsVectorLayerFeatureSource>( layer ) )
+  , mLayerName( layer->name() )
 {
 
 }
 
-bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback )
+bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature )
 {
   // Why is there a write lock acquired here? Weird, we only want to read a feature from the cache, right?
   // A method like `QCache::object(const Key &key) const` certainly would not modify its internals.
@@ -55,11 +55,9 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedba
   }
   else
   {
-    std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );
-
     // Feature not in cache, retrieve from layer
     // TODO: avoid always querying all attributes (attribute values are needed when merging by attribute)
-    if ( !source || !source->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
+    if ( !mFeatureSource->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
     {
       return false;
     }
@@ -72,15 +70,22 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedba
 
 QgsFeatureIds QgsFeaturePool::getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback )
 {
+  QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Write );
+  Q_UNUSED( feedback )
+  Q_ASSERT( QThread::currentThread() == qApp->thread() );
+
+  mFeatureCache.clear();
+  mIndex = QgsSpatialIndex();
+
   QgsFeatureIds fids;
 
-  std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );
+  mFeatureSource = qgis::make_unique<QgsVectorLayerFeatureSource>( mLayer );
 
-  QgsFeatureIterator it = source->getFeatures( request );
+  QgsFeatureIterator it = mFeatureSource->getFeatures( request );
   QgsFeature feature;
   while ( it.nextFeature( feature ) )
   {
-    insertFeature( feature );
+    insertFeature( feature, true );
     fids << feature.id();
   }
 
@@ -111,9 +116,11 @@ QPointer<QgsVectorLayer> QgsFeaturePool::layerPtr() const
   return mLayer;
 }
 
-void QgsFeaturePool::insertFeature( const QgsFeature &feature )
+void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
 {
-  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
+  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked );
+  if ( !skipLock )
+    locker.changeMode( QgsReadWriteLocker::Write );
   mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
   QgsFeature indexFeature( feature );
   mIndex.addFeature( indexFeature );
@@ -150,12 +157,19 @@ void QgsFeaturePool::setFeatureIds( const QgsFeatureIds &ids )
 
 bool QgsFeaturePool::isFeatureCached( QgsFeatureId fid )
 {
+  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
   return mFeatureCache.contains( fid );
 }
 
+QString QgsFeaturePool::layerName() const
+{
+  return mLayerName;
+}
+
 QgsCoordinateReferenceSystem QgsFeaturePool::crs() const
 {
-  return mCrs;
+  QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
+  return mFeatureSource->crs();
 }
 
 QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
@@ -165,5 +179,6 @@ QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
 
 QString QgsFeaturePool::layerId() const
 {
-  return mLayerId;
+  QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
+  return mFeatureSource->id();
 }
diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.h b/src/analysis/vector/geometry_checker/qgsfeaturepool.h
@@ -25,8 +25,7 @@
 #include "qgsfeature.h"
 #include "qgsspatialindex.h"
 #include "qgsfeaturesink.h"
-
-class QgsVectorLayer;
+#include "qgsvectorlayerfeatureiterator.h"
 
 /**
  * \ingroup analysis
@@ -48,18 +47,19 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
 
     /**
      * Retrieves the feature with the specified \a id into \a feature.
-     * It will be retrieved from the cache or from the underlying layer if unavailable.
-     * If the feature is neither available from the cache nor from the layer it will return FALSE.
-     * If \a feedback is specified, the call may return if the feedback is canceled.
+     * It will be retrieved from the cache or from the underlying feature source if unavailable.
+     * If the feature is neither available from the cache nor from the source it will return FALSE.
      */
-    bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = nullptr );
+    bool getFeature( QgsFeatureId id, QgsFeature &feature );
 
     /**
      * Gets features for the provided \a request. No features will be fetched
      * from the cache and the request is sent directly to the underlying feature source.
      * Results of the request are cached in the pool and the ids of all the features
-     * are returned. This can be used to warm the cache for a particular area of interest
+     * are returned. This is used to warm the cache for a particular area of interest
      * (bounding box) or other set of features.
+     * This will get a new feature source from the source vector layer.
+     * This needs to be called from the main thread.
      * If \a feedback is specified, the call may return if the feedback is canceled.
      */
     QgsFeatureIds getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback = nullptr ) SIP_SKIP;
@@ -125,13 +125,21 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
      */
     QgsCoordinateReferenceSystem crs() const;
 
+    /**
+     * Returns the name of the layer.
+     *
+     * Should be preferred over layer().name() because it can directly be run on
+     * the background thread.
+     */
+    QString layerName() const;
+
   protected:
 
     /**
      * Inserts a feature into the cache and the spatial index.
      * To be used by implementations of ``addFeature``.
      */
-    void insertFeature( const QgsFeature &feature );
+    void insertFeature( const QgsFeature &feature, bool skipLock = false );
 
     /**
      * Changes a feature in the cache and the spatial index.
@@ -174,9 +182,9 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
     mutable QReadWriteLock mCacheLock;
     QgsFeatureIds mFeatureIds;
     QgsSpatialIndex mIndex;
-    QString mLayerId;
     QgsWkbTypes::GeometryType mGeometryType;
-    QgsCoordinateReferenceSystem mCrs;
+    std::unique_ptr<QgsVectorLayerFeatureSource> mFeatureSource;
+    QString mLayerName;
 };
 
 #endif // QGS_FEATUREPOOL_H
diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp b/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp
@@ -74,17 +74,17 @@ QgsGeometry QgsGeometryCheckerUtils::LayerFeature::geometry() const
 
 QString QgsGeometryCheckerUtils::LayerFeature::id() const
 {
-  return QStringLiteral( "%1:%2" ).arg( layer()->name() ).arg( mFeature.id() );
+  return QStringLiteral( "%1:%2" ).arg( mFeaturePool->layerName() ).arg( mFeature.id() );
 }
 
 bool QgsGeometryCheckerUtils::LayerFeature::operator==( const LayerFeature &other ) const
 {
-  return layer()->id() == other.layer()->id() && feature().id() == other.feature().id();
+  return layerId() == other.layerId() && mFeature.id() == other.mFeature.id();
 }
 
 bool QgsGeometryCheckerUtils::LayerFeature::operator!=( const LayerFeature &other ) const
 {
-  return layer()->id() != other.layer()->id() || feature().id() != other.feature().id();
+  return layerId() != other.layerId() || mFeature.id() != other.mFeature.id();
 }
 
 /////////////////////////////////////////////////////////////////////////////
@@ -197,7 +197,7 @@ bool QgsGeometryCheckerUtils::LayerFeatures::iterator::nextFeature( bool begin )
     QgsFeature feature;
     if ( featurePool->getFeature( *mFeatureIt, feature ) && !feature.geometry().isNull() )
     {
-      mCurrentFeature.reset( new LayerFeature( mParent->mFeaturePools[*mLayerIt], feature, mParent->mContext, mParent->mUseMapCrs ) );
+      mCurrentFeature = qgis::make_unique<LayerFeature>( featurePool, feature, mParent->mContext, mParent->mUseMapCrs );
       return true;
     }
     ++mFeatureIt;

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp b/src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp
@@ -151,7 +151,7 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg
     if ( fid == currentFeature.id() )
       continue;
 
-    if ( featurePool->getFeature( fid, compareFeature, feedback ) )
+    if ( featurePool->getFeature( fid, compareFeature ) )
     {
       if ( feedback && feedback->isCanceled() )
         break;

diff --git a/tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp b/tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp
@@ -142,17 +142,29 @@ void TestQgsVectorLayerFeaturePool::changeGeometry()
   feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((100 100, 110 100, 110 110, 100 110, 100 100))" ) ) );
   vl->updateFeature( feat );
 
-  // No features in the AOI
+  // Still working on the cached data
   QgsFeatureIds ids3 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
-  QCOMPARE( ids3.size(), 0 );
+  QCOMPARE( ids3.size(), 1 );
+
+  // Repopulate the cache
+  QgsFeatureIds ids4 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
+  QCOMPARE( ids4.size(), 0 );
+
+  // Still working on the cached data
+  QgsFeatureIds ids5 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
+  QCOMPARE( ids5.size(), 0 );
 
   // Update a feature to be inside the AOI
   feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((0 0, 10 0, 10 10, 0 10, 0 0))" ) ) );
   vl->updateFeature( feat );
 
-  // One there again
-  QgsFeatureIds ids4 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
-  QCOMPARE( ids4.size(), 1 );
+  // Still cached
+  QgsFeatureIds ids6 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
+  QCOMPARE( ids6.size(), 0 );
+
+  // One in there again
+  QgsFeatureIds ids7 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
+  QCOMPARE( ids7.size(), 1 );
 }
 
 std::unique_ptr<QgsVectorLayer> TestQgsVectorLayerFeaturePool::createPopulatedLayer()