Skip to content

Commit

Permalink
Merge branch 'fix_release_conn_on_ogr_and_spatialite_closing'
Browse files Browse the repository at this point in the history
  • Loading branch information
rouault committed Jun 30, 2016
2 parents 0409f5e + 4ad50a7 commit 5188a52
Show file tree
Hide file tree
Showing 4 changed files with 175 additions and 0 deletions.
3 changes: 3 additions & 0 deletions src/providers/ogr/qgsogrprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/providers/spatialite/qgsspatialiteprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,7 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri )
QgsSpatiaLiteProvider::~QgsSpatiaLiteProvider()
{
closeDb();
invalidateConnections( mSqlitePath );
}

QgsAbstractFeatureSource* QgsSpatiaLiteProvider::featureSource() const
Expand Down
86 changes: 86 additions & 0 deletions tests/src/python/test_provider_ogr.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import os
import shutil
import sys
import tempfile

from qgis.core import QgsVectorLayer, QgsVectorDataProvider, QgsWKBTypes
Expand All @@ -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
Expand Down Expand Up @@ -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()
85 changes: 85 additions & 0 deletions tests/src/python/test_provider_spatialite.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()

0 comments on commit 5188a52

Please sign in to comment.