Fix more multithreading issues

qgis · Sep 11, 2018 · e192e18 · e192e18
1 parent 6d2ec20
commit e192e18
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 24 deletions.
diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
@@ -81,6 +81,11 @@ QgsVectorLayer *QgsFeaturePool::layer() const
   return mLayer.data();
 }
 
+QPointer<QgsVectorLayer> QgsFeaturePool::layerPtr() const
+{
+  return mLayer;
+}
+
 void QgsFeaturePool::insertFeature( const QgsFeature &feature )
 {
   QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );

diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.h b/src/analysis/vector/geometry_checker/qgsfeaturepool.h
@@ -79,6 +79,8 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink
      */
     QgsVectorLayer *layer() const;
 
+    QPointer<QgsVectorLayer> layerPtr() const;
+
     /**
      * The layer id of the layer.
      */

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheck.cpp b/src/analysis/vector/geometry_checker/qgsgeometrycheck.cpp
@@ -18,6 +18,22 @@
 #include "qgsgeometrycheck.h"
 #include "qgsfeaturepool.h"
 #include "qgsvectorlayer.h"
+#include "qgsreadwritelocker.h"
+
+template <typename Func>
+void runOnMainThread( const Func &func )
+{
+#if QT_VERSION >= QT_VERSION_CHECK( 5, 10, 0 )
+  // Make sure we only deal with the vector layer on the main thread where it lives.
+  // Anything else risks a crash.
+  if ( QThread::currentThread() == qApp->thread() )
+    func();
+  else
+    QMetaObject::invokeMethod( qApp, func, Qt::BlockingQueuedConnection );
+#else
+  func();
+#endif
+}
 
 QgsGeometryCheckerContext::QgsGeometryCheckerContext( int _precision, const QgsCoordinateReferenceSystem &_mapCrs, const QMap<QString, QgsFeaturePool *> &_featurePools, const QgsCoordinateTransformContext &transformContext )
   : tolerance( std::pow( 10, -_precision ) )
@@ -28,27 +44,42 @@ QgsGeometryCheckerContext::QgsGeometryCheckerContext( int _precision, const QgsC
 {
 }
 
-const QgsCoordinateTransform &QgsGeometryCheckerContext::layerTransform( QgsVectorLayer *layer )
+const QgsCoordinateTransform &QgsGeometryCheckerContext::layerTransform( const QPointer<QgsVectorLayer> &layer )
 {
-  Q_ASSERT( QThread::currentThread() == qApp->thread() );
-
+  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
   if ( !mTransformCache.contains( layer ) )
   {
-    QgsCoordinateTransform transform = QgsCoordinateTransform( layer->crs(), mapCrs, transformContext );
+    QgsCoordinateTransform transform;
+    runOnMainThread( [this, &transform, layer]()
+    {
+      QgsVectorLayer *lyr = layer.data();
+      if ( lyr )
+        transform = QgsCoordinateTransform( lyr->crs(), mapCrs, transformContext );
+    } );
+    locker.changeMode( QgsReadWriteLocker::Write );
     mTransformCache[layer] = transform;
+    locker.changeMode( QgsReadWriteLocker::Read );
   }
 
   return mTransformCache[layer];
 }
 
-double QgsGeometryCheckerContext::layerScaleFactor( QgsVectorLayer *layer )
+double QgsGeometryCheckerContext::layerScaleFactor( const QPointer<QgsVectorLayer> &layer )
 {
-  Q_ASSERT( QThread::currentThread() == qApp->thread() );
-
+  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
   if ( !mScaleFactorCache.contains( layer ) )
   {
-    double scaleFactor = layerTransform( layer ).scaleFactor( layer->extent() );
+    double scaleFactor = 1.0;
+    runOnMainThread( [this, layer, &scaleFactor]()
+    {
+      QgsVectorLayer *lyr = layer.data();
+      if ( lyr )
+        scaleFactor = layerTransform( layer ).scaleFactor( lyr->extent() );
+    } );
+
+    locker.changeMode( QgsReadWriteLocker::Write );
     mScaleFactorCache[layer] = scaleFactor;
+    locker.changeMode( QgsReadWriteLocker::Read );
   }
 
   return mScaleFactorCache.value( layer );
@@ -76,7 +107,7 @@ QgsGeometryCheckError::QgsGeometryCheckError( const QgsGeometryCheck *check,
     const QgsPointXY &errorLocation, QgsVertexId vidx,
     const QVariant &value, ValueType valueType )
   : mCheck( check )
-  , mLayerId( layerFeature.layer()->id() )
+  , mLayerId( layerFeature.layerId() )
   , mFeatureId( layerFeature.feature().id() )
   , mErrorLocation( errorLocation )
   , mVidx( vidx )

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheck.h b/src/analysis/vector/geometry_checker/qgsgeometrycheck.h
@@ -21,8 +21,11 @@
 #include <QApplication>
 #include <limits>
 #include <QStringList>
+#include <QPointer>
+
 #include "qgis_analysis.h"
 #include "qgsfeature.h"
+#include "qgsvectorlayer.h"
 #include "geometry/qgsgeometry.h"
 #include "qgsgeometrycheckerutils.h"
 
@@ -39,12 +42,13 @@ struct ANALYSIS_EXPORT QgsGeometryCheckerContext
     const QgsCoordinateReferenceSystem mapCrs;
     const QMap<QString, QgsFeaturePool *> featurePools;
     const QgsCoordinateTransformContext transformContext;
-    const QgsCoordinateTransform &layerTransform( QgsVectorLayer *layer );
-    double layerScaleFactor( QgsVectorLayer *layer );
+    const QgsCoordinateTransform &layerTransform( const QPointer<QgsVectorLayer> &layer );
+    double layerScaleFactor( const QPointer<QgsVectorLayer> &layer );
 
   private:
-    QMap<QgsVectorLayer *, QgsCoordinateTransform> mTransformCache;
-    QMap<QgsVectorLayer *, double> mScaleFactorCache;
+    QMap<QPointer<QgsVectorLayer>, QgsCoordinateTransform> mTransformCache;
+    QMap<QPointer<QgsVectorLayer>, double> mScaleFactorCache;
+    QReadWriteLock mCacheLock;
 };
 
 class ANALYSIS_EXPORT QgsGeometryCheck

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrychecker.h b/src/analysis/vector/geometry_checker/qgsgeometrychecker.h
@@ -23,11 +23,10 @@
 #include <QList>
 #include <QMutex>
 #include <QStringList>
-#include <memory>
-
 #include "qgis_analysis.h"
-#include "qgsfeatureid.h"
 
+typedef qint64 QgsFeatureId;
+typedef QSet<QgsFeatureId> QgsFeatureIds;
 struct QgsGeometryCheckerContext;
 class QgsGeometryCheck;
 class QgsGeometryCheckError;
@@ -57,10 +56,6 @@ class ANALYSIS_EXPORT QgsGeometryChecker : public QObject
     class RunCheckWrapper
     {
       public:
-
-        /**
-         * The caller needs to make sure that the context is not deleted before this thread ends.
-         */
         explicit RunCheckWrapper( QgsGeometryChecker *instance );
         void operator()( const QgsGeometryCheck *check );
       private:

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp b/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp
@@ -35,7 +35,7 @@ namespace QgsGeometryCheckerUtils
     , mMapCrs( useMapCrs )
   {
     mGeometry = feature.geometry().constGet()->clone();
-    const QgsCoordinateTransform &transform = context->layerTransform( mFeaturePool->layer() );
+    const QgsCoordinateTransform &transform = context->layerTransform( mFeaturePool->layerPtr() );
     if ( useMapCrs && context->mapCrs.isValid() && !transform.isShortCircuited() )
     {
       mGeometry->transform( transform );
@@ -47,9 +47,14 @@ namespace QgsGeometryCheckerUtils
     delete mGeometry;
   }
 
-  QgsVectorLayer *LayerFeature::layer() const
+  QPointer<QgsVectorLayer> LayerFeature::layer() const
   {
-    return mFeaturePool->layer();
+    return mFeaturePool->layerPtr();
+  }
+
+  QString LayerFeature::layerId() const
+  {
+    return mFeaturePool->layerId();
   }
 
   QString LayerFeature::id() const

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.h b/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.h
@@ -36,7 +36,8 @@ namespace QgsGeometryCheckerUtils
       LayerFeature( const QgsFeaturePool *pool, const QgsFeature &feature, QgsGeometryCheckerContext *context, bool useMapCrs );
       ~LayerFeature();
       const QgsFeature &feature() const { return mFeature; }
-      QgsVectorLayer *layer() const;
+      QPointer<QgsVectorLayer> layer() const;
+      QString layerId() const;
       double layerToMapUnits() const;
       const QgsCoordinateTransform &layerToMapTransform() const;
       const QgsAbstractGeometry *geometry() const { return mGeometry; }