[Spatialite provider] Make sure to release dangling connections on pr…

…ovider closing Fixes #15137
qgis · Jun 30, 2016 · c237ba7 · c237ba7
1 parent 55f172b
commit c237ba7
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 5 deletions.
diff --git a/src/providers/spatialite/qgsspatialiteconnection.h b/src/providers/spatialite/qgsspatialiteconnection.h
@@ -131,7 +131,10 @@ class QgsSqliteHandle
     //
   public:
     QgsSqliteHandle( sqlite3 * handle, const QString& dbPath, bool shared )
-        : ref( shared ? 1 : -1 ), sqlite_handle( handle ), mDbPath( dbPath )
+        : ref( shared ? 1 : -1 )
+        , sqlite_handle( handle )
+        , mDbPath( dbPath )
+        , mIsValid( true )
     {
     }
 
@@ -145,6 +148,16 @@ class QgsSqliteHandle
       return mDbPath;
     }
 
+    bool isValid() const
+    {
+      return mIsValid;
+    }
+
+    void invalidate()
+    {
+      mIsValid = false;
+    }
+
     //
     // libsqlite3 wrapper
     //
@@ -165,6 +178,7 @@ class QgsSqliteHandle
     int ref;
     sqlite3 *sqlite_handle;
     QString mDbPath;
+    bool mIsValid;
 
     static QMap < QString, QgsSqliteHandle * > handles;
 };

diff --git a/src/providers/spatialite/qgsspatialiteconnpool.h b/src/providers/spatialite/qgsspatialiteconnpool.h
@@ -36,13 +36,15 @@ inline void qgsConnectionPool_ConnectionDestroy( QgsSqliteHandle* c )
 
 inline void qgsConnectionPool_InvalidateConnection( QgsSqliteHandle* c )
 {
-  Q_UNUSED( c );
+  /* Invalidation is used in particular by the WFS provider that uses a */
+  /* temporary spatialite DB and want to delete it at some point. For that */
+  /* it must invalidate all handles pointing to it */
+  c->invalidate();
 }
 
 inline bool qgsConnectionPool_ConnectionIsValid( QgsSqliteHandle* c )
 {
-  Q_UNUSED( c );
-  return true;
+  return c->isValid();
 }
 
 

diff --git a/src/providers/spatialite/qgsspatialiteprovider.cpp b/src/providers/spatialite/qgsspatialiteprovider.cpp
@@ -589,6 +589,7 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri )
 QgsSpatiaLiteProvider::~QgsSpatiaLiteProvider()
 {
   closeDb();
+  invalidateConnections( mSqlitePath );
 }
 
 QgsAbstractFeatureSource* QgsSpatiaLiteProvider::featureSource() const

diff --git a/tests/src/python/test_provider_spatialite.py b/tests/src/python/test_provider_spatialite.py
@@ -15,6 +15,7 @@
 import qgis
 import os
 import shutil
+import sys
 import tempfile
 
 from qgis.core import QgsVectorLayer, QgsPoint, QgsFeature
@@ -36,10 +37,23 @@
 start_app()
 TEST_DATA_DIR = unitTestDataPath()
 
-
 def die(error_message):
     raise Exception(error_message)
 
+def count_opened_filedescriptors(filename_to_test):
+    count = -1
+    if sys.platform.startswith('linux'):
+        count = 0
+        open_files_dirname = '/proc/%d/fd' % os.getpid()
+        filenames = os.listdir(open_files_dirname)
+        for filename in filenames:
+            full_filename = open_files_dirname + '/' + filename
+            if os.path.exists(full_filename):
+                link = os.readlink(full_filename)
+                if os.path.basename(link) == os.path.basename(filename_to_test):
+                    count += 1
+    return count
+
 
 class TestQgsSpatialiteProvider(unittest.TestCase, ProviderTestCase):
 
@@ -214,5 +228,74 @@ def test_invalid_iterator(self):
         layer = None
         os.unlink(corrupt_dbname)
 
+
+    def testNoDanglingFileDescriptorAfterCloseVariant1(self):
+        ''' Test that when closing the provider all file handles are released '''
+
+        temp_dbname = self.dbname + '.no_dangling_test1'
+        shutil.copy(self.dbname, temp_dbname)
+
+        vl = QgsVectorLayer("dbname=%s table=test_n (geometry)" % temp_dbname, "test_n", "spatialite")
+        self.assertTrue(vl.isValid())
+        # The iterator will take one extra connection
+        myiter = vl.getFeatures()
+        print(vl.featureCount())
+        # Consume one feature but the iterator is still opened
+        f = next(myiter)
+        self.assertTrue(f.isValid())
+
+        if sys.platform.startswith('linux'):
+            self.assertEqual(count_opened_filedescriptors(temp_dbname), 2)
+
+        # does NO release one file descriptor, because shared with the iterator
+        del vl
+
+        # Non portable, but Windows testing is done with trying to unlink
+        if sys.platform.startswith('linux'):
+            self.assertEqual(count_opened_filedescriptors(temp_dbname), 2)
+
+        f = next(myiter)
+        self.assertTrue(f.isValid())
+
+        # Should release one file descriptor
+        del myiter
+
+        # Non portable, but Windows testing is done with trying to unlink
+        if sys.platform.startswith('linux'):
+            self.assertEqual(count_opened_filedescriptors(temp_dbname), 0)
+
+        # Check that deletion works well (can only fail on Windows)
+        os.unlink(temp_dbname)
+        self.assertFalse(os.path.exists(temp_dbname))
+
+    def testNoDanglingFileDescriptorAfterCloseVariant2(self):
+        ''' Test that when closing the provider all file handles are released '''
+
+        temp_dbname = self.dbname + '.no_dangling_test2'
+        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.isValid())
+        # Consume all features.
+        myiter = vl.getFeatures()
+        for feature in myiter:
+            pass
+        # The iterator is closed
+        if sys.platform.startswith('linux'):
+            self.assertEqual(count_opened_filedescriptors(temp_dbname), 2)
+
+        # Should release one file descriptor
+        del vl
+
+        # Non portable, but Windows testing is done with trying to unlink
+        if sys.platform.startswith('linux'):
+            self.assertEqual(count_opened_filedescriptors(temp_dbname), 0)
+
+        # Check that deletion works well (can only fail on Windows)
+        os.unlink(temp_dbname)
+        self.assertFalse(os.path.exists(temp_dbname))
+
+
 if __name__ == '__main__':
     unittest.main()