Implement manual locks on QgsSpatialIndex

Since libspatialindex is not thread safe on all platforms, and
have expressed desire to remove the thread safety that they DO
have on remaining platforms, it's safer and easier for us
to manually add locks to QgsSpatialIndex and be gauranteed that
this class is thread safe on all platforms and libspatialindex
versions.

Also improve docs for the class.

Author: nyalldawson

python/core/qgsspatialindex.sip.in

@@ -16,6 +16,18 @@
 
 class QgsSpatialIndex
 {
+%Docstring
+
+A spatial index for QgsFeature objects.
+
+QgsSpatialIndex objects are implicitly shared and can be inexpensively copied.
+
+.. note::
+
+   While the underlying libspatialindex is not thread safe on some platforms, the QgsSpatialIndex
+   class implements its own locks and accordingly, a single QgsSpatialIndex object can safely
+   be used across multiple threads.
+%End
 
 %TypeHeaderCode
 #include "qgsspatialindex.h"
@@ -25,7 +37,7 @@ class QgsSpatialIndex
 
     QgsSpatialIndex();
 %Docstring
-Constructor - creates R-tree
+Constructor for QgsSpatialIndex. Creates an empty R-tree index.
 %End
 
     explicit QgsSpatialIndex( const QgsFeatureIterator &fi, QgsFeedback *feedback = 0 );
@@ -60,24 +72,10 @@ Copy constructor
     ~QgsSpatialIndex();
 
 
-    void detach();
-%Docstring
-Detaches the index, forcing a deep copy of the underlying
-spatial index data.
-
-Since the underlying libspatialindex is not thread safe on some platforms (e.g. Windows),
-manual calls to detach() must be made if a QgsSpatialIndex is to be accessed across multiple threads.
-
-Note that for platforms on which libspatialindex is thread safe, calling
-detach() has no effect and does not force the deep copy.
 
-.. versionadded:: 3.0
-%End
-
-
-    bool insertFeature( const QgsFeature &f );
+    bool insertFeature( const QgsFeature &feature );
 %Docstring
-Add feature to index
+Adds a ``feature`` to the index.
 %End
 
     bool insertFeature( QgsFeatureId id, const QgsRectangle &bounds );
@@ -89,21 +87,33 @@ Add a feature ``id`` to the index with a specified bounding box.
 .. versionadded:: 3.0
 %End
 
-    bool deleteFeature( const QgsFeature &f );
+    bool deleteFeature( const QgsFeature &feature );
 %Docstring
-Remove feature from index
+Removes a ``feature`` from the index.
 %End
 
 
 
-    QList<QgsFeatureId> intersects( const QgsRectangle &rect ) const;
+    QList<QgsFeatureId> intersects( const QgsRectangle &rectangle ) const;
 %Docstring
-Returns features that intersect the specified rectangle
+Returns a list of features with a bounding box which intersects the specified ``rectangle``.
+
+.. note::
+
+   The intersection test is performed based on the feature bounding boxes only, so for non-point
+   geometry features it is necessary to manually test the returned features for exact geometry intersection
+   when required.
 %End
 
     QList<QgsFeatureId> nearestNeighbor( const QgsPointXY &point, int neighbors ) const;
 %Docstring
-Returns nearest neighbors (their count is specified by second parameter)
+Returns nearest neighbors to a ``point``. The number of neighbours returned is specified
+by the ``neighbours`` argument.
+
+.. note::
+
+   The nearest neighbour test is performed based on the feature bounding boxes only, so for non-point
+   geometry features this method is not guaranteed to return the actual closest neighbours.
 %End
 
 

src/core/providers/memory/qgsmemoryfeatureiterator.cpp

@@ -231,17 +231,13 @@ bool QgsMemoryFeatureIterator::close()
 QgsMemoryFeatureSource::QgsMemoryFeatureSource( const QgsMemoryProvider *p )
   : mFields( p->mFields )
   , mFeatures( p->mFeatures )
-  , mSpatialIndex( p->mSpatialIndex ? qgis::make_unique< QgsSpatialIndex >( *p->mSpatialIndex ) : nullptr ) // this is just a shallow copy, but see below...
+  , mSpatialIndex( p->mSpatialIndex ? qgis::make_unique< QgsSpatialIndex >( *p->mSpatialIndex ) : nullptr ) // just shallow copy
   , mSubsetString( p->mSubsetString )
   , mCrs( p->mCrs )
 {
   mExpressionContext << QgsExpressionContextUtils::globalScope()
                      << QgsExpressionContextUtils::projectScope( QgsProject::instance() );
   mExpressionContext.setFields( mFields );
-
-  // QgsSpatialIndex is not thread safe - so make spatial index safe to use across threads by forcing a full deep copy
-  if ( mSpatialIndex && p->thread() != QThread::currentThread() )
-    mSpatialIndex->detach();
 }
 
 QgsFeatureIterator QgsMemoryFeatureSource::getFeatures( const QgsFeatureRequest &request )

src/core/qgsspatialindex.cpp

@@ -24,6 +24,8 @@
 #include "qgsfeedback.h"
 
 #include "SpatialIndex.h"
+#include <QMutex>
+#include <QMutexLocker>
 
 using namespace SpatialIndex;
 
@@ -183,6 +185,8 @@ class QgsSpatialIndexData : public QSharedData
     QgsSpatialIndexData( const QgsSpatialIndexData &other )
       : QSharedData( other )
     {
+      QMutexLocker locker( &other.mMutex );
+
       initTree();
 
       // copy R-tree data one by one (is there a faster way??)
@@ -230,6 +234,8 @@ class QgsSpatialIndexData : public QSharedData
     //! R-tree containing spatial index
     SpatialIndex::ISpatialIndex *mRTree = nullptr;
 
+    mutable QMutex mMutex;
+
 };
 
 // -------------------------------------------------------------------------
@@ -266,14 +272,6 @@ QgsSpatialIndex &QgsSpatialIndex::operator=( const QgsSpatialIndex &other )
   return *this;
 }
 
-void QgsSpatialIndex::detach()
-{
-  // libspatialindex is not thread safe on windows - so force the deep copy
-#if defined(Q_OS_WIN)
-  d.detach();
-#endif
-}
-
 SpatialIndex::Region QgsSpatialIndex::rectToRegion( const QgsRectangle &rect )
 {
   double pt1[2] = { rect.xMinimum(), rect.yMinimum() },
@@ -315,6 +313,8 @@ bool QgsSpatialIndex::insertFeature( QgsFeatureId id, const QgsRectangle &rect )
 {
   SpatialIndex::Region r( rectToRegion( rect ) );
 
+  QMutexLocker locker( &d->mMutex );
+
   // TODO: handle possible exceptions correctly
   try
   {
@@ -346,6 +346,7 @@ bool QgsSpatialIndex::deleteFeature( const QgsFeature &f )
   if ( !featureInfo( f, r, id ) )
     return false;
 
+  QMutexLocker locker( &d->mMutex );
   // TODO: handle exceptions
   return d->mRTree->deleteData( r, FID_TO_NUMBER( id ) );
 }
@@ -357,6 +358,7 @@ QList<QgsFeatureId> QgsSpatialIndex::intersects( const QgsRectangle &rect ) cons
 
   SpatialIndex::Region r = rectToRegion( rect );
 
+  QMutexLocker locker( &d->mMutex );
   d->mRTree->intersectsWithQuery( r, visitor );
 
   return list;
@@ -370,6 +372,7 @@ QList<QgsFeatureId> QgsSpatialIndex::nearestNeighbor( const QgsPointXY &point, i
   double pt[2] = { point.x(), point.y() };
   Point p( pt, 2 );
 
+  QMutexLocker locker( &d->mMutex );
   d->mRTree->nearestNeighborQuery( neighbors, p, visitor );
 
   return list;

src/core/qgsspatialindex.h

@@ -52,6 +52,14 @@ class QgsFeatureSource;
 /**
  * \ingroup core
  * \class QgsSpatialIndex
+ *
+ * A spatial index for QgsFeature objects.
+ *
+ * QgsSpatialIndex objects are implicitly shared and can be inexpensively copied.
+ *
+ * \note While the underlying libspatialindex is not thread safe on some platforms, the QgsSpatialIndex
+ * class implements its own locks and accordingly, a single QgsSpatialIndex object can safely
+ * be used across multiple threads.
  */
 class CORE_EXPORT QgsSpatialIndex
 {
@@ -60,7 +68,9 @@ class CORE_EXPORT QgsSpatialIndex
 
     /* creation of spatial index */
 
-    //! Constructor - creates R-tree
+    /**
+     * Constructor for QgsSpatialIndex. Creates an empty R-tree index.
+     */
     QgsSpatialIndex();
 
     /**
@@ -97,24 +107,12 @@ class CORE_EXPORT QgsSpatialIndex
     //! Implement assignment operator
     QgsSpatialIndex &operator=( const QgsSpatialIndex &other );
 
-    /**
-     * Detaches the index, forcing a deep copy of the underlying
-     * spatial index data.
-     *
-     * Since the underlying libspatialindex is not thread safe on some platforms (e.g. Windows),
-     * manual calls to detach() must be made if a QgsSpatialIndex is to be accessed across multiple threads.
-     *
-     * Note that for platforms on which libspatialindex is thread safe, calling
-     * detach() has no effect and does not force the deep copy.
-     *
-     * \since QGIS 3.0
-     */
-    void detach();
-
     /* operations */
 
-    //! Add feature to index
-    bool insertFeature( const QgsFeature &f );
+    /**
+     * Adds a \a feature to the index.
+     */
+    bool insertFeature( const QgsFeature &feature );
 
     /**
      * Add a feature \a id to the index with a specified bounding box.
@@ -123,16 +121,30 @@ class CORE_EXPORT QgsSpatialIndex
     */
     bool insertFeature( QgsFeatureId id, const QgsRectangle &bounds );
 
-    //! Remove feature from index
-    bool deleteFeature( const QgsFeature &f );
+    /**
+     * Removes a \a feature from the index.
+     */
+    bool deleteFeature( const QgsFeature &feature );
 
 
     /* queries */
 
-    //! Returns features that intersect the specified rectangle
-    QList<QgsFeatureId> intersects( const QgsRectangle &rect ) const;
+    /**
+     * Returns a list of features with a bounding box which intersects the specified \a rectangle.
+     *
+     * \note The intersection test is performed based on the feature bounding boxes only, so for non-point
+     * geometry features it is necessary to manually test the returned features for exact geometry intersection
+     * when required.
+     */
+    QList<QgsFeatureId> intersects( const QgsRectangle &rectangle ) const;
 
-    //! Returns nearest neighbors (their count is specified by second parameter)
+    /**
+     * Returns nearest neighbors to a \a point. The number of neighbours returned is specified
+     * by the \a neighbours argument.
+     *
+     * \note The nearest neighbour test is performed based on the feature bounding boxes only, so for non-point
+     * geometry features this method is not guaranteed to return the actual closest neighbours.
+     */
     QList<QgsFeatureId> nearestNeighbor( const QgsPointXY &point, int neighbors ) const;
 
     /* debugging */