[Spatialite provider] Make sure to release dangling connections on provider closing

rouault · rouault · commit 4ad50a708bc2 · 2016-06-30T17:09:47.000+02:00
Fixes #15137
diff --git a/src/providers/spatialite/qgsspatialiteprovider.cpp b/src/providers/spatialite/qgsspatialiteprovider.cpp
@@ -600,6 +600,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
@@ -37,6 +37,21 @@
 TEST_DATA_DIR = unitTestDataPath()
 
 
+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):
 
     @classmethod
@@ -217,5 +232,75 @@ def test_invalid_iterator(self):
         layer = None
         os.unlink(corrupt_dbname)
 
+    # FIXME: this test case does not work whereas equivalent OGR one does
+    # I believe the issue is that spatialite should have a Qgs
+    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()

Original file line number	Diff line number	Diff line change
`@@ -600,6 +600,7 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri )`
`600`	`600`	`QgsSpatiaLiteProvider::~QgsSpatiaLiteProvider()`
`601`	`601`	`{`
`602`	`602`	`closeDb();`
	`603`	`+ invalidateConnections( mSqlitePath );`
`603`	`604`	`}`
`604`	`605`
`605`	`606`	`QgsAbstractFeatureSource* QgsSpatiaLiteProvider::featureSource() const`