Skip to content

Commit 1f38b44

Browse files
m-kuhnm-kuhn
authoredMar 18, 2019
Merge pull request #9493 from m-kuhn/fix-geomvalidator-freeze
Fix freeze in geometry validator
·
ltr-3_40final-3_8_0
2 parents 128477d + 87686cd commit 1f38b44

File tree

6 files changed

+80
-39
lines changed

6 files changed

+80
-39
lines changed
 

‎python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111

1212

13-
1413
class QgsFeaturePool : QgsFeatureSink /Abstract/
1514
{
1615
%Docstring
@@ -34,12 +33,11 @@ Creates a new feature pool for ``layer``.
3433
%End
3534
virtual ~QgsFeaturePool();
3635

37-
bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = 0 );
36+
bool getFeature( QgsFeatureId id, QgsFeature &feature );
3837
%Docstring
3938
Retrieves the feature with the specified ``id`` into ``feature``.
40-
It will be retrieved from the cache or from the underlying layer if unavailable.
41-
If the feature is neither available from the cache nor from the layer it will return ``False``.
42-
If ``feedback`` is specified, the call may return if the feedback is canceled.
39+
It will be retrieved from the cache or from the underlying feature source if unavailable.
40+
If the feature is neither available from the cache nor from the source it will return ``False``.
4341
%End
4442

4543

@@ -78,11 +76,19 @@ The geometry type of this layer.
7876
QgsCoordinateReferenceSystem crs() const;
7977
%Docstring
8078
The coordinate reference system of this layer.
79+
%End
80+
81+
QString layerName() const;
82+
%Docstring
83+
Returns the name of the layer.
84+
85+
Should be preferred over layer().name() because it can directly be run on
86+
the background thread.
8187
%End
8288

8389
protected:
8490

85-
void insertFeature( const QgsFeature &feature );
91+
void insertFeature( const QgsFeature &feature, bool skipLock = false );
8692
%Docstring
8793
Inserts a feature into the cache and the spatial index.
8894
To be used by implementations of ``addFeature``.

‎src/analysis/vector/geometry_checker/qgsfeaturepool.cpp

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,14 @@
2929
QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer )
3030
: mFeatureCache( CACHE_SIZE )
3131
, mLayer( layer )
32-
, mLayerId( layer->id() )
3332
, mGeometryType( layer->geometryType() )
34-
, mCrs( layer->crs() )
33+
, mFeatureSource( qgis::make_unique<QgsVectorLayerFeatureSource>( layer ) )
34+
, mLayerName( layer->name() )
3535
{
3636

3737
}
3838

39-
bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback )
39+
bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature )
4040
{
4141
// Why is there a write lock acquired here? Weird, we only want to read a feature from the cache, right?
4242
// 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
5555
}
5656
else
5757
{
58-
std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );
59-
6058
// Feature not in cache, retrieve from layer
6159
// TODO: avoid always querying all attributes (attribute values are needed when merging by attribute)
62-
if ( !source || !source->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
60+
if ( !mFeatureSource->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
6361
{
6462
return false;
6563
}
@@ -72,15 +70,22 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedba
7270

7371
QgsFeatureIds QgsFeaturePool::getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback )
7472
{
73+
QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Write );
74+
Q_UNUSED( feedback )
75+
Q_ASSERT( QThread::currentThread() == qApp->thread() );
76+
77+
mFeatureCache.clear();
78+
mIndex = QgsSpatialIndex();
79+
7580
QgsFeatureIds fids;
7681

77-
std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );
82+
mFeatureSource = qgis::make_unique<QgsVectorLayerFeatureSource>( mLayer );
7883

79-
QgsFeatureIterator it = source->getFeatures( request );
84+
QgsFeatureIterator it = mFeatureSource->getFeatures( request );
8085
QgsFeature feature;
8186
while ( it.nextFeature( feature ) )
8287
{
83-
insertFeature( feature );
88+
insertFeature( feature, true );
8489
fids << feature.id();
8590
}
8691

@@ -111,9 +116,11 @@ QPointer<QgsVectorLayer> QgsFeaturePool::layerPtr() const
111116
return mLayer;
112117
}
113118

114-
void QgsFeaturePool::insertFeature( const QgsFeature &feature )
119+
void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
115120
{
116-
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
121+
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked );
122+
if ( !skipLock )
123+
locker.changeMode( QgsReadWriteLocker::Write );
117124
mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
118125
QgsFeature indexFeature( feature );
119126
mIndex.addFeature( indexFeature );
@@ -150,12 +157,19 @@ void QgsFeaturePool::setFeatureIds( const QgsFeatureIds &ids )
150157

151158
bool QgsFeaturePool::isFeatureCached( QgsFeatureId fid )
152159
{
160+
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
153161
return mFeatureCache.contains( fid );
154162
}
155163

164+
QString QgsFeaturePool::layerName() const
165+
{
166+
return mLayerName;
167+
}
168+
156169
QgsCoordinateReferenceSystem QgsFeaturePool::crs() const
157170
{
158-
return mCrs;
171+
QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
172+
return mFeatureSource->crs();
159173
}
160174

161175
QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
@@ -165,5 +179,6 @@ QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
165179

166180
QString QgsFeaturePool::layerId() const
167181
{
168-
return mLayerId;
182+
QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
183+
return mFeatureSource->id();
169184
}

‎src/analysis/vector/geometry_checker/qgsfeaturepool.h

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,7 @@
2525
#include "qgsfeature.h"
2626
#include "qgsspatialindex.h"
2727
#include "qgsfeaturesink.h"
28-
29-
class QgsVectorLayer;
28+
#include "qgsvectorlayerfeatureiterator.h"
3029

3130
/**
3231
* \ingroup analysis
@@ -48,18 +47,19 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
4847

4948
/**
5049
* Retrieves the feature with the specified \a id into \a feature.
51-
* It will be retrieved from the cache or from the underlying layer if unavailable.
52-
* If the feature is neither available from the cache nor from the layer it will return FALSE.
53-
* If \a feedback is specified, the call may return if the feedback is canceled.
50+
* It will be retrieved from the cache or from the underlying feature source if unavailable.
51+
* If the feature is neither available from the cache nor from the source it will return FALSE.
5452
*/
55-
bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = nullptr );
53+
bool getFeature( QgsFeatureId id, QgsFeature &feature );
5654

5755
/**
5856
* Gets features for the provided \a request. No features will be fetched
5957
* from the cache and the request is sent directly to the underlying feature source.
6058
* Results of the request are cached in the pool and the ids of all the features
61-
* are returned. This can be used to warm the cache for a particular area of interest
59+
* are returned. This is used to warm the cache for a particular area of interest
6260
* (bounding box) or other set of features.
61+
* This will get a new feature source from the source vector layer.
62+
* This needs to be called from the main thread.
6363
* If \a feedback is specified, the call may return if the feedback is canceled.
6464
*/
6565
QgsFeatureIds getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback = nullptr ) SIP_SKIP;
@@ -125,13 +125,21 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
125125
*/
126126
QgsCoordinateReferenceSystem crs() const;
127127

128+
/**
129+
* Returns the name of the layer.
130+
*
131+
* Should be preferred over layer().name() because it can directly be run on
132+
* the background thread.
133+
*/
134+
QString layerName() const;
135+
128136
protected:
129137

130138
/**
131139
* Inserts a feature into the cache and the spatial index.
132140
* To be used by implementations of ``addFeature``.
133141
*/
134-
void insertFeature( const QgsFeature &feature );
142+
void insertFeature( const QgsFeature &feature, bool skipLock = false );
135143

136144
/**
137145
* Changes a feature in the cache and the spatial index.
@@ -174,9 +182,9 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
174182
mutable QReadWriteLock mCacheLock;
175183
QgsFeatureIds mFeatureIds;
176184
QgsSpatialIndex mIndex;
177-
QString mLayerId;
178185
QgsWkbTypes::GeometryType mGeometryType;
179-
QgsCoordinateReferenceSystem mCrs;
186+
std::unique_ptr<QgsVectorLayerFeatureSource> mFeatureSource;
187+
QString mLayerName;
180188
};
181189

182190
#endif // QGS_FEATUREPOOL_H

‎src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,17 +74,17 @@ QgsGeometry QgsGeometryCheckerUtils::LayerFeature::geometry() const
7474

7575
QString QgsGeometryCheckerUtils::LayerFeature::id() const
7676
{
77-
return QStringLiteral( "%1:%2" ).arg( layer()->name() ).arg( mFeature.id() );
77+
return QStringLiteral( "%1:%2" ).arg( mFeaturePool->layerName() ).arg( mFeature.id() );
7878
}
7979

8080
bool QgsGeometryCheckerUtils::LayerFeature::operator==( const LayerFeature &other ) const
8181
{
82-
return layer()->id() == other.layer()->id() && feature().id() == other.feature().id();
82+
return layerId() == other.layerId() && mFeature.id() == other.mFeature.id();
8383
}
8484

8585
bool QgsGeometryCheckerUtils::LayerFeature::operator!=( const LayerFeature &other ) const
8686
{
87-
return layer()->id() != other.layer()->id() || feature().id() != other.feature().id();
87+
return layerId() != other.layerId() || mFeature.id() != other.mFeature.id();
8888
}
8989

9090
/////////////////////////////////////////////////////////////////////////////
@@ -197,7 +197,7 @@ bool QgsGeometryCheckerUtils::LayerFeatures::iterator::nextFeature( bool begin )
197197
QgsFeature feature;
198198
if ( featurePool->getFeature( *mFeatureIt, feature ) && !feature.geometry().isNull() )
199199
{
200-
mCurrentFeature.reset( new LayerFeature( mParent->mFeaturePools[*mLayerIt], feature, mParent->mContext, mParent->mUseMapCrs ) );
200+
mCurrentFeature = qgis::make_unique<LayerFeature>( featurePool, feature, mParent->mContext, mParent->mUseMapCrs );
201201
return true;
202202
}
203203
++mFeatureIt;

‎src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg
151151
if ( fid == currentFeature.id() )
152152
continue;
153153

154-
if ( featurePool->getFeature( fid, compareFeature, feedback ) )
154+
if ( featurePool->getFeature( fid, compareFeature ) )
155155
{
156156
if ( feedback && feedback->isCanceled() )
157157
break;

‎tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -142,17 +142,29 @@ void TestQgsVectorLayerFeaturePool::changeGeometry()
142142
feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((100 100, 110 100, 110 110, 100 110, 100 100))" ) ) );
143143
vl->updateFeature( feat );
144144

145-
// No features in the AOI
145+
// Still working on the cached data
146146
QgsFeatureIds ids3 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
147-
QCOMPARE( ids3.size(), 0 );
147+
QCOMPARE( ids3.size(), 1 );
148+
149+
// Repopulate the cache
150+
QgsFeatureIds ids4 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
151+
QCOMPARE( ids4.size(), 0 );
152+
153+
// Still working on the cached data
154+
QgsFeatureIds ids5 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
155+
QCOMPARE( ids5.size(), 0 );
148156

149157
// Update a feature to be inside the AOI
150158
feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((0 0, 10 0, 10 10, 0 10, 0 0))" ) ) );
151159
vl->updateFeature( feat );
152160

153-
// One there again
154-
QgsFeatureIds ids4 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
155-
QCOMPARE( ids4.size(), 1 );
161+
// Still cached
162+
QgsFeatureIds ids6 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
163+
QCOMPARE( ids6.size(), 0 );
164+
165+
// One in there again
166+
QgsFeatureIds ids7 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
167+
QCOMPARE( ids7.size(), 1 );
156168
}
157169

158170
std::unique_ptr<QgsVectorLayer> TestQgsVectorLayerFeaturePool::createPopulatedLayer()

0 commit comments

Comments
 (0)
Please sign in to comment.