From c9e761649820f8444a41da5e18850061b207c09c Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Fri, 1 Feb 2019 13:37:33 +1000 Subject: [PATCH] Fix hang when WMS credentials requested Remove responsibility for credentials mutex locking from external callers and handle appropriate locks internally. This allows the mutex lock to be much "tighter" and avoids deadlocks when credentials are requested while an existing credentials dialog is being shown. (No mutex is required protecting the credentials dialog itself as this is ALWAYS shown in the main thread) Fixes #20826 --- .../core/auto_generated/qgscredentials.sip.in | 12 +++---- src/app/qgisapp.cpp | 7 ---- src/app/qgsappauthrequesthandler.cpp | 33 +++++++------------ src/core/auth/qgsauthmanager.cpp | 2 -- src/core/qgscredentials.cpp | 4 +++ src/core/qgscredentials.h | 16 +++++---- src/providers/db2/qgsdb2provider.cpp | 3 -- src/providers/oracle/qgsoracleconn.cpp | 4 --- src/providers/postgres/qgspostgresconn.cpp | 4 --- 9 files changed, 31 insertions(+), 54 deletions(-) diff --git a/python/core/auto_generated/qgscredentials.sip.in b/python/core/auto_generated/qgscredentials.sip.in index 12454ac9a2f..fc78456a2e8 100644 --- a/python/core/auto_generated/qgscredentials.sip.in +++ b/python/core/auto_generated/qgscredentials.sip.in @@ -41,27 +41,27 @@ signal destroyed() to be notified of the deletion retrieves instance %End - void lock(); + void lock() /Deprecated/; %Docstring Lock the instance against access from multiple threads. This does not really lock access to get/put methds, it will just prevent other threads to lock the instance and continue the execution. When the class is used from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions. -.. versionadded:: 2.4 +.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled %End - void unlock(); + void unlock() /Deprecated/; %Docstring Unlock the instance after being locked. -.. versionadded:: 2.4 +.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled %End - QMutex *mutex(); + QMutex *mutex() /Deprecated/; %Docstring Returns pointer to mutex -.. versionadded:: 2.4 +.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled %End protected: diff --git a/src/app/qgisapp.cpp b/src/app/qgisapp.cpp index 58920f98b7a..576e5303bae 100644 --- a/src/app/qgisapp.cpp +++ b/src/app/qgisapp.cpp @@ -1589,10 +1589,6 @@ QgisApp::~QgisApp() qDeleteAll( mCustomDropHandlers ); qDeleteAll( mCustomLayoutDropHandlers ); - // replace gui based network access managers with failing only ones - QgsNetworkAccessManager::instance()->setSslErrorHandler( qgis::make_unique< QgsSslErrorHandler >() ); - QgsNetworkAccessManager::instance()->setAuthHandler( qgis::make_unique< QgsNetworkAuthenticationHandler >() ); - const QList canvases = mapCanvases(); for ( QgsMapCanvas *canvas : canvases ) { @@ -13978,7 +13974,6 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe bool ok; { - QMutexLocker lock( QgsCredentials::instance()->mutex() ); ok = QgsCredentials::instance()->get( QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ), username, password, @@ -13992,7 +13987,6 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe // credentials didn't change - stored ones probably wrong? clear password and retry { - QMutexLocker lock( QgsCredentials::instance()->mutex() ); QgsCredentials::instance()->put( QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ), username, QString() ); @@ -14000,7 +13994,6 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe } { - QMutexLocker lock( QgsCredentials::instance()->mutex() ); QgsCredentials::instance()->put( QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ), username, password diff --git a/src/app/qgsappauthrequesthandler.cpp b/src/app/qgsappauthrequesthandler.cpp index 33baefc7df7..61387d577b6 100644 --- a/src/app/qgsappauthrequesthandler.cpp +++ b/src/app/qgsappauthrequesthandler.cpp @@ -44,15 +44,10 @@ void QgsAppAuthRequestHandler::handleAuthRequest( QNetworkReply *reply, QAuthent for ( ;; ) { - bool ok; - - { - QMutexLocker lock( QgsCredentials::instance()->mutex() ); - ok = QgsCredentials::instance()->get( - QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ), - username, password, - QObject::tr( "Authentication required" ) ); - } + bool ok = QgsCredentials::instance()->get( + QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ), + username, password, + QObject::tr( "Authentication required" ) ); if ( !ok ) return; @@ -60,22 +55,16 @@ void QgsAppAuthRequestHandler::handleAuthRequest( QNetworkReply *reply, QAuthent break; // credentials didn't change - stored ones probably wrong? clear password and retry - { - QMutexLocker lock( QgsCredentials::instance()->mutex() ); - QgsCredentials::instance()->put( - QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ), - username, QString() ); - } + QgsCredentials::instance()->put( + QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ), + username, QString() ); } // save credentials - { - QMutexLocker lock( QgsCredentials::instance()->mutex() ); - QgsCredentials::instance()->put( - QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ), - username, password - ); - } + QgsCredentials::instance()->put( + QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ), + username, password + ); auth->setUser( username ); auth->setPassword( password ); diff --git a/src/core/auth/qgsauthmanager.cpp b/src/core/auth/qgsauthmanager.cpp index c9fdac7b4fd..cdcb281d172 100644 --- a/src/core/auth/qgsauthmanager.cpp +++ b/src/core/auth/qgsauthmanager.cpp @@ -3193,10 +3193,8 @@ bool QgsAuthManager::masterPasswordInput() if ( ! ok ) { QgsCredentials *creds = QgsCredentials::instance(); - creds->lock(); pass.clear(); ok = creds->getMasterPassword( pass, masterPasswordHashInDatabase() ); - creds->unlock(); } if ( ok && !pass.isEmpty() && mMasterPass != pass ) diff --git a/src/core/qgscredentials.cpp b/src/core/qgscredentials.cpp index b9dd5f6d94f..6367b1fbdd4 100644 --- a/src/core/qgscredentials.cpp +++ b/src/core/qgscredentials.cpp @@ -40,9 +40,11 @@ QgsCredentials *QgsCredentials::instance() bool QgsCredentials::get( const QString &realm, QString &username, QString &password, const QString &message ) { + QMutexLocker locker( &mMutex ); if ( mCredentialCache.contains( realm ) ) { QPair credentials = mCredentialCache.take( realm ); + locker.unlock(); username = credentials.first; password = credentials.second; QgsDebugMsg( QStringLiteral( "retrieved realm:%1 username:%2 password:%3" ).arg( realm, username, password ) ); @@ -50,6 +52,7 @@ bool QgsCredentials::get( const QString &realm, QString &username, QString &pass if ( !password.isNull() ) return true; } + locker.unlock(); if ( request( realm, username, password, message ) ) { @@ -66,6 +69,7 @@ bool QgsCredentials::get( const QString &realm, QString &username, QString &pass void QgsCredentials::put( const QString &realm, const QString &username, const QString &password ) { QgsDebugMsg( QStringLiteral( "inserting realm:%1 username:%2 password:%3" ).arg( realm, username, password ) ); + QMutexLocker locker( &mMutex ); mCredentialCache.insert( realm, QPair( username, password ) ); } diff --git a/src/core/qgscredentials.h b/src/core/qgscredentials.h index dbbad2d451c..fffa8f2e093 100644 --- a/src/core/qgscredentials.h +++ b/src/core/qgscredentials.h @@ -59,21 +59,25 @@ class CORE_EXPORT QgsCredentials * Lock the instance against access from multiple threads. This does not really lock access to get/put methds, * it will just prevent other threads to lock the instance and continue the execution. When the class is used * from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions. - * \since QGIS 2.4 + * + * \deprecated since QGIS 3.4 - mutex locking is automatically handled */ - void lock(); + Q_DECL_DEPRECATED void lock() SIP_DEPRECATED; /** * Unlock the instance after being locked. - * \since QGIS 2.4 + * \deprecated since QGIS 3.4 - mutex locking is automatically handled */ - void unlock(); + Q_DECL_DEPRECATED void unlock() SIP_DEPRECATED; /** * Returns pointer to mutex - * \since QGIS 2.4 + * \deprecated since QGIS 3.4 - mutex locking is automatically handled */ - QMutex *mutex() { return &mMutex; } + Q_DECL_DEPRECATED QMutex *mutex() SIP_DEPRECATED + { + return &mMutex; + } protected: diff --git a/src/providers/db2/qgsdb2provider.cpp b/src/providers/db2/qgsdb2provider.cpp index c42c0cba164..3e28512f11b 100644 --- a/src/providers/db2/qgsdb2provider.cpp +++ b/src/providers/db2/qgsdb2provider.cpp @@ -235,7 +235,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM db.setPort( port.toInt() ); bool connected = false; int i = 0; - QgsCredentials::instance()->lock(); while ( !connected && i < 3 ) { i++; @@ -249,7 +248,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM { errMsg = QStringLiteral( "Cancel clicked" ); QgsDebugMsg( errMsg ); - QgsCredentials::instance()->unlock(); break; } } @@ -291,7 +289,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM { QgsCredentials::instance()->put( databaseName, userName, password ); } - QgsCredentials::instance()->unlock(); return db; } diff --git a/src/providers/oracle/qgsoracleconn.cpp b/src/providers/oracle/qgsoracleconn.cpp index e9e39ccc66a..25af0f0ecf7 100644 --- a/src/providers/oracle/qgsoracleconn.cpp +++ b/src/providers/oracle/qgsoracleconn.cpp @@ -96,8 +96,6 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri ) QgsDebugMsg( QStringLiteral( "Connecting with options: " ) + options ); if ( !mDatabase.open() ) { - QgsCredentials::instance()->lock(); - while ( !mDatabase.open() ) { bool ok = QgsCredentials::instance()->get( realm, username, password, mDatabase.lastError().text() ); @@ -127,8 +125,6 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri ) if ( mDatabase.isOpen() ) QgsCredentials::instance()->put( realm, username, password ); - - QgsCredentials::instance()->unlock(); } if ( !mDatabase.isOpen() ) diff --git a/src/providers/postgres/qgspostgresconn.cpp b/src/providers/postgres/qgspostgresconn.cpp index c0aafbc685f..0578ffc8cc1 100644 --- a/src/providers/postgres/qgspostgresconn.cpp +++ b/src/providers/postgres/qgspostgresconn.cpp @@ -272,8 +272,6 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s QString username = uri.username(); QString password = uri.password(); - QgsCredentials::instance()->lock(); - int i = 0; while ( PQstatus() != CONNECTION_OK && i < 5 ) { @@ -298,8 +296,6 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s if ( PQstatus() == CONNECTION_OK ) QgsCredentials::instance()->put( conninfo, username, password ); - - QgsCredentials::instance()->unlock(); } if ( PQstatus() != CONNECTION_OK )