RAII for QgsCoordinateTransform locking

m-kuhn · m-kuhn · commit 1730a40a8235 · 2018-12-17T17:26:57.000+01:00
I experienced a freeze where the main thread was stuck in QgsCoordinateTransformPrivate::freeProj(), waiting for a write lock.
None of the other threads had anything useful in their stack trace that would have explained why the lock was not available.
The only explanation I can come up with is that an exception or thread termination was messing with the lock, leaving it in a
locked state.
By using RAII we can avoid this scenario.
diff --git a/src/core/qgscoordinatetransform_p.cpp b/src/core/qgscoordinatetransform_p.cpp
@@ -18,6 +18,7 @@
 #include "qgscoordinatetransform_p.h"
 #include "qgslogger.h"
 #include "qgsapplication.h"
+#include "qgsreadwritelocker.h"
 
 extern "C"
 {
@@ -225,7 +226,7 @@ void QgsCoordinateTransformPrivate::calculateTransforms( const QgsCoordinateTran
 
 QPair<projPJ, projPJ> QgsCoordinateTransformPrivate::threadLocalProjData()
 {
-  mProjLock.lockForRead();
+  QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Read );
 
 #ifdef USE_THREAD_LOCAL
   QMap < uintptr_t, QPair< projPJ, projPJ > >::const_iterator it = mProjProjections.constFind( reinterpret_cast< uintptr_t>( mProjContext.get() ) );
@@ -246,13 +247,11 @@ QPair<projPJ, projPJ> QgsCoordinateTransformPrivate::threadLocalProjData()
   if ( it != mProjProjections.constEnd() )
   {
     QPair<projPJ, projPJ> res = it.value();
-    mProjLock.unlock();
     return res;
   }
 
   // proj projections don't exist yet, so we need to create
-  mProjLock.unlock();
-  mProjLock.lockForWrite();
+  locker.changeMode( QgsReadWriteLocker::Write );
 
 #ifdef USE_THREAD_LOCAL
   QPair<projPJ, projPJ> res = qMakePair( pj_init_plus_ctx( mProjContext.get(), mSourceProjString.toUtf8() ),
@@ -263,7 +262,6 @@ QPair<projPJ, projPJ> QgsCoordinateTransformPrivate::threadLocalProjData()
                                          pj_init_plus_ctx( pContext, mDestProjString.toUtf8() ) );
   mProjProjections.insert( reinterpret_cast< uintptr_t>( pContext ), res );
 #endif
-  mProjLock.unlock();
   return res;
 }
 
@@ -332,15 +330,14 @@ void QgsCoordinateTransformPrivate::setFinder()
 
 void QgsCoordinateTransformPrivate::freeProj()
 {
-  mProjLock.lockForWrite();
+  QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Write );
   QMap < uintptr_t, QPair< projPJ, projPJ > >::const_iterator it = mProjProjections.constBegin();
   for ( ; it != mProjProjections.constEnd(); ++it )
   {
     pj_free( it.value().first );
     pj_free( it.value().second );
   }
   mProjProjections.clear();
-  mProjLock.unlock();
 }
 
 ///@endcond

Original file line number	Diff line number	Diff line change
`@@ -18,6 +18,7 @@`
`18`	`18`	`#include "qgscoordinatetransform_p.h"`
`19`	`19`	`#include "qgslogger.h"`
`20`	`20`	`#include "qgsapplication.h"`
	`21`	`+#include "qgsreadwritelocker.h"`
`21`	`22`
`22`	`23`	`extern "C"`
`23`	`24`	`{`
`@@ -225,7 +226,7 @@ void QgsCoordinateTransformPrivate::calculateTransforms( const QgsCoordinateTran`
`225`	`226`
`226`	`227`	`QPair<projPJ, projPJ> QgsCoordinateTransformPrivate::threadLocalProjData()`
`227`	`228`	`{`
`228`		`- mProjLock.lockForRead();`
	`229`	`+ QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Read );`
`229`	`230`
`230`	`231`	`#ifdef USE_THREAD_LOCAL`
`231`	`232`	`QMap < uintptr_t, QPair< projPJ, projPJ > >::const_iterator it = mProjProjections.constFind( reinterpret_cast< uintptr_t>( mProjContext.get() ) );`
`@@ -246,13 +247,11 @@ QPair<projPJ, projPJ> QgsCoordinateTransformPrivate::threadLocalProjData()`
`246`	`247`	`if ( it != mProjProjections.constEnd() )`
`247`	`248`	`{`
`248`	`249`	`QPair<projPJ, projPJ> res = it.value();`
`249`		`- mProjLock.unlock();`
`250`	`250`	`return res;`
`251`	`251`	`}`
`252`	`252`
`253`	`253`	`// proj projections don't exist yet, so we need to create`
`254`		`- mProjLock.unlock();`
`255`		`- mProjLock.lockForWrite();`
	`254`	`+ locker.changeMode( QgsReadWriteLocker::Write );`
`256`	`255`
`257`	`256`	`#ifdef USE_THREAD_LOCAL`
`258`	`257`	`QPair<projPJ, projPJ> res = qMakePair( pj_init_plus_ctx( mProjContext.get(), mSourceProjString.toUtf8() ),`
`@@ -263,7 +262,6 @@ QPair<projPJ, projPJ> QgsCoordinateTransformPrivate::threadLocalProjData()`
`263`	`262`	`pj_init_plus_ctx( pContext, mDestProjString.toUtf8() ) );`
`264`	`263`	`mProjProjections.insert( reinterpret_cast< uintptr_t>( pContext ), res );`
`265`	`264`	`#endif`
`266`		`- mProjLock.unlock();`
`267`	`265`	`return res;`
`268`	`266`	`}`
`269`	`267`
`@@ -332,15 +330,14 @@ void QgsCoordinateTransformPrivate::setFinder()`
`332`	`330`
`333`	`331`	`void QgsCoordinateTransformPrivate::freeProj()`
`334`	`332`	`{`
`335`		`- mProjLock.lockForWrite();`
	`333`	`+ QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Write );`
`336`	`334`	`QMap < uintptr_t, QPair< projPJ, projPJ > >::const_iterator it = mProjProjections.constBegin();`
`337`	`335`	`for ( ; it != mProjProjections.constEnd(); ++it )`
`338`	`336`	`{`
`339`	`337`	`pj_free( it.value().first );`
`340`	`338`	`pj_free( it.value().second );`
`341`	`339`	`}`
`342`	`340`	`mProjProjections.clear();`
`343`		`- mProjLock.unlock();`
`344`	`341`	`}`
`345`	`342`
`346`	`343`	`///@endcond`