Skip to content
Permalink
Browse files

Merge pull request #7519 from m-kuhn/nestedConnectionPoolDeadlock

Fix freeze with `get_feature`
  • Loading branch information
m-kuhn committed Aug 5, 2018
2 parents 87d7583 + f301a89 commit a7f0f2b34e937f26cf383343572824bd4023882c
@@ -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)
@@ -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:
@@ -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 );
@@ -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()
{
@@ -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
@@ -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
@@ -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"
@@ -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 )
@@ -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;
}
@@ -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();
@@ -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
@@ -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();
@@ -17,13 +17,15 @@
__revision__ = '$Format:%H$'

from qgis.core import (
QgsApplication,
QgsRectangle,
QgsFeatureRequest,
QgsFeature,
QgsGeometry,
QgsAbstractFeatureIterator,
QgsExpressionContextScope,
QgsExpressionContext,
QgsExpression,
QgsVectorDataProvider,
QgsVectorLayerFeatureSource,
QgsFeatureSink,
@@ -886,3 +888,25 @@ def testStringComparison(self):
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)

0 comments on commit a7f0f2b

Please sign in to comment.
You can’t perform that action at this time.