Skip to content

Commit c9e7616

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
·
ltr-3_40final-3_6_0
1 parent 59aea69 commit c9e7616

File tree

9 files changed

+31
-54
lines changed

9 files changed

+31
-54
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: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1589,10 +1589,6 @@ QgisApp::~QgisApp()
15891589
qDeleteAll( mCustomDropHandlers );
15901590
qDeleteAll( mCustomLayoutDropHandlers );
15911591

1592-
// replace gui based network access managers with failing only ones
1593-
QgsNetworkAccessManager::instance()->setSslErrorHandler( qgis::make_unique< QgsSslErrorHandler >() );
1594-
QgsNetworkAccessManager::instance()->setAuthHandler( qgis::make_unique< QgsNetworkAuthenticationHandler >() );
1595-
15961592
const QList<QgsMapCanvas *> canvases = mapCanvases();
15971593
for ( QgsMapCanvas *canvas : canvases )
15981594
{
@@ -13978,7 +13974,6 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
1397813974
bool ok;
1397913975

1398013976
{
13981-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1398213977
ok = QgsCredentials::instance()->get(
1398313978
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
1398413979
username, password,
@@ -13992,15 +13987,13 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
1399213987

1399313988
// credentials didn't change - stored ones probably wrong? clear password and retry
1399413989
{
13995-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1399613990
QgsCredentials::instance()->put(
1399713991
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
1399813992
username, QString() );
1399913993
}
1400013994
}
1400113995

1400213996
{
14003-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
1400413997
QgsCredentials::instance()->put(
1400513998
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
1400613999
username, password

‎src/app/qgsappauthrequesthandler.cpp

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -44,39 +44,28 @@ void QgsAppAuthRequestHandler::handleAuthRequest( QNetworkReply *reply, QAuthent
4444

4545
for ( ;; )
4646
{
47-
bool ok;
48-
49-
{
50-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
51-
ok = QgsCredentials::instance()->get(
52-
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
53-
username, password,
54-
QObject::tr( "Authentication required" ) );
55-
}
47+
bool ok = QgsCredentials::instance()->get(
48+
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
49+
username, password,
50+
QObject::tr( "Authentication required" ) );
5651
if ( !ok )
5752
return;
5853

5954
if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
6055
break;
6156

6257
// credentials didn't change - stored ones probably wrong? clear password and retry
63-
{
64-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
65-
QgsCredentials::instance()->put(
66-
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
67-
username, QString() );
68-
}
69-
}
70-
71-
// save credentials
72-
{
73-
QMutexLocker lock( QgsCredentials::instance()->mutex() );
7458
QgsCredentials::instance()->put(
7559
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
76-
username, password
77-
);
60+
username, QString() );
7861
}
7962

63+
// save credentials
64+
QgsCredentials::instance()->put(
65+
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
66+
username, password
67+
);
68+
8069
auth->setUser( username );
8170
auth->setPassword( password );
8271
}

‎src/core/auth/qgsauthmanager.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3193,10 +3193,8 @@ bool QgsAuthManager::masterPasswordInput()
31933193
if ( ! ok )
31943194
{
31953195
QgsCredentials *creds = QgsCredentials::instance();
3196-
creds->lock();
31973196
pass.clear();
31983197
ok = creds->getMasterPassword( pass, masterPasswordHashInDatabase() );
3199-
creds->unlock();
32003198
}
32013199

32023200
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
@@ -235,7 +235,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
235235
db.setPort( port.toInt() );
236236
bool connected = false;
237237
int i = 0;
238-
QgsCredentials::instance()->lock();
239238
while ( !connected && i < 3 )
240239
{
241240
i++;
@@ -249,7 +248,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
249248
{
250249
errMsg = QStringLiteral( "Cancel clicked" );
251250
QgsDebugMsg( errMsg );
252-
QgsCredentials::instance()->unlock();
253251
break;
254252
}
255253
}
@@ -291,7 +289,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
291289
{
292290
QgsCredentials::instance()->put( databaseName, userName, password );
293291
}
294-
QgsCredentials::instance()->unlock();
295292

296293
return db;
297294
}

‎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.