[OGR provider] Make sure to release dangling connections on provider …

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

Hide file tree

Showing 2 changed files with 89 additions and 0 deletions.
diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -385,6 +385,9 @@ 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() );
 }
 
 QgsAbstractFeatureSource* QgsOgrProvider::featureSource() const

diff --git a/tests/src/python/test_provider_ogr.py b/tests/src/python/test_provider_ogr.py
@@ -14,6 +14,7 @@
 
 import os
 import shutil
+import sys
 import tempfile
 
 from qgis.core import QgsVectorLayer, QgsVectorDataProvider
@@ -29,6 +30,21 @@
 # Note - doesn't implement ProviderTestCase as most OGR provider is tested by the shapefile provider test
 
 
+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 PyQgsOGRProvider(unittest.TestCase):
 
     @classmethod
@@ -66,6 +82,76 @@ def testUpdateMode(self):
         self.assertTrue(vl.dataProvider().leaveUpdateMode())
         self.assertEqual(vl.dataProvider().property("_debug_open_mode"), "read-write")
 
+    def testNoDanglingFileDescriptorAfterCloseVariant1(self):
+        ''' Test that when closing the provider all file handles are released '''
+
+        datasource = os.path.join(self.basetestpath, 'testNoDanglingFileDescriptorAfterCloseVariant1.csv')
+        with open(datasource, 'wt') as f:
+            f.write('id,WKT\n')
+            f.write('1,\n')
+            f.write('2,POINT(2 49)\n')
+
+        vl = QgsVectorLayer(u'{}|layerid=0'.format(datasource), u'test', u'ogr')
+        self.assertTrue(vl.isValid())
+        # 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())
+
+        if sys.platform.startswith('linux'):
+            self.assertEqual(count_opened_filedescriptors(datasource), 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(datasource), 1)
+
+        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(datasource), 0)
+
+        # Check that deletion works well (can only fail on Windows)
+        os.unlink(datasource)
+        self.assertFalse(os.path.exists(datasource))
+
+    def testNoDanglingFileDescriptorAfterCloseVariant2(self):
+        ''' Test that when closing the provider all file handles are released '''
+
+        datasource = os.path.join(self.basetestpath, 'testNoDanglingFileDescriptorAfterCloseVariant2.csv')
+        with open(datasource, 'wt') as f:
+            f.write('id,WKT\n')
+            f.write('1,\n')
+            f.write('2,POINT(2 49)\n')
+
+        vl = QgsVectorLayer(u'{}|layerid=0'.format(datasource), u'test', u'ogr')
+        self.assertTrue(vl.isValid())
+        # Consume all features.
+        myiter = vl.getFeatures()
+        for feature in myiter:
+            pass
+        # The iterator is closed, but the corresponding connection still not closed
+        if sys.platform.startswith('linux'):
+            self.assertEqual(count_opened_filedescriptors(datasource), 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(datasource), 0)
+
+        # Check that deletion works well (can only fail on Windows)
+        os.unlink(datasource)
+        self.assertFalse(os.path.exists(datasource))
 
 if __name__ == '__main__':
     unittest.main()