diff --git a/python/core/auto_generated/qgsapplication.sip.in b/python/core/auto_generated/qgsapplication.sip.in index 9464fadbf85..2ec2ad6f277 100644 --- a/python/core/auto_generated/qgsapplication.sip.in +++ b/python/core/auto_generated/qgsapplication.sip.in @@ -797,6 +797,18 @@ Do not include generated variables (like system name, user name etc.) Set a single custom expression variable. .. versionadded:: 3.0 +%End + + int maxConcurrentConnectionsPerPool() const; +%Docstring +The maximum number of concurrent connections per connections pool. + +.. note:: + + QGIS may in some situations allocate more than this amount + of connections to avoid deadlocks. + +.. versionadded:: 3.4 %End %If (ANDROID) diff --git a/python/core/auto_generated/qgsfeaturerequest.sip.in b/python/core/auto_generated/qgsfeaturerequest.sip.in index 888595316cd..cc387388525 100644 --- a/python/core/auto_generated/qgsfeaturerequest.sip.in +++ b/python/core/auto_generated/qgsfeaturerequest.sip.in @@ -657,6 +657,34 @@ at this moment. A negative value (which is set by default) will wait forever. Only works if the provider supports this option. .. versionadded:: 3.0 +%End + + bool requestMayBeNested() const; +%Docstring +In case this request may be run nested within another already running +iteration on the same connection, set this to true. + +If this flag is true, this request will be able to make use of "spare" +connections to avoid deadlocks. + +For example, this should be set on requests that are issued from an +expression function. + +.. versionadded:: 3.4 +%End + + void setRequestMayBeNested( bool requestMayBeNested ); +%Docstring +In case this request may be run nested within another already running +iteration on the same connection, set this to true. + +If this flag is true, this request will be able to make use of "spare" +connections to avoid deadlocks. + +For example, this should be set on requests that are issued from an +expression function. + +.. versionadded:: 3.4 %End protected: diff --git a/src/core/expression/qgsexpressionfunction.cpp b/src/core/expression/qgsexpressionfunction.cpp index a943bd244da..f5466697b6f 100644 --- a/src/core/expression/qgsexpressionfunction.cpp +++ b/src/core/expression/qgsexpressionfunction.cpp @@ -3570,6 +3570,8 @@ static QVariant fcnGetFeatureById( const QVariantList &values, const QgsExpressi QgsFeatureRequest req; req.setFilterFid( fid ); + req.setConnectionTimeout( 10000 ); + req.setRequestMayBeNested( true ); QgsFeatureIterator fIt = vl->getFeatures( req ); QgsFeature fet; @@ -3611,6 +3613,8 @@ static QVariant fcnGetFeature( const QVariantList &values, const QgsExpressionCo req.setFilterExpression( QStringLiteral( "%1=%2" ).arg( QgsExpression::quotedColumnRef( attribute ), QgsExpression::quotedString( attVal.toString() ) ) ); req.setLimit( 1 ); + req.setConnectionTimeout( 10000 ); + req.setRequestMayBeNested( true ); if ( !parent->needsGeometry() ) { req.setFlags( QgsFeatureRequest::NoGeometry ); diff --git a/src/core/qgsapplication.cpp b/src/core/qgsapplication.cpp index e7b9d9ad245..b6e6a566f01 100644 --- a/src/core/qgsapplication.cpp +++ b/src/core/qgsapplication.cpp @@ -85,6 +85,8 @@ #include // for setting gdal options #include +#define CONN_POOL_MAX_CONCURRENT_CONNS 4 + QObject *ABISYM( QgsApplication::mFileOpenEventReceiver ); QStringList ABISYM( QgsApplication::mFileOpenEventList ); QString ABISYM( QgsApplication::mPrefixPath ); @@ -1461,6 +1463,10 @@ void QgsApplication::setCustomVariable( const QString &name, const QVariant &val emit instance()->customVariablesChanged(); } +int QgsApplication::maxConcurrentConnectionsPerPool() const +{ + return CONN_POOL_MAX_CONCURRENT_CONNS; +} QString QgsApplication::nullRepresentation() { diff --git a/src/core/qgsapplication.h b/src/core/qgsapplication.h index d13adbfdd55..22806acdca0 100644 --- a/src/core/qgsapplication.h +++ b/src/core/qgsapplication.h @@ -732,6 +732,16 @@ class CORE_EXPORT QgsApplication : public QApplication */ static void setCustomVariable( const QString &name, const QVariant &value ); + /** + * The maximum number of concurrent connections per connections pool. + * + * \note QGIS may in some situations allocate more than this amount + * of connections to avoid deadlocks. + * + * \since QGIS 3.4 + */ + int maxConcurrentConnectionsPerPool() const; + #ifdef SIP_RUN SIP_IF_FEATURE( ANDROID ) //dummy method to workaround sip generation issue diff --git a/src/core/qgsconnectionpool.h b/src/core/qgsconnectionpool.h index ddfd35ac9fb..85b7722bb3a 100644 --- a/src/core/qgsconnectionpool.h +++ b/src/core/qgsconnectionpool.h @@ -19,6 +19,7 @@ #define SIP_NO_FILE #include "qgis.h" +#include "qgsapplication.h" #include #include #include @@ -29,8 +30,8 @@ #include -#define CONN_POOL_MAX_CONCURRENT_CONNS 4 #define CONN_POOL_EXPIRATION_TIME 60 // in seconds +#define CONN_POOL_SPARE_CONNECTIONS 2 // number of spare connections in case all the base connections are used but we have a nested request with the risk of a deadlock /** @@ -59,8 +60,6 @@ class QgsConnectionPoolGroup { public: - static const int MAX_CONCURRENT_CONNECTIONS; - struct Item { T c; @@ -69,7 +68,7 @@ class QgsConnectionPoolGroup QgsConnectionPoolGroup( const QString &ci ) : connInfo( ci ) - , sem( CONN_POOL_MAX_CONCURRENT_CONNS ) + , sem( QgsApplication::instance()->maxConcurrentConnectionsPerPool() + CONN_POOL_SPARE_CONNECTIONS ) { } @@ -93,12 +92,13 @@ class QgsConnectionPoolGroup * * \returns initialized connection or nullptr if unsuccessful */ - T acquire( int timeout ) + T acquire( int timeout, bool requestMayBeNested ) { + const int requiredFreeConnectionCount = requestMayBeNested ? 1 : 3; // we are going to acquire a resource - if no resource is available, we will block here if ( timeout >= 0 ) { - if ( !sem.tryAcquire( 1, timeout ) ) + if ( !sem.tryAcquire( requiredFreeConnectionCount, timeout ) ) return nullptr; } else @@ -107,8 +107,9 @@ class QgsConnectionPoolGroup // tryAcquire is broken on Qt > 5.8 with negative timeouts - see // https://bugreports.qt.io/browse/QTBUG-64413 // https://lists.osgeo.org/pipermail/qgis-developer/2017-November/050456.html - sem.acquire( 1 ); + sem.acquire( requiredFreeConnectionCount ); } + sem.release( requiredFreeConnectionCount - 1 ); // quick (preferred) way - use cached connection { @@ -123,6 +124,7 @@ class QgsConnectionPoolGroup qgsConnectionPool_ConnectionCreate( connInfo, i.c ); } + // no need to run if nothing can expire if ( conns.isEmpty() ) { @@ -283,9 +285,11 @@ class QgsConnectionPool * If \a timeout is a negative value the calling thread will be blocked * until a connection becomes available. This is the default behavior. * + * + * * \returns initialized connection or nullptr if unsuccessful */ - T acquireConnection( const QString &connInfo, int timeout = -1 ) + T acquireConnection( const QString &connInfo, int timeout = -1, bool requestMayBeNested = false ) { mMutex.lock(); typename T_Groups::iterator it = mGroups.find( connInfo ); @@ -296,7 +300,7 @@ class QgsConnectionPool T_Group *group = *it; mMutex.unlock(); - return group->acquire( timeout ); + return group->acquire( timeout, requestMayBeNested ); } //! Release an existing connection so it will get back into the pool and can be reused diff --git a/src/core/qgsfeaturerequest.cpp b/src/core/qgsfeaturerequest.cpp index 03ca00e1d89..f4244213843 100644 --- a/src/core/qgsfeaturerequest.cpp +++ b/src/core/qgsfeaturerequest.cpp @@ -86,6 +86,7 @@ QgsFeatureRequest &QgsFeatureRequest::operator=( const QgsFeatureRequest &rh ) mCrs = rh.mCrs; mTransformErrorCallback = rh.mTransformErrorCallback; mConnectionTimeout = rh.mConnectionTimeout; + mRequestMayBeNested = rh.mRequestMayBeNested; return *this; } @@ -298,6 +299,16 @@ void QgsFeatureRequest::setConnectionTimeout( int connectionTimeout ) mConnectionTimeout = connectionTimeout; } +bool QgsFeatureRequest::requestMayBeNested() const +{ + return mRequestMayBeNested; +} + +void QgsFeatureRequest::setRequestMayBeNested( bool requestMayBeNested ) +{ + mRequestMayBeNested = requestMayBeNested; +} + #include "qgsfeatureiterator.h" #include "qgslogger.h" diff --git a/src/core/qgsfeaturerequest.h b/src/core/qgsfeaturerequest.h index fdb823f93a1..b8ac9620a31 100644 --- a/src/core/qgsfeaturerequest.h +++ b/src/core/qgsfeaturerequest.h @@ -636,6 +636,34 @@ class CORE_EXPORT QgsFeatureRequest */ void setConnectionTimeout( int connectionTimeout ); + /** + * In case this request may be run nested within another already running + * iteration on the same connection, set this to true. + * + * If this flag is true, this request will be able to make use of "spare" + * connections to avoid deadlocks. + * + * For example, this should be set on requests that are issued from an + * expression function. + * + * \since QGIS 3.4 + */ + bool requestMayBeNested() const; + + /** + * In case this request may be run nested within another already running + * iteration on the same connection, set this to true. + * + * If this flag is true, this request will be able to make use of "spare" + * connections to avoid deadlocks. + * + * For example, this should be set on requests that are issued from an + * expression function. + * + * \since QGIS 3.4 + */ + void setRequestMayBeNested( bool requestMayBeNested ); + protected: FilterType mFilter = FilterNone; QgsRectangle mFilterRect; @@ -654,6 +682,7 @@ class CORE_EXPORT QgsFeatureRequest QgsCoordinateReferenceSystem mCrs; QgsCoordinateTransformContext mTransformContext; int mConnectionTimeout = -1; + int mRequestMayBeNested = false; }; Q_DECLARE_OPERATORS_FOR_FLAGS( QgsFeatureRequest::Flags ) diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp index e6c6e358463..453834e43f1 100644 --- a/src/providers/ogr/qgsogrfeatureiterator.cpp +++ b/src/providers/ogr/qgsogrfeatureiterator.cpp @@ -56,8 +56,8 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool else { //QgsDebugMsg( "Feature iterator of " + mSource->mLayerName + ": acquiring connection"); - mConn = QgsOgrConnPool::instance()->acquireConnection( QgsOgrProviderUtils::connectionPoolId( mSource->mDataSource ) ); - if ( !mConn->ds ) + mConn = QgsOgrConnPool::instance()->acquireConnection( QgsOgrProviderUtils::connectionPoolId( mSource->mDataSource ), request.connectionTimeout(), request.requestMayBeNested() ); + if ( !mConn || !mConn->ds ) { return; } diff --git a/src/providers/oracle/qgsoraclefeatureiterator.cpp b/src/providers/oracle/qgsoraclefeatureiterator.cpp index 3866b54b8de..114479b1039 100644 --- a/src/providers/oracle/qgsoraclefeatureiterator.cpp +++ b/src/providers/oracle/qgsoraclefeatureiterator.cpp @@ -29,7 +29,7 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource *source, bool ownSource, const QgsFeatureRequest &request ) : QgsAbstractFeatureIteratorFromSource( source, ownSource, request ) { - mConnection = QgsOracleConnPool::instance()->acquireConnection( QgsOracleConn::toPoolName( mSource->mUri ) ); + mConnection = QgsOracleConnPool::instance()->acquireConnection( QgsOracleConn::toPoolName( mSource->mUri ), request.connectionTimeout(), request.requestMayBeNested() ); if ( !mConnection ) { close(); diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp index da9e75d1026..e418f30a12d 100644 --- a/src/providers/postgres/qgspostgresfeatureiterator.cpp +++ b/src/providers/postgres/qgspostgresfeatureiterator.cpp @@ -38,7 +38,7 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource if ( !source->mTransactionConnection ) { - mConn = QgsPostgresConnPool::instance()->acquireConnection( mSource->mConnInfo ); + mConn = QgsPostgresConnPool::instance()->acquireConnection( mSource->mConnInfo, request.connectionTimeout(), request.requestMayBeNested() ); mIsTransactionConnection = false; } else diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp index 106c299a1e4..04438ca65f3 100644 --- a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp +++ b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp @@ -29,7 +29,7 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeatureSource *source, bool ownSource, const QgsFeatureRequest &request ) : QgsAbstractFeatureIteratorFromSource( source, ownSource, request ) { - mHandle = QgsSpatiaLiteConnPool::instance()->acquireConnection( mSource->mSqlitePath ); + mHandle = QgsSpatiaLiteConnPool::instance()->acquireConnection( mSource->mSqlitePath, request.connectionTimeout(), request.requestMayBeNested() ); mFetchGeometry = !mSource->mGeometryColumn.isNull() && !( mRequest.flags() & QgsFeatureRequest::NoGeometry ); mHasPrimaryKey = !mSource->mPrimaryKey.isEmpty(); diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py index f455986049c..fe11734a415 100644 --- a/tests/src/python/providertestbase.py +++ b/tests/src/python/providertestbase.py @@ -17,6 +17,7 @@ __copyright__ = 'Copyright 2015, The QGIS Project' __revision__ = '$Format:%H$' from qgis.core import ( + QgsApplication, QgsRectangle, QgsFeatureRequest, QgsFeature, @@ -24,6 +25,7 @@ from qgis.core import ( QgsAbstractFeatureIterator, QgsExpressionContextScope, QgsExpressionContext, + QgsExpression, QgsVectorDataProvider, QgsVectorLayerFeatureSource, QgsFeatureSink, @@ -886,3 +888,25 @@ class ProviderTestCase(FeatureSourceTestCase): self.assertEqual(count, 5) self.assertFalse(iterator.compileFailed()) self.disableCompiler() + + def testConcurrency(self): + """ + The connection pool has a maximum of 4 connections defined (+2 spare connections) + Make sure that if we exhaust those 4 connections and force another connection + it is actually using the spare connections and does not freeze. + This situation normally happens when (at least) 4 rendering threads are active + in parallel and one requires an expression to be evaluated. + """ + # Acquire the maximum amount of concurrent connections + iterators = list() + for i in range(QgsApplication.instance().maxConcurrentConnectionsPerPool()): + iterators.append(self.vl.getFeatures()) + + # Run an expression that will also do a request and should use a spare + # connection. It just should not deadlock here. + + feat = next(iterators[0]) + context = QgsExpressionContext() + context.setFeature(feat) + exp = QgsExpression('get_feature(\'{layer}\', \'pk\', 5)'.format(layer=self.vl.id())) + exp.evaluate(context)