Skip to content

Commit a7f0f2b

Browse files
m-kuhnm-kuhn
authoredAug 5, 2018
Merge pull request #7519 from m-kuhn/nestedConnectionPoolDeadlock
Fix freeze with `get_feature`
·
ltr-3_40final-3_4_0
2 parents 87d7583 + f301a89 commit a7f0f2b

13 files changed

+142
-14
lines changed
 

‎python/core/auto_generated/qgsapplication.sip.in

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -797,6 +797,18 @@ Do not include generated variables (like system name, user name etc.)
797797
Set a single custom expression variable.
798798

799799
.. versionadded:: 3.0
800+
%End
801+
802+
int maxConcurrentConnectionsPerPool() const;
803+
%Docstring
804+
The maximum number of concurrent connections per connections pool.
805+
806+
.. note::
807+
808+
QGIS may in some situations allocate more than this amount
809+
of connections to avoid deadlocks.
810+
811+
.. versionadded:: 3.4
800812
%End
801813

802814
%If (ANDROID)

‎python/core/auto_generated/qgsfeaturerequest.sip.in

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,34 @@ at this moment. A negative value (which is set by default) will wait forever.
657657
Only works if the provider supports this option.
658658

659659
.. versionadded:: 3.0
660+
%End
661+
662+
bool requestMayBeNested() const;
663+
%Docstring
664+
In case this request may be run nested within another already running
665+
iteration on the same connection, set this to true.
666+
667+
If this flag is true, this request will be able to make use of "spare"
668+
connections to avoid deadlocks.
669+
670+
For example, this should be set on requests that are issued from an
671+
expression function.
672+
673+
.. versionadded:: 3.4
674+
%End
675+
676+
void setRequestMayBeNested( bool requestMayBeNested );
677+
%Docstring
678+
In case this request may be run nested within another already running
679+
iteration on the same connection, set this to true.
680+
681+
If this flag is true, this request will be able to make use of "spare"
682+
connections to avoid deadlocks.
683+
684+
For example, this should be set on requests that are issued from an
685+
expression function.
686+
687+
.. versionadded:: 3.4
660688
%End
661689

662690
protected:

‎src/core/expression/qgsexpressionfunction.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3570,6 +3570,8 @@ static QVariant fcnGetFeatureById( const QVariantList &values, const QgsExpressi
35703570

35713571
QgsFeatureRequest req;
35723572
req.setFilterFid( fid );
3573+
req.setConnectionTimeout( 10000 );
3574+
req.setRequestMayBeNested( true );
35733575
QgsFeatureIterator fIt = vl->getFeatures( req );
35743576

35753577
QgsFeature fet;
@@ -3611,6 +3613,8 @@ static QVariant fcnGetFeature( const QVariantList &values, const QgsExpressionCo
36113613
req.setFilterExpression( QStringLiteral( "%1=%2" ).arg( QgsExpression::quotedColumnRef( attribute ),
36123614
QgsExpression::quotedString( attVal.toString() ) ) );
36133615
req.setLimit( 1 );
3616+
req.setConnectionTimeout( 10000 );
3617+
req.setRequestMayBeNested( true );
36143618
if ( !parent->needsGeometry() )
36153619
{
36163620
req.setFlags( QgsFeatureRequest::NoGeometry );

‎src/core/qgsapplication.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@
8585
#include <cpl_conv.h> // for setting gdal options
8686
#include <sqlite3.h>
8787

88+
#define CONN_POOL_MAX_CONCURRENT_CONNS 4
89+
8890
QObject *ABISYM( QgsApplication::mFileOpenEventReceiver );
8991
QStringList ABISYM( QgsApplication::mFileOpenEventList );
9092
QString ABISYM( QgsApplication::mPrefixPath );
@@ -1461,6 +1463,10 @@ void QgsApplication::setCustomVariable( const QString &name, const QVariant &val
14611463
emit instance()->customVariablesChanged();
14621464
}
14631465

1466+
int QgsApplication::maxConcurrentConnectionsPerPool() const
1467+
{
1468+
return CONN_POOL_MAX_CONCURRENT_CONNS;
1469+
}
14641470

14651471
QString QgsApplication::nullRepresentation()
14661472
{

‎src/core/qgsapplication.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -732,6 +732,16 @@ class CORE_EXPORT QgsApplication : public QApplication
732732
*/
733733
static void setCustomVariable( const QString &name, const QVariant &value );
734734

735+
/**
736+
* The maximum number of concurrent connections per connections pool.
737+
*
738+
* \note QGIS may in some situations allocate more than this amount
739+
* of connections to avoid deadlocks.
740+
*
741+
* \since QGIS 3.4
742+
*/
743+
int maxConcurrentConnectionsPerPool() const;
744+
735745
#ifdef SIP_RUN
736746
SIP_IF_FEATURE( ANDROID )
737747
//dummy method to workaround sip generation issue

‎src/core/qgsconnectionpool.h

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#define SIP_NO_FILE
2020

2121
#include "qgis.h"
22+
#include "qgsapplication.h"
2223
#include <QCoreApplication>
2324
#include <QMap>
2425
#include <QMutex>
@@ -29,8 +30,8 @@
2930
#include <QThread>
3031

3132

32-
#define CONN_POOL_MAX_CONCURRENT_CONNS 4
3333
#define CONN_POOL_EXPIRATION_TIME 60 // in seconds
34+
#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
3435

3536

3637
/**
@@ -59,8 +60,6 @@ class QgsConnectionPoolGroup
5960
{
6061
public:
6162

62-
static const int MAX_CONCURRENT_CONNECTIONS;
63-
6463
struct Item
6564
{
6665
T c;
@@ -69,7 +68,7 @@ class QgsConnectionPoolGroup
6968

7069
QgsConnectionPoolGroup( const QString &ci )
7170
: connInfo( ci )
72-
, sem( CONN_POOL_MAX_CONCURRENT_CONNS )
71+
, sem( QgsApplication::instance()->maxConcurrentConnectionsPerPool() + CONN_POOL_SPARE_CONNECTIONS )
7372
{
7473
}
7574

@@ -93,12 +92,13 @@ class QgsConnectionPoolGroup
9392
*
9493
* \returns initialized connection or nullptr if unsuccessful
9594
*/
96-
T acquire( int timeout )
95+
T acquire( int timeout, bool requestMayBeNested )
9796
{
97+
const int requiredFreeConnectionCount = requestMayBeNested ? 1 : 3;
9898
// we are going to acquire a resource - if no resource is available, we will block here
9999
if ( timeout >= 0 )
100100
{
101-
if ( !sem.tryAcquire( 1, timeout ) )
101+
if ( !sem.tryAcquire( requiredFreeConnectionCount, timeout ) )
102102
return nullptr;
103103
}
104104
else
@@ -107,8 +107,9 @@ class QgsConnectionPoolGroup
107107
// tryAcquire is broken on Qt > 5.8 with negative timeouts - see
108108
// https://bugreports.qt.io/browse/QTBUG-64413
109109
// https://lists.osgeo.org/pipermail/qgis-developer/2017-November/050456.html
110-
sem.acquire( 1 );
110+
sem.acquire( requiredFreeConnectionCount );
111111
}
112+
sem.release( requiredFreeConnectionCount - 1 );
112113

113114
// quick (preferred) way - use cached connection
114115
{
@@ -123,6 +124,7 @@ class QgsConnectionPoolGroup
123124
qgsConnectionPool_ConnectionCreate( connInfo, i.c );
124125
}
125126

127+
126128
// no need to run if nothing can expire
127129
if ( conns.isEmpty() )
128130
{
@@ -283,9 +285,11 @@ class QgsConnectionPool
283285
* If \a timeout is a negative value the calling thread will be blocked
284286
* until a connection becomes available. This is the default behavior.
285287
*
288+
*
289+
*
286290
* \returns initialized connection or nullptr if unsuccessful
287291
*/
288-
T acquireConnection( const QString &connInfo, int timeout = -1 )
292+
T acquireConnection( const QString &connInfo, int timeout = -1, bool requestMayBeNested = false )
289293
{
290294
mMutex.lock();
291295
typename T_Groups::iterator it = mGroups.find( connInfo );
@@ -296,7 +300,7 @@ class QgsConnectionPool
296300
T_Group *group = *it;
297301
mMutex.unlock();
298302

299-
return group->acquire( timeout );
303+
return group->acquire( timeout, requestMayBeNested );
300304
}
301305

302306
//! Release an existing connection so it will get back into the pool and can be reused

‎src/core/qgsfeaturerequest.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ QgsFeatureRequest &QgsFeatureRequest::operator=( const QgsFeatureRequest &rh )
8686
mCrs = rh.mCrs;
8787
mTransformErrorCallback = rh.mTransformErrorCallback;
8888
mConnectionTimeout = rh.mConnectionTimeout;
89+
mRequestMayBeNested = rh.mRequestMayBeNested;
8990
return *this;
9091
}
9192

@@ -298,6 +299,16 @@ void QgsFeatureRequest::setConnectionTimeout( int connectionTimeout )
298299
mConnectionTimeout = connectionTimeout;
299300
}
300301

302+
bool QgsFeatureRequest::requestMayBeNested() const
303+
{
304+
return mRequestMayBeNested;
305+
}
306+
307+
void QgsFeatureRequest::setRequestMayBeNested( bool requestMayBeNested )
308+
{
309+
mRequestMayBeNested = requestMayBeNested;
310+
}
311+
301312

302313
#include "qgsfeatureiterator.h"
303314
#include "qgslogger.h"

‎src/core/qgsfeaturerequest.h

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,34 @@ class CORE_EXPORT QgsFeatureRequest
636636
*/
637637
void setConnectionTimeout( int connectionTimeout );
638638

639+
/**
640+
* In case this request may be run nested within another already running
641+
* iteration on the same connection, set this to true.
642+
*
643+
* If this flag is true, this request will be able to make use of "spare"
644+
* connections to avoid deadlocks.
645+
*
646+
* For example, this should be set on requests that are issued from an
647+
* expression function.
648+
*
649+
* \since QGIS 3.4
650+
*/
651+
bool requestMayBeNested() const;
652+
653+
/**
654+
* In case this request may be run nested within another already running
655+
* iteration on the same connection, set this to true.
656+
*
657+
* If this flag is true, this request will be able to make use of "spare"
658+
* connections to avoid deadlocks.
659+
*
660+
* For example, this should be set on requests that are issued from an
661+
* expression function.
662+
*
663+
* \since QGIS 3.4
664+
*/
665+
void setRequestMayBeNested( bool requestMayBeNested );
666+
639667
protected:
640668
FilterType mFilter = FilterNone;
641669
QgsRectangle mFilterRect;
@@ -654,6 +682,7 @@ class CORE_EXPORT QgsFeatureRequest
654682
QgsCoordinateReferenceSystem mCrs;
655683
QgsCoordinateTransformContext mTransformContext;
656684
int mConnectionTimeout = -1;
685+
int mRequestMayBeNested = false;
657686
};
658687

659688
Q_DECLARE_OPERATORS_FOR_FLAGS( QgsFeatureRequest::Flags )

‎src/providers/ogr/qgsogrfeatureiterator.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
5656
else
5757
{
5858
//QgsDebugMsg( "Feature iterator of " + mSource->mLayerName + ": acquiring connection");
59-
mConn = QgsOgrConnPool::instance()->acquireConnection( QgsOgrProviderUtils::connectionPoolId( mSource->mDataSource ) );
60-
if ( !mConn->ds )
59+
mConn = QgsOgrConnPool::instance()->acquireConnection( QgsOgrProviderUtils::connectionPoolId( mSource->mDataSource ), request.connectionTimeout(), request.requestMayBeNested() );
60+
if ( !mConn || !mConn->ds )
6161
{
6262
return;
6363
}

‎src/providers/oracle/qgsoraclefeatureiterator.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
3030
: QgsAbstractFeatureIteratorFromSource<QgsOracleFeatureSource>( source, ownSource, request )
3131
{
32-
mConnection = QgsOracleConnPool::instance()->acquireConnection( QgsOracleConn::toPoolName( mSource->mUri ) );
32+
mConnection = QgsOracleConnPool::instance()->acquireConnection( QgsOracleConn::toPoolName( mSource->mUri ), request.connectionTimeout(), request.requestMayBeNested() );
3333
if ( !mConnection )
3434
{
3535
close();

‎src/providers/postgres/qgspostgresfeatureiterator.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
3838

3939
if ( !source->mTransactionConnection )
4040
{
41-
mConn = QgsPostgresConnPool::instance()->acquireConnection( mSource->mConnInfo );
41+
mConn = QgsPostgresConnPool::instance()->acquireConnection( mSource->mConnInfo, request.connectionTimeout(), request.requestMayBeNested() );
4242
mIsTransactionConnection = false;
4343
}
4444
else

‎src/providers/spatialite/qgsspatialitefeatureiterator.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
3030
: QgsAbstractFeatureIteratorFromSource<QgsSpatiaLiteFeatureSource>( source, ownSource, request )
3131
{
32-
mHandle = QgsSpatiaLiteConnPool::instance()->acquireConnection( mSource->mSqlitePath );
32+
mHandle = QgsSpatiaLiteConnPool::instance()->acquireConnection( mSource->mSqlitePath, request.connectionTimeout(), request.requestMayBeNested() );
3333

3434
mFetchGeometry = !mSource->mGeometryColumn.isNull() && !( mRequest.flags() & QgsFeatureRequest::NoGeometry );
3535
mHasPrimaryKey = !mSource->mPrimaryKey.isEmpty();

‎tests/src/python/providertestbase.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
__revision__ = '$Format:%H$'
1818

1919
from qgis.core import (
20+
QgsApplication,
2021
QgsRectangle,
2122
QgsFeatureRequest,
2223
QgsFeature,
2324
QgsGeometry,
2425
QgsAbstractFeatureIterator,
2526
QgsExpressionContextScope,
2627
QgsExpressionContext,
28+
QgsExpression,
2729
QgsVectorDataProvider,
2830
QgsVectorLayerFeatureSource,
2931
QgsFeatureSink,
@@ -886,3 +888,25 @@ def testStringComparison(self):
886888
self.assertEqual(count, 5)
887889
self.assertFalse(iterator.compileFailed())
888890
self.disableCompiler()
891+
892+
def testConcurrency(self):
893+
"""
894+
The connection pool has a maximum of 4 connections defined (+2 spare connections)
895+
Make sure that if we exhaust those 4 connections and force another connection
896+
it is actually using the spare connections and does not freeze.
897+
This situation normally happens when (at least) 4 rendering threads are active
898+
in parallel and one requires an expression to be evaluated.
899+
"""
900+
# Acquire the maximum amount of concurrent connections
901+
iterators = list()
902+
for i in range(QgsApplication.instance().maxConcurrentConnectionsPerPool()):
903+
iterators.append(self.vl.getFeatures())
904+
905+
# Run an expression that will also do a request and should use a spare
906+
# connection. It just should not deadlock here.
907+
908+
feat = next(iterators[0])
909+
context = QgsExpressionContext()
910+
context.setFeature(feat)
911+
exp = QgsExpression('get_feature(\'{layer}\', \'pk\', 5)'.format(layer=self.vl.id()))
912+
exp.evaluate(context)

0 commit comments

Comments
 (0)
Please sign in to comment.