From 5c94938d7b74a18faedbc5a4e0d2802f6cacf532 Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Wed, 7 Aug 2024 15:01:01 +0200 Subject: [PATCH 1/3] Allow creation of OGR providers in parallel (when opening projects) --- src/core/providers/ogr/qgsogrprovidermetadata.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/providers/ogr/qgsogrprovidermetadata.cpp b/src/core/providers/ogr/qgsogrprovidermetadata.cpp index f2f028b05d3b..3f409a2be4ff 100644 --- a/src/core/providers/ogr/qgsogrprovidermetadata.cpp +++ b/src/core/providers/ogr/qgsogrprovidermetadata.cpp @@ -1250,7 +1250,7 @@ void QgsOgrProviderMetadata::saveConnection( const QgsAbstractProviderConnection QgsProviderMetadata::ProviderCapabilities QgsOgrProviderMetadata::providerCapabilities() const { - return FileBasedUris | SaveLayerMetadata; + return FileBasedUris | SaveLayerMetadata | ParallelCreateProvider; } ///@endcond From 6630c22cdea2b239bf0e613c421fe7a915973839 Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Wed, 7 Aug 2024 15:02:39 +0200 Subject: [PATCH 2/3] Fix a deadlock when loading multiple OGR providers at once There was a deadlock between QgsOgrProvider's loadMetadata() and open() functions: - loadMetadata() would first lock data source related mutex (QgsOgrProviderUtils::DatasetWithLayers::mutex) and then lock the global mutex (sGlobalMutex) - the open() function would call QgsOgrProviderUtils::getLayer() where the order of locking is reversed, i.e. first lock the global mutex, then the data source related mutex Fixed by updating loadMetadata() to lock data source related mutex for the minimum time necessary (certainly before the global mutex gets locked). --- src/core/providers/ogr/qgsogrprovider.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp index 2af338597ac7..0e38e6caa3a6 100644 --- a/src/core/providers/ogr/qgsogrprovider.cpp +++ b/src/core/providers/ogr/qgsogrprovider.cpp @@ -1018,14 +1018,17 @@ void QgsOgrProvider::loadMetadata() { QRecursiveMutex *mutex = nullptr; OGRLayerH layer = mOgrOrigLayer->getHandleAndMutex( mutex ); - QMutexLocker locker( mutex ); - const QString identifier = GDALGetMetadataItem( layer, "IDENTIFIER", "" ); - if ( !identifier.isEmpty() ) - mLayerMetadata.setTitle( identifier ); // see geopackage specs -- "'identifier' is analogous to 'title'" - const QString abstract = GDALGetMetadataItem( layer, "DESCRIPTION", "" ); - if ( !abstract.isEmpty() ) - mLayerMetadata.setAbstract( abstract ); + { + QMutexLocker locker( mutex ); + + const QString identifier = GDALGetMetadataItem( layer, "IDENTIFIER", "" ); + if ( !identifier.isEmpty() ) + mLayerMetadata.setTitle( identifier ); // see geopackage specs -- "'identifier' is analogous to 'title'" + const QString abstract = GDALGetMetadataItem( layer, "DESCRIPTION", "" ); + if ( !abstract.isEmpty() ) + mLayerMetadata.setAbstract( abstract ); + } if ( mGDALDriverName == QLatin1String( "GPKG" ) ) { From 5a94278dc25c088bf62faf7ea0dd6539f24e425d Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Wed, 7 Aug 2024 15:18:40 +0200 Subject: [PATCH 3/3] Allow opening of GDAL datasets in OGR provider in parallel Until now, creation of GDAL datasets from within OGR provider would get serialized, because QgsOgrProviderUtils::getLayer() starts by locking a global mutex. This is now changed to lock only for the time while shared data structures need to be accessed, and therefore it is possible that multiple threads can open GDAL datasets in parallel. --- .../providers/ogr/qgsogrproviderutils.cpp | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/core/providers/ogr/qgsogrproviderutils.cpp b/src/core/providers/ogr/qgsogrproviderutils.cpp index ed29a42da452..1fda20a1ee88 100644 --- a/src/core/providers/ogr/qgsogrproviderutils.cpp +++ b/src/core/providers/ogr/qgsogrproviderutils.cpp @@ -2253,7 +2253,11 @@ QgsOgrProviderUtils::DatasetWithLayers *QgsOgrProviderUtils::createDatasetWithLa errCause = QObject::tr( "Cannot open %1." ).arg( dsName ); return nullptr; } - ( *sMapDSNameToLastModifiedDate() )[dsName] = getLastModified( dsName ); + + { + QMutexLocker locker( sGlobalMutex() ); + ( *sMapDSNameToLastModifiedDate() )[dsName] = getLastModified( dsName ); + } OGRLayerH hLayer = GDALDatasetGetLayerByName( hDS, layerName.toUtf8().constData() ); @@ -2354,15 +2358,26 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName, return layer; } + // It can take a while to open a dataset (especially if it is from a remote source), + // so let's unlock the mutex while opening, so that we can create datasets from other + // threads as well. + locker.unlock(); + QgsOgrLayerUniquePtr layer; QgsOgrProviderUtils::DatasetWithLayers *ds = createDatasetWithLayers( dsName, updateMode, options, layerName, ident, layer, errCause ); if ( !ds ) return nullptr; - QList datasetList; - datasetList.push_back( ds ); - sMapSharedDS[ident] = datasetList; + locker.relock(); + + // In theory it could have happened that some other thread has opened this dataset + // at the same time as we did, let's only add a new entry to sMapSharedDS if there's none + if ( !sMapSharedDS.contains( ident ) ) + { + sMapSharedDS[ident] = QList(); + } + sMapSharedDS[ident].push_back( ds ); return layer; }