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
qgis · Feb 3, 2019 · c9e7616 · c9e7616
1 parent 59aea69
commit c9e7616
Show file tree

Hide file tree

Showing 9 changed files with 31 additions and 54 deletions.
diff --git 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
@@ -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<QgsMapCanvas *> 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,15 +13987,13 @@ 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() );
     }
   }
 
   {
-    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
@@ -44,39 +44,28 @@ 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;
 
     if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
       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() );
-    }
-  }
-
-  // save credentials
-  {
-    QMutexLocker lock( QgsCredentials::instance()->mutex() );
     QgsCredentials::instance()->put(
       QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
-      username, password
-    );
+      username, QString() );
   }
 
+  // save credentials
+  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
@@ -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
@@ -40,16 +40,19 @@ QgsCredentials *QgsCredentials::instance()
 
 bool QgsCredentials::get( const QString &realm, QString &username, QString &password, const QString &message )
 {
+  QMutexLocker locker( &mMutex );
   if ( mCredentialCache.contains( realm ) )
   {
     QPair<QString, QString> 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 ) );
 
     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<QString, QString>( username, password ) );
 }
 

diff --git 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
@@ -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
@@ -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
@@ -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 )