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

rouault · rouault · commit 2d825bcdf08b · 2016-06-30T17:09:02.000+02:00
Fixes #15137
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, QgsWKBTypes
@@ -34,6 +35,21 @@ def GDAL_COMPUTE_VERSION(maj, min, rev):
 # 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
@@ -134,6 +150,76 @@ def testGpxElevation(self):
         self.assertEqual(f.constGeometry().geometry().pointN(1).z(), 2)
         self.assertEqual(f.constGeometry().geometry().pointN(2).z(), 3)
 
+    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()

Original file line number	Diff line number	Diff line change
`@@ -385,6 +385,9 @@ QgsOgrProvider::~QgsOgrProvider()`
`385`	`385`	`{`
`386`	`386`	`close();`
`387`	`387`	`QgsOgrConnPool::instance()->unref( dataSourceUri() );`
	`388`	`+ // We must also make sure to flush unusef cached connections so that`
	`389`	`+ // the file can be removed (#15137)`
	`390`	`+ QgsOgrConnPool::instance()->invalidateConnections( dataSourceUri() );`
`388`	`391`	`}`
`389`	`392`
`390`	`393`	`QgsAbstractFeatureSource* QgsOgrProvider::featureSource() const`