Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nyalldawson committed Feb 9, 2018
1 parent 426c72f commit 8f902e7
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 57 deletions.
54 changes: 32 additions & 22 deletions python/core/qgsspatialindex.sip.in
Expand Up @@ -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"
Expand All @@ -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 );
Expand Down Expand Up @@ -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 );
Expand All @@ -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


Expand Down
6 changes: 1 addition & 5 deletions src/core/providers/memory/qgsmemoryfeatureiterator.cpp
Expand Up @@ -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 )
Expand Down
19 changes: 11 additions & 8 deletions src/core/qgsspatialindex.cpp
Expand Up @@ -24,6 +24,8 @@
#include "qgsfeedback.h"

#include "SpatialIndex.h"
#include <QMutex>
#include <QMutexLocker>

using namespace SpatialIndex;

Expand Down Expand Up @@ -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??)
Expand Down Expand Up @@ -230,6 +234,8 @@ class QgsSpatialIndexData : public QSharedData
//! R-tree containing spatial index
SpatialIndex::ISpatialIndex *mRTree = nullptr;

mutable QMutex mMutex;

};

// -------------------------------------------------------------------------
Expand Down Expand Up @@ -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() },
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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 ) );
}
Expand All @@ -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;
Expand All @@ -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;
Expand Down
56 changes: 34 additions & 22 deletions src/core/qgsspatialindex.h
Expand Up @@ -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
{
Expand All @@ -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();

/**
Expand Down Expand Up @@ -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.
Expand All @@ -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 */
Expand Down

0 comments on commit 8f902e7

Please sign in to comment.