Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inconsistency between layer providers in geometry checker #58132

Closed
wants to merge 4 commits into from
Closed

Fix inconsistency between layer providers in geometry checker #58132

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3393264
fix(qgsfeaturepool): cache management - fixes #58113
Djedouas Aug 1, 2024
57fce5a
fix(qgsfeaturepool): correctly remove features from spatial index
Djedouas Aug 6, 2024
cf0da99
fix(qgsfeaturepool): better synchronization between cache and spatial…
Djedouas Aug 27, 2024
542c10a
fix(qgsfeaturepool): fix locker mode
Djedouas Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,14 @@ To be used by implementations of ``addFeature``.
the original layer's thread.
%End

void refreshCache( QgsFeature feature );
void refreshCache( QgsFeature feature, const QgsFeature origFeature );
%Docstring
Changes a feature in the cache and the spatial index.
To be used by implementations of ``updateFeature``.

:param feature: the new feature to put in the cache and index
:param origFeature: the original feature to remove from the index

.. note::

This method can safely be called from a different thread vs the object's creation thread or
Expand Down
5 changes: 4 additions & 1 deletion python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,14 @@ To be used by implementations of ``addFeature``.
the original layer's thread.
%End

void refreshCache( QgsFeature feature );
void refreshCache( QgsFeature feature, const QgsFeature origFeature );
%Docstring
Changes a feature in the cache and the spatial index.
To be used by implementations of ``updateFeature``.

:param feature: the new feature to put in the cache and index
:param origFeature: the original feature to remove from the index

.. note::

This method can safely be called from a different thread vs the object's creation thread or
Expand Down
4 changes: 2 additions & 2 deletions src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,11 @@ void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
mIndex.addFeature( indexFeature );
}

void QgsFeaturePool::refreshCache( QgsFeature feature )
void QgsFeaturePool::refreshCache( QgsFeature feature, const QgsFeature origFeature )
{
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
mIndex.deleteFeature( feature );
mIndex.deleteFeature( origFeature );
mIndex.addFeature( feature );
locker.unlock();
}
Expand Down
5 changes: 4 additions & 1 deletion src/analysis/vector/geometry_checker/qgsfeaturepool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,13 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
* Changes a feature in the cache and the spatial index.
* To be used by implementations of ``updateFeature``.
*
* \param feature the new feature to put in the cache and index
* \param origFeature the original feature to remove from the index
*
* \note This method can safely be called from a different thread vs the object's creation thread or
* the original layer's thread.
*/
void refreshCache( QgsFeature feature );
void refreshCache( QgsFeature feature, const QgsFeature origFeature );

/**
* Removes a feature from the cache and the spatial index.
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ void QgsVectorDataProviderFeaturePool::updateFeature( QgsFeature &feature )
}
} );

refreshCache( feature );
refreshCache( feature, origFeature );
}

void QgsVectorDataProviderFeaturePool::deleteFeature( QgsFeatureId fid )
Expand Down
18 changes: 12 additions & 6 deletions src/analysis/vector/geometry_checker/qgsvectorlayerfeaturepool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ bool QgsVectorLayerFeaturePool::addFeatures( QgsFeatureList &features, QgsFeatur

void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature )
{
QgsFeature origFeature;
getFeature( feature.id(), origFeature );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be getting confused here, but I think there's still a risk of index corruption in this scenario:

  1. Feature A is retrieved (version "1"), it is cached in the pool. Bounds are used in the spatial index.
  2. More features are fetched, cache fills up, feature A is evicted from cache
  3. Some external caller changes the geometry on feature A in the vector layer or data provider, now there's version 2 of feature A.
  4. A call to updateFeature for feature A is made. getFeature won't find it in the pool's cache (it was evicted), so it's retrieved from the layer. We get feature A version 2.
  5. Version 2 is passed to refreshCache, and the index is corrupted because we try to remove different bounds (version 2, whereas the cache is using v1)

I don't see any alternative but keeping a local map/hash of the feature bounds which were used in the index.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I did not expect that the spatial index could evict a feature... It is an index, not strictly a cache, isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't get evicted from the spatial index, but will from mFeatureCache.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course 🙈 obviously.

Okay, what about "synchronizing" the cache and the index?

In fact, in the end of step 4. inside the index, we have both feature A version 2 and feature A version 1.

We could fix this: when getFeature doesn't find a cached feature, we can proactively delete any pre-existing corresponding feature in the index (see new commit).

If the feature was not present in the index, it does nearly nothing, otherwise it removes the possibly out-of-date feature from the index, to re-add the possibly same or up-to-date feature.


QgsThreadingUtils::runOnMainThread( [this, &feature]()
{
QgsVectorLayer *lyr = layer();
Expand All @@ -117,7 +120,7 @@ void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature )
}
} );

refreshCache( feature );
refreshCache( feature, origFeature );
}

void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )
Expand All @@ -135,14 +138,17 @@ void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )

void QgsVectorLayerFeaturePool::onGeometryChanged( QgsFeatureId fid, const QgsGeometry &geometry )
{
Q_UNUSED( geometry )

QgsFeature feature;
if ( isFeatureCached( fid ) )
{
QgsFeature feature;
getFeature( fid, feature );
refreshCache( feature );
QgsFeature origFeature;
getFeature( fid, origFeature ); // gets the cached feature
feature = origFeature;
feature.setGeometry( geometry );
refreshCache( feature, origFeature );
}
else
getFeature( fid, feature ); // gets the feature and adds it to the cache and index
}

void QgsVectorLayerFeaturePool::onFeatureDeleted( QgsFeatureId fid )
Expand Down
22 changes: 7 additions & 15 deletions tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,29 +142,21 @@ void TestQgsVectorLayerFeaturePool::changeGeometry()
feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((100 100, 110 100, 110 110, 100 110, 100 100))" ) ) );
vl->updateFeature( feat );

// Still working on the cached data
// Cached data updated with geometryChanged vector layer signal
const QgsFeatureIds ids3 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids3.size(), 1 );

// Repopulate the cache
const QgsFeatureIds ids4 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
QCOMPARE( ids4.size(), 0 );
QCOMPARE( ids3.size(), 0 );

// Still working on the cached data
const QgsFeatureIds ids5 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids5.size(), 0 );
const QgsFeatureIds ids4 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids4.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 );

// Still cached
const QgsFeatureIds ids6 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids6.size(), 0 );

// One in there again
const QgsFeatureIds ids7 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
QCOMPARE( ids7.size(), 1 );
// Cached data updated with geometryChanged vector layer signal
const QgsFeatureIds ids5 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids5.size(), 1 );
}

std::unique_ptr<QgsVectorLayer> TestQgsVectorLayerFeaturePool::createPopulatedLayer()
Expand Down