Skip to content
Permalink
Browse files

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

Fix freeze in geometry validator
  • Loading branch information
m-kuhn committed Mar 18, 2019
2 parents 128477d + 87686cd commit 1f38b441e7f61a669e8bb4856f4b8dff0fb8f161
@@ -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``.
@@ -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();
}
@@ -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
@@ -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;
@@ -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;
@@ -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()

0 comments on commit 1f38b44

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