Fix database locking when editing GeoPackage

Concurrent read and write can lock a GeoPackage database given
the default journaling mode of SQLite (delete). Use WAL when
possible to avoid that.

Fixes #15351

Author: rouault

src/providers/ogr/qgsogrconnpool.h

@@ -17,6 +17,7 @@
 #define QGSOGRCONNPOOL_H
 
 #include "qgsconnectionpool.h"
+#include "qgsogrprovider.h"
 #include <ogr_api.h>
 
 
@@ -43,7 +44,7 @@ inline void qgsConnectionPool_ConnectionCreate( QString connInfo, QgsOgrConn*& c
 
 inline void qgsConnectionPool_ConnectionDestroy( QgsOgrConn* c )
 {
-  OGR_DS_Destroy( c->ds );
+  QgsOgrProviderUtils::OGRDestroyWrapper( c->ds );
   delete c;
 }
 

src/providers/ogr/qgsogrfeatureiterator.cpp

@@ -254,6 +254,12 @@ bool QgsOgrFeatureIterator::close()
 
   iteratorClosed();
 
+  // Will for example release SQLite3 statements
+  if ( ogrLayer )
+  {
+    OGR_L_ResetReading( ogrLayer );
+  }
+
   if ( mSubsetStringSet )
   {
     OGR_DS_ReleaseResultSet( mConn->ds, ogrLayer );
@@ -263,6 +269,7 @@ bool QgsOgrFeatureIterator::close()
     QgsOgrConnPool::instance()->releaseConnection( mConn );
 
   mConn = nullptr;
+  ogrLayer = nullptr;
 
   mClosed = true;
   return true;

src/providers/ogr/qgsogrprovider.cpp

@@ -48,6 +48,13 @@ email                : sherman at mrcc.com
 #include "qgsvectorlayerimport.h"
 #include "qgslocalec.h"
 
+#ifdef Q_OS_WIN
+#include <windows.h>
+#endif
+#ifdef Q_OS_LINUX
+#include <sys/vfs.h>
+#endif
+
 static const QString TEXT_PROVIDER_KEY = "ogr";
 static const QString TEXT_PROVIDER_DESCRIPTION =
   QString( "OGR data provider" )
@@ -382,11 +389,14 @@ QgsOgrProvider::QgsOgrProvider( QString const & uri )
 
 QgsOgrProvider::~QgsOgrProvider()
 {
-  close();
   QgsOgrConnPool::instance()->unref( dataSourceUri() );
   // We must also make sure to flush unusef cached connections so that
   // the file can be removed (#15137)
   QgsOgrConnPool::instance()->invalidateConnections( dataSourceUri() );
+
+  // Do that as last step for final cleanup that might be prevented by
+  // still opened datasets.
+  close();
 }
 
 QgsAbstractFeatureSource* QgsOgrProvider::featureSource() const
@@ -2903,6 +2913,125 @@ OGRDataSourceH QgsOgrProviderUtils::OGROpenWrapper( const char* pszPath, bool bU
   return hDS;
 }
 
+static bool IsLocalFile( const QString& path )
+{
+  QString dirName( QFileInfo( path ).absolutePath() );
+  // Start with the OS specific methods since the QT >= 5.4 method just
+  // return a string and not an enumerated type.
+#if defined(Q_OS_WIN)
+  if ( dirName.startsWith( "\\\\" ) )
+    return false;
+  if ( dirName.length() >= 3 && dirName[1] == ':' &&
+       ( dirName[2] == '\\' || dirName[2] == '/' ) )
+  {
+    dirName.resize( 3 );
+    return GetDriveType( dirName.toAscii().constData() ) != DRIVE_REMOTE;
+  }
+  return true;
+#elif defined(Q_OS_LINUX)
+  struct statfs sStatFS;
+  if ( statfs( dirName.toAscii().constData(), &sStatFS ) == 0 )
+  {
+    // Codes from http://man7.org/linux/man-pages/man2/statfs.2.html
+    if ( sStatFS.f_type == 0x6969 /* NFS */ ||
+         sStatFS.f_type == 0x517b /* SMB */ ||
+         sStatFS.f_type == 0xff534d42 /* CIFS */ )
+    {
+      return false;
+    }
+  }
+  return true;
+#elif QT_VERSION >= QT_VERSION_CHECK(5, 4, 0)
+  QStorageInfo info( dirName );
+  QString fileSystem( info.fileSystemType() );
+  QgsDebugMsg( QString( "Filesystem for %1 is %2" ).arg( path ).arg( fileSystem ) );
+  return path != "nfs" && path != "smbfs";
+#else
+  return true;
+#endif
+}
+
+void QgsOgrProviderUtils::OGRDestroyWrapper( OGRDataSourceH ogrDataSource )
+{
+  if ( !ogrDataSource )
+    return;
+  OGRSFDriverH ogrDriver = OGR_DS_GetDriver( ogrDataSource );
+  QString ogrDriverName = OGR_Dr_GetName( ogrDriver );
+  QString datasetName( FROM8( OGR_DS_GetName( ogrDataSource ) ) );
+  if ( ogrDriverName == "GPKG" &&
+       IsLocalFile( datasetName ) &&
+       !CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) )
+  {
+    // We need to reset all iterators on layers, otherwise we will not
+    // be able to change journal_mode.
+    int layerCount = OGR_DS_GetLayerCount( ogrDataSource );
+    for ( int i = 0; i < layerCount; i ++ )
+    {
+      OGR_L_ResetReading( OGR_DS_GetLayer( ogrDataSource, i ) );
+    }
+
+    CPLPushErrorHandler( CPLQuietErrorHandler );
+    QgsDebugMsg( "GPKG: Trying to return to delete mode" );
+    bool bSuccess = false;
+    OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
+                                           "PRAGMA journal_mode = delete",
+                                           NULL, NULL );
+    if ( hSqlLyr != NULL )
+    {
+      OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
+      if ( hFeat != NULL )
+      {
+        const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
+        bSuccess = EQUAL( pszRet, "delete" );
+        QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
+        OGR_F_Destroy( hFeat );
+      }
+    }
+    else if ( CPLGetLastErrorType() != CE_None )
+    {
+      QgsDebugMsg( QString( "Return: %1" ).arg( CPLGetLastErrorMsg() ) );
+    }
+    OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
+    CPLPopErrorHandler();
+    OGR_DS_Destroy( ogrDataSource );
+
+    // This may have not worked if the file was opened in read-only mode,
+    // so retry in update mode
+    if ( !bSuccess )
+    {
+      QgsDebugMsg( "GPKG: Trying again" );
+      CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "DELETE" );
+      ogrDataSource = OGROpen( TO8F( datasetName ), TRUE, NULL );
+      CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
+      if ( ogrDataSource )
+      {
+#ifdef QGISDEBUG
+        CPLPushErrorHandler( CPLQuietErrorHandler );
+        OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
+                                               "PRAGMA journal_mode",
+                                               NULL, NULL );
+        CPLPopErrorHandler();
+        if ( hSqlLyr != NULL )
+        {
+          OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
+          if ( hFeat != NULL )
+          {
+            const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
+            QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
+            OGR_F_Destroy( hFeat );
+          }
+          OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
+        }
+#endif
+        OGR_DS_Destroy( ogrDataSource );
+      }
+    }
+  }
+  else
+  {
+    OGR_DS_Destroy( ogrDataSource );
+  }
+}
 
 QByteArray QgsOgrProviderUtils::quotedIdentifier( QByteArray field, const QString& ogrDriverName )
 {
@@ -3138,7 +3267,22 @@ void QgsOgrProvider::open( OpenMode mode )
 
   // first try to open in update mode (unless specified otherwise)
   if ( !openReadOnly )
+  {
+    if ( QFileInfo( mFilePath ).suffix().compare( "gpkg", Qt::CaseInsensitive ) == 0 &&
+         IsLocalFile( mFilePath ) &&
+         !CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) &&
+         QSettings().value( "/qgis/walForSqlite3", true ).toBool() )
+    {
+      // For GeoPackage, we force opening of the file in WAL (Write Ahead Log)
+      // mode so as to avoid readers blocking writer(s), and vice-versa.
+      // https://www.sqlite.org/wal.html
+      // But only do that on a local file since WAL is advertized not to work
+      // on network shares
+      CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "WAL" );
+    }
     ogrDataSource = QgsOgrProviderUtils::OGROpenWrapper( TO8F( mFilePath ), true, &ogrDriver );
+    CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
+  }
 
   mValid = false;
   if ( ogrDataSource )
@@ -3268,7 +3412,7 @@ void QgsOgrProvider::close()
 
   if ( ogrDataSource )
   {
-    OGR_DS_Destroy( ogrDataSource );
+    QgsOgrProviderUtils::OGRDestroyWrapper( ogrDataSource );
   }
   ogrDataSource = nullptr;
   ogrLayer = nullptr;

src/providers/ogr/qgsogrprovider.h

@@ -15,6 +15,9 @@ email                : sherman at mrcc.com
  *                                                                         *
  ***************************************************************************/
 
+#ifndef QGSOGRPROVIDER_H
+#define QGSOGRPROVIDER_H
+
 #include "QTextCodec"
 
 #include "qgsrectangle.h"
@@ -387,4 +390,7 @@ class QgsOgrProviderUtils
     static QString quotedValue( const QVariant& value );
 
     static OGRDataSourceH OGROpenWrapper( const char* pszPath, bool bUpdate, OGRSFDriverH *phDriver );
+    static void OGRDestroyWrapper( OGRDataSourceH ogrDataSource );
 };
+
+#endif // QGSOGRPROVIDER_H

tests/src/python/test_provider_ogr_gpkg.py

@@ -31,6 +31,15 @@ def GDAL_COMPUTE_VERSION(maj, min, rev):
     return ((maj) * 1000000 + (min) * 10000 + (rev) * 100)
 
 
+class ErrorReceiver():
+
+    def __init__(self):
+        self.msg = None
+
+    def receiveError(self, msg):
+        self.msg = msg
+
+
 class TestPyQgsOGRProviderGpkg(unittest.TestCase):
 
     @classmethod
@@ -80,5 +89,72 @@ def testCurveGeometryType(self):
         # The geometries must be binarily identical
         self.assertEqual(got_geom.asWkb(), reference.asWkb(), 'Expected {}, got {}'.format(reference.exportToWkt(), got_geom.exportToWkt()))
 
+    def internalTestBug15351(self, orderClosing):
+
+        tmpfile = os.path.join(self.basetestpath, 'testBug15351.gpkg')
+        ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
+        lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(0 0)'))
+        lyr.CreateFeature(f)
+        f = None
+        ds = None
+
+        vl = QgsVectorLayer(u'{}'.format(tmpfile), u'test', u'ogr')
+        self.assertTrue(vl.startEditing())
+        self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('Point (3 50)')))
+
+        # Iterate over features (will open a new OGR connection), but do not
+        # close the iterator for now
+        it = vl.getFeatures()
+        f = QgsFeature()
+        it.nextFeature(f)
+
+        if orderClosing == 'closeIter_commit_closeProvider':
+            it = None
+
+        # Commit changes
+        cbk = ErrorReceiver()
+        vl.dataProvider().raiseError.connect(cbk.receiveError)
+        self.assertTrue(vl.commitChanges())
+        self.assertIsNone(cbk.msg)
+
+        # Close layer and iterator in different orders
+        if orderClosing == 'closeIter_commit_closeProvider':
+            vl = None
+        elif orderClosing == 'commit_closeProvider_closeIter':
+            vl = None
+            it = None
+        else:
+            assert orderClosing == 'commit_closeIter_closeProvider'
+            it = None
+            vl = None
+
+        # Test that we succeeded restoring default journal mode, and we
+        # are not let in WAL mode.
+        ds = ogr.Open(tmpfile)
+        lyr = ds.ExecuteSQL('PRAGMA journal_mode')
+        f = lyr.GetNextFeature()
+        res = f.GetField(0)
+        ds.ReleaseResultSet(lyr)
+        ds = None
+        self.assertEqual(res, 'delete')
+
+    @unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
+    # We need GDAL 2.0 to issue PRAGMA journal_mode
+    # Note: for that case, we don't strictly need turning on WAL
+    def testBug15351_closeIter_commit_closeProvider(self):
+        self.internalTestBug15351('closeIter_commit_closeProvider')
+
+    @unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
+    # We need GDAL 2.0 to issue PRAGMA journal_mode
+    def testBug15351_closeIter_commit_closeProvider(self):
+        self.internalTestBug15351('closeIter_commit_closeProvider')
+
+    @unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
+    # We need GDAL 2.0 to issue PRAGMA journal_mode
+    def testBug15351_commit_closeIter_closeProvider(self):
+        self.internalTestBug15351('commit_closeIter_closeProvider')
+
 if __name__ == '__main__':
     unittest.main()

tests/src/python/test_provider_spatialite.py

@@ -19,7 +19,7 @@
 import shutil
 import tempfile
 
-from qgis.core import QgsVectorLayer, QgsPoint, QgsFeature
+from qgis.core import QgsVectorLayer, QgsPoint, QgsFeature, QgsGeometry
 
 from qgis.testing import start_app, unittest
 from utilities import unitTestDataPath
@@ -347,6 +347,27 @@ def test_arrays(self):
         self.assertEqual(read_back['ints'], new_f['ints'])
         self.assertEqual(read_back['reals'], new_f['reals'])
 
+    # This test would fail. It would require turning on WAL
+    def XXXXXtestLocking(self):
+
+        temp_dbname = self.dbname + '.locking'
+        shutil.copy(self.dbname, temp_dbname)
+
+        vl = QgsVectorLayer("dbname=%s table=test_n (geometry)" % temp_dbname, "test_n", "spatialite")
+        self.assertTrue(vl.isValid())
+
+        self.assertTrue(vl.startEditing())
+        self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('POLYGON((0 0,1 0,1 1,0 1,0 0))')))
+
+        # The iterator will take one extra connection
+        myiter = vl.getFeatures()
+
+        # Consume one feature but the iterator is still opened
+        f = next(myiter)
+        self.assertTrue(f.isValid())
+
+        self.assertTrue(vl.commitChanges())
+
 
 if __name__ == '__main__':
     unittest.main()