Skip to content

Commit ad6e156

Browse files
nyalldawsonnyalldawson
committedFeb 3, 2019
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 (cherry picked from commit c9e7616)
·
final-3_4_15final-3_4_5
1 parent 0e63582 commit ad6e156

File tree

8 files changed

+21
-40
lines changed

8 files changed

+21
-40
lines changed
 

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,27 +41,27 @@ signal destroyed() to be notified of the deletion
4141
retrieves instance
4242
%End
4343

44-
void lock();
44+
void lock() /Deprecated/;
4545
%Docstring
4646
Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
4747
it will just prevent other threads to lock the instance and continue the execution. When the class is used
4848
from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.
4949

50-
.. versionadded:: 2.4
50+
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
5151
%End
5252

53-
void unlock();
53+
void unlock() /Deprecated/;
5454
%Docstring
5555
Unlock the instance after being locked.
5656

57-
.. versionadded:: 2.4
57+
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
5858
%End
5959

60-
QMutex *mutex();
60+
QMutex *mutex() /Deprecated/;
6161
%Docstring
6262
Returns pointer to mutex
6363

64-
.. versionadded:: 2.4
64+
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
6565
%End
6666

6767
protected:

‎src/app/qgisapp.cpp

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13657,15 +13657,10 @@ void QgisApp::namAuthenticationRequired( QNetworkReply *inReply, QAuthenticator
1365713657

1365813658
for ( ;; )
1365913659
{
13660-
bool ok;
13661-
13662-
{
13663-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
13664-
ok = QgsCredentials::instance()->get(
13660+
bool ok = QgsCredentials::instance()->get(
1366513661
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
1366613662
username, password,
1366713663
tr( "Authentication required" ) );
13668-
}
1366913664
if ( !ok )
1367013665
return;
1367113666

@@ -13676,22 +13671,16 @@ void QgisApp::namAuthenticationRequired( QNetworkReply *inReply, QAuthenticator
1367613671
break;
1367713672

1367813673
// credentials didn't change - stored ones probably wrong? clear password and retry
13679-
{
13680-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1368113674
QgsCredentials::instance()->put(
1368213675
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
1368313676
username, QString() );
13684-
}
1368513677
}
1368613678

1368713679
// save credentials
13688-
{
13689-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1369013680
QgsCredentials::instance()->put(
1369113681
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
1369213682
username, password
1369313683
);
13694-
}
1369513684

1369613685
auth->setUser( username );
1369713686
auth->setPassword( password );
@@ -13715,7 +13704,6 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
1371513704
bool ok;
1371613705

1371713706
{
13718-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1371913707
ok = QgsCredentials::instance()->get(
1372013708
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
1372113709
username, password,
@@ -13729,15 +13717,13 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
1372913717

1373013718
// credentials didn't change - stored ones probably wrong? clear password and retry
1373113719
{
13732-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1373313720
QgsCredentials::instance()->put(
1373413721
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
1373513722
username, QString() );
1373613723
}
1373713724
}
1373813725

1373913726
{
13740-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1374113727
QgsCredentials::instance()->put(
1374213728
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
1374313729
username, password

‎src/core/auth/qgsauthmanager.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3195,10 +3195,8 @@ bool QgsAuthManager::masterPasswordInput()
31953195
if ( ! ok )
31963196
{
31973197
QgsCredentials *creds = QgsCredentials::instance();
3198-
creds->lock();
31993198
pass.clear();
32003199
ok = creds->getMasterPassword( pass, masterPasswordHashInDatabase() );
3201-
creds->unlock();
32023200
}
32033201

32043202
if ( ok && !pass.isEmpty() && mMasterPass != pass )

‎src/core/qgscredentials.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,16 +40,19 @@ QgsCredentials *QgsCredentials::instance()
4040

4141
bool QgsCredentials::get( const QString &realm, QString &username, QString &password, const QString &message )
4242
{
43+
QMutexLocker locker( &mMutex );
4344
if ( mCredentialCache.contains( realm ) )
4445
{
4546
QPair<QString, QString> credentials = mCredentialCache.take( realm );
47+
locker.unlock();
4648
username = credentials.first;
4749
password = credentials.second;
4850
QgsDebugMsg( QStringLiteral( "retrieved realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
4951

5052
if ( !password.isNull() )
5153
return true;
5254
}
55+
locker.unlock();
5356

5457
if ( request( realm, username, password, message ) )
5558
{
@@ -66,6 +69,7 @@ bool QgsCredentials::get( const QString &realm, QString &username, QString &pass
6669
void QgsCredentials::put( const QString &realm, const QString &username, const QString &password )
6770
{
6871
QgsDebugMsg( QStringLiteral( "inserting realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
72+
QMutexLocker locker( &mMutex );
6973
mCredentialCache.insert( realm, QPair<QString, QString>( username, password ) );
7074
}
7175

‎src/core/qgscredentials.h

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,21 +59,25 @@ class CORE_EXPORT QgsCredentials
5959
* Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
6060
* it will just prevent other threads to lock the instance and continue the execution. When the class is used
6161
* from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.
62-
* \since QGIS 2.4
62+
*
63+
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
6364
*/
64-
void lock();
65+
Q_DECL_DEPRECATED void lock() SIP_DEPRECATED;
6566

6667
/**
6768
* Unlock the instance after being locked.
68-
* \since QGIS 2.4
69+
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
6970
*/
70-
void unlock();
71+
Q_DECL_DEPRECATED void unlock() SIP_DEPRECATED;
7172

7273
/**
7374
* Returns pointer to mutex
74-
* \since QGIS 2.4
75+
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
7576
*/
76-
QMutex *mutex() { return &mMutex; }
77+
Q_DECL_DEPRECATED QMutex *mutex() SIP_DEPRECATED
78+
{
79+
return &mMutex;
80+
}
7781

7882
protected:
7983

‎src/providers/db2/qgsdb2provider.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
201201
db.setPort( port.toInt() );
202202
bool connected = false;
203203
int i = 0;
204-
QgsCredentials::instance()->lock();
205204
while ( !connected && i < 3 )
206205
{
207206
i++;
@@ -215,7 +214,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
215214
{
216215
errMsg = QStringLiteral( "Cancel clicked" );
217216
QgsDebugMsg( errMsg );
218-
QgsCredentials::instance()->unlock();
219217
break;
220218
}
221219
}
@@ -257,7 +255,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
257255
{
258256
QgsCredentials::instance()->put( databaseName, userName, password );
259257
}
260-
QgsCredentials::instance()->unlock();
261258

262259
return db;
263260
}

‎src/providers/oracle/qgsoracleconn.cpp

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,6 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
9696
QgsDebugMsg( QStringLiteral( "Connecting with options: " ) + options );
9797
if ( !mDatabase.open() )
9898
{
99-
QgsCredentials::instance()->lock();
100-
10199
while ( !mDatabase.open() )
102100
{
103101
bool ok = QgsCredentials::instance()->get( realm, username, password, mDatabase.lastError().text() );
@@ -127,8 +125,6 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
127125

128126
if ( mDatabase.isOpen() )
129127
QgsCredentials::instance()->put( realm, username, password );
130-
131-
QgsCredentials::instance()->unlock();
132128
}
133129

134130
if ( !mDatabase.isOpen() )

‎src/providers/postgres/qgspostgresconn.cpp

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -272,8 +272,6 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
272272
QString username = uri.username();
273273
QString password = uri.password();
274274

275-
QgsCredentials::instance()->lock();
276-
277275
int i = 0;
278276
while ( PQstatus() != CONNECTION_OK && i < 5 )
279277
{
@@ -298,8 +296,6 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
298296

299297
if ( PQstatus() == CONNECTION_OK )
300298
QgsCredentials::instance()->put( conninfo, username, password );
301-
302-
QgsCredentials::instance()->unlock();
303299
}
304300

305301
if ( PQstatus() != CONNECTION_OK )

0 commit comments

Comments
 (0)
Please sign in to comment.