Merge pull request #7519 from m-kuhn/nestedConnectionPoolDeadlock

Fix freeze with `get_feature`

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)

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:

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 );

src/core/qgsapplication.cpp

@@ -85,6 +85,8 @@
 #include <cpl_conv.h> // for setting gdal options
 #include <sqlite3.h>
 
+#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()
 {

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

src/core/qgsconnectionpool.h

@@ -19,6 +19,7 @@
 #define SIP_NO_FILE
 
 #include "qgis.h"
+#include "qgsapplication.h"
 #include <QCoreApplication>
 #include <QMap>
 #include <QMutex>
@@ -29,8 +30,8 @@
 #include <QThread>
 
 
-#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

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"

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 )

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 ) );
+    mConn = QgsOgrConnPool::instance()->acquireConnection( QgsOgrProviderUtils::connectionPoolId( mSource->mDataSource ), request.connectionTimeout(), request.requestMayBeNested() );
-    if ( !mConn->ds )
+    if ( !mConn || !mConn->ds )
     {
       return;
     }

src/providers/oracle/qgsoraclefeatureiterator.cpp

@@ -29,7 +29,7 @@
 QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
   : QgsAbstractFeatureIteratorFromSource<QgsOracleFeatureSource>( 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();

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

src/providers/spatialite/qgsspatialitefeatureiterator.cpp

@@ -29,7 +29,7 @@
 QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
   : QgsAbstractFeatureIteratorFromSource<QgsSpatiaLiteFeatureSource>( 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();

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)