From b01ceae843c74062460fdf2f8e9fd3c5cc75ea45 Mon Sep 17 00:00:00 2001 From: Sergey Fedoseev Date: Fri, 16 Dec 2016 03:59:08 +0600 Subject: [PATCH] Fixed #25938 -- Factored out CPointerBase base class for GEOSBase/GDALBase. --- django/contrib/gis/gdal/base.py | 38 +---------- django/contrib/gis/gdal/datasource.py | 8 +-- django/contrib/gis/gdal/driver.py | 2 +- django/contrib/gis/gdal/feature.py | 8 +-- django/contrib/gis/gdal/geometries.py | 8 +-- django/contrib/gis/gdal/raster/source.py | 8 +-- django/contrib/gis/gdal/srs.py | 16 +---- django/contrib/gis/geos/base.py | 38 +---------- django/contrib/gis/geos/geometry.py | 11 +--- django/contrib/gis/geos/prepared.py | 7 +- django/contrib/gis/geos/prototypes/io.py | 19 ++---- .../contrib/gis/geos/prototypes/threadsafe.py | 10 +-- django/contrib/gis/ptr.py | 38 +++++++++++ tests/gis_tests/geos_tests/test_geos.py | 47 -------------- tests/gis_tests/geos_tests/test_io.py | 2 +- tests/gis_tests/test_ptr.py | 65 +++++++++++++++++++ 16 files changed, 131 insertions(+), 194 deletions(-) create mode 100644 django/contrib/gis/ptr.py create mode 100644 tests/gis_tests/test_ptr.py diff --git a/django/contrib/gis/gdal/base.py b/django/contrib/gis/gdal/base.py index 7405810496..74a2689f03 100644 --- a/django/contrib/gis/gdal/base.py +++ b/django/contrib/gis/gdal/base.py @@ -1,38 +1,6 @@ -from ctypes import c_void_p - from django.contrib.gis.gdal.error import GDALException -from django.utils import six +from django.contrib.gis.ptr import CPointerBase -class GDALBase(object): - """ - Base object for GDAL objects that has a pointer access property - that controls access to the underlying C pointer. - """ - # Initially the pointer is NULL. - _ptr = None - - # Default allowed pointer type. - ptr_type = c_void_p - - # Pointer access property. - def _get_ptr(self): - # Raise an exception if the pointer isn't valid don't - # want to be passing NULL pointers to routines -- - # that's very bad. - if self._ptr: - return self._ptr - else: - raise GDALException('GDAL %s pointer no longer valid.' % self.__class__.__name__) - - def _set_ptr(self, ptr): - # Only allow the pointer to be set with pointers of the - # compatible type or None (NULL). - if isinstance(ptr, six.integer_types): - self._ptr = self.ptr_type(ptr) - elif ptr is None or isinstance(ptr, self.ptr_type): - self._ptr = ptr - else: - raise TypeError('Incompatible pointer type') - - ptr = property(_get_ptr, _set_ptr) +class GDALBase(CPointerBase): + null_ptr_exception_class = GDALException diff --git a/django/contrib/gis/gdal/datasource.py b/django/contrib/gis/gdal/datasource.py index adb32a0784..4588a1731a 100644 --- a/django/contrib/gis/gdal/datasource.py +++ b/django/contrib/gis/gdal/datasource.py @@ -51,6 +51,7 @@ from django.utils.six.moves import range # The OGR_DS_* routines are relevant here. class DataSource(GDALBase): "Wraps an OGR Data Source object." + destructor = capi.destroy_ds def __init__(self, ds_input, ds_driver=False, write=False, encoding='utf-8'): # The write flag. @@ -85,13 +86,6 @@ class DataSource(GDALBase): # Raise an exception if the returned pointer is NULL raise GDALException('Invalid data source file "%s"' % ds_input) - def __del__(self): - "Destroys this DataStructure object." - try: - capi.destroy_ds(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - def __iter__(self): "Allows for iteration over the layers in a data source." for i in range(self.layer_count): diff --git a/django/contrib/gis/gdal/driver.py b/django/contrib/gis/gdal/driver.py index 030e3b162b..7ed4587073 100644 --- a/django/contrib/gis/gdal/driver.py +++ b/django/contrib/gis/gdal/driver.py @@ -49,7 +49,7 @@ class Driver(GDALBase): # Attempting to get the GDAL/OGR driver by the string name. for iface in (vcapi, rcapi): - driver = iface.get_driver_by_name(force_bytes(name)) + driver = c_void_p(iface.get_driver_by_name(force_bytes(name))) if driver: break elif isinstance(dr_input, int): diff --git a/django/contrib/gis/gdal/feature.py b/django/contrib/gis/gdal/feature.py index 7983c2f599..2335df733b 100644 --- a/django/contrib/gis/gdal/feature.py +++ b/django/contrib/gis/gdal/feature.py @@ -17,6 +17,7 @@ class Feature(GDALBase): This class that wraps an OGR Feature, needs to be instantiated from a Layer object. """ + destructor = capi.destroy_feature def __init__(self, feat, layer): """ @@ -27,13 +28,6 @@ class Feature(GDALBase): self.ptr = feat self._layer = layer - def __del__(self): - "Releases a reference to this object." - try: - capi.destroy_feature(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - def __getitem__(self, index): """ Gets the Field object at the specified index, which may be either diff --git a/django/contrib/gis/gdal/geometries.py b/django/contrib/gis/gdal/geometries.py index ef3dbb10d2..08d0a23767 100644 --- a/django/contrib/gis/gdal/geometries.py +++ b/django/contrib/gis/gdal/geometries.py @@ -62,6 +62,7 @@ from django.utils.six.moves import range # The OGR_G_* routines are relevant here. class OGRGeometry(GDALBase): "Generally encapsulates an OGR geometry." + destructor = capi.destroy_geom def __init__(self, geom_input, srs=None): "Initializes Geometry on either WKT or an OGR pointer as input." @@ -120,13 +121,6 @@ class OGRGeometry(GDALBase): # Setting the class depending upon the OGR Geometry Type self.__class__ = GEO_CLASSES[self.geom_type.num] - def __del__(self): - "Deletes this Geometry." - try: - capi.destroy_geom(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - # Pickle routines def __getstate__(self): srs = self.srs diff --git a/django/contrib/gis/gdal/raster/source.py b/django/contrib/gis/gdal/raster/source.py index 432a1eb7f4..4aea283dd8 100644 --- a/django/contrib/gis/gdal/raster/source.py +++ b/django/contrib/gis/gdal/raster/source.py @@ -57,6 +57,8 @@ class GDALRaster(GDALBase): """ Wraps a raster GDAL Data Source object. """ + destructor = capi.close_ds + def __init__(self, ds_input, write=False): self._write = 1 if write else 0 Driver.ensure_registered() @@ -143,12 +145,6 @@ class GDALRaster(GDALBase): else: raise GDALException('Invalid data source input type: "{}".'.format(type(ds_input))) - def __del__(self): - try: - capi.close_ds(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - def __str__(self): return self.name diff --git a/django/contrib/gis/gdal/srs.py b/django/contrib/gis/gdal/srs.py index 13e47132cd..635cdc2b5a 100644 --- a/django/contrib/gis/gdal/srs.py +++ b/django/contrib/gis/gdal/srs.py @@ -41,6 +41,7 @@ class SpatialReference(GDALBase): the SpatialReference object "provide[s] services to represent coordinate systems (projections and datums) and to transform between them." """ + destructor = capi.release_srs def __init__(self, srs_input='', srs_type='user'): """ @@ -91,13 +92,6 @@ class SpatialReference(GDALBase): elif srs_type == 'epsg': self.import_epsg(srs_input) - def __del__(self): - "Destroys this spatial reference." - try: - capi.release_srs(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - def __getitem__(self, target): """ Returns the value of the given string attribute node, None if the node @@ -329,6 +323,7 @@ class SpatialReference(GDALBase): class CoordTransform(GDALBase): "The coordinate system transformation object." + destructor = capi.destroy_ct def __init__(self, source, target): "Initializes on a source and target SpatialReference objects." @@ -338,12 +333,5 @@ class CoordTransform(GDALBase): self._srs1_name = source.name self._srs2_name = target.name - def __del__(self): - "Deletes this Coordinate Transformation object." - try: - capi.destroy_ct(self._ptr) - except (AttributeError, TypeError): - pass - def __str__(self): return 'Transform from "%s" to "%s"' % (self._srs1_name, self._srs2_name) diff --git a/django/contrib/gis/geos/base.py b/django/contrib/gis/geos/base.py index 9487845026..b7466ba2a1 100644 --- a/django/contrib/gis/geos/base.py +++ b/django/contrib/gis/geos/base.py @@ -1,38 +1,6 @@ -from ctypes import c_void_p - from django.contrib.gis.geos.error import GEOSException +from django.contrib.gis.ptr import CPointerBase -class GEOSBase(object): - """ - Base object for GEOS objects that has a pointer access property - that controls access to the underlying C pointer. - """ - # Initially the pointer is NULL. - _ptr = None - - # Default allowed pointer type. - ptr_type = c_void_p - - # Pointer access property. - def _get_ptr(self): - # Raise an exception if the pointer isn't valid don't - # want to be passing NULL pointers to routines -- - # that's very bad. - if self._ptr: - return self._ptr - else: - raise GEOSException('NULL GEOS %s pointer encountered.' % self.__class__.__name__) - - def _set_ptr(self, ptr): - # Only allow the pointer to be set with pointers of the - # compatible type or None (NULL). - if ptr is None or isinstance(ptr, self.ptr_type): - self._ptr = ptr - else: - raise TypeError('Incompatible pointer type') - - # Property for controlling access to the GEOS object pointers. Using - # this raises an exception when the pointer is NULL, thus preventing - # the C library from attempting to access an invalid memory location. - ptr = property(_get_ptr, _set_ptr) +class GEOSBase(CPointerBase): + null_ptr_exception_class = GEOSException diff --git a/django/contrib/gis/geos/geometry.py b/django/contrib/gis/geos/geometry.py index 164b275301..ffbbc0c859 100644 --- a/django/contrib/gis/geos/geometry.py +++ b/django/contrib/gis/geos/geometry.py @@ -33,6 +33,7 @@ class GEOSGeometry(GEOSBase, ListMixin): _GEOS_CLASSES = None ptr_type = GEOM_PTR + destructor = capi.destroy_geom has_cs = False # Only Point, LineString, LinearRing have coordinate sequences def __init__(self, geo_input, srid=None): @@ -120,16 +121,6 @@ class GEOSGeometry(GEOSBase, ListMixin): # geometries that do not have coordinate sequences) self._set_cs() - def __del__(self): - """ - Destroys this Geometry; in other words, frees the memory used by the - GEOS C++ object. - """ - try: - capi.destroy_geom(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - def __copy__(self): """ Returns a clone because the copy of a GEOSGeometry may contain an diff --git a/django/contrib/gis/geos/prepared.py b/django/contrib/gis/geos/prepared.py index c18218bdf0..789432f81a 100644 --- a/django/contrib/gis/geos/prepared.py +++ b/django/contrib/gis/geos/prepared.py @@ -9,6 +9,7 @@ class PreparedGeometry(GEOSBase): operations. """ ptr_type = capi.PREPGEOM_PTR + destructor = capi.prepared_destroy def __init__(self, geom): # Keeping a reference to the original geometry object to prevent it @@ -20,12 +21,6 @@ class PreparedGeometry(GEOSBase): raise TypeError self.ptr = capi.geos_prepare(geom.ptr) - def __del__(self): - try: - capi.prepared_destroy(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - def contains(self, other): return capi.prepared_contains(self.ptr, other.ptr) diff --git a/django/contrib/gis/geos/prototypes/io.py b/django/contrib/gis/geos/prototypes/io.py index e563aa105e..c932a1a15f 100644 --- a/django/contrib/gis/geos/prototypes/io.py +++ b/django/contrib/gis/geos/prototypes/io.py @@ -119,17 +119,10 @@ class IOBase(GEOSBase): self.ptr = self._constructor() # Loading the real destructor function at this point as doing it in # __del__ is too late (import error). - self._destructor.func = self._destructor.get_func( - *self._destructor.args, **self._destructor.kwargs + self.destructor.func = self.destructor.get_func( + *self.destructor.args, **self.destructor.kwargs ) - def __del__(self): - # Cleaning up with the appropriate destructor. - try: - self._destructor(self._ptr) - except (AttributeError, TypeError): - pass # Some part might already have been garbage collected - # ### Base WKB/WKT Reading and Writing objects ### @@ -138,8 +131,8 @@ class IOBase(GEOSBase): # objects. class _WKTReader(IOBase): _constructor = wkt_reader_create - _destructor = wkt_reader_destroy ptr_type = WKT_READ_PTR + destructor = wkt_reader_destroy def read(self, wkt): if not isinstance(wkt, (bytes, six.string_types)): @@ -149,8 +142,8 @@ class _WKTReader(IOBase): class _WKBReader(IOBase): _constructor = wkb_reader_create - _destructor = wkb_reader_destroy ptr_type = WKB_READ_PTR + destructor = wkb_reader_destroy def read(self, wkb): "Returns a _pointer_ to C GEOS Geometry object from the given WKB." @@ -166,8 +159,8 @@ class _WKBReader(IOBase): # ### WKB/WKT Writer Classes ### class WKTWriter(IOBase): _constructor = wkt_writer_create - _destructor = wkt_writer_destroy ptr_type = WKT_WRITE_PTR + destructor = wkt_writer_destroy _trim = False _precision = None @@ -219,8 +212,8 @@ class WKTWriter(IOBase): class WKBWriter(IOBase): _constructor = wkb_writer_create - _destructor = wkb_writer_destroy ptr_type = WKB_WRITE_PTR + destructor = wkb_writer_destroy def __init__(self, dim=2): super(WKBWriter, self).__init__() diff --git a/django/contrib/gis/geos/prototypes/threadsafe.py b/django/contrib/gis/geos/prototypes/threadsafe.py index 8345c385df..a293671b5f 100644 --- a/django/contrib/gis/geos/prototypes/threadsafe.py +++ b/django/contrib/gis/geos/prototypes/threadsafe.py @@ -1,23 +1,23 @@ import threading +from django.contrib.gis.geos.base import GEOSBase from django.contrib.gis.geos.libgeos import ( CONTEXT_PTR, error_h, lgeos, notice_h, ) -class GEOSContextHandle(object): +class GEOSContextHandle(GEOSBase): """ Python object representing a GEOS context handle. """ + ptr_type = CONTEXT_PTR + destructor = lgeos.finishGEOS_r + def __init__(self): # Initializing the context handler for this thread with # the notice and error handler. self.ptr = lgeos.initGEOS_r(notice_h, error_h) - def __del__(self): - if self.ptr and lgeos: - lgeos.finishGEOS_r(self.ptr) - # Defining a thread-local object and creating an instance # to hold a reference to GEOSContextHandle for this thread. diff --git a/django/contrib/gis/ptr.py b/django/contrib/gis/ptr.py new file mode 100644 index 0000000000..7d6a21a12f --- /dev/null +++ b/django/contrib/gis/ptr.py @@ -0,0 +1,38 @@ +from ctypes import c_void_p + + +class CPointerBase(object): + """ + Base class for objects that have a pointer access property + that controls access to the underlying C pointer. + """ + _ptr = None # Initially the pointer is NULL. + ptr_type = c_void_p + destructor = None + null_ptr_exception_class = AttributeError + + @property + def ptr(self): + # Raise an exception if the pointer isn't valid so that NULL pointers + # aren't passed to routines -- that's very bad. + if self._ptr: + return self._ptr + raise self.null_ptr_exception_class('NULL %s pointer encountered.' % self.__class__.__name__) + + @ptr.setter + def ptr(self, ptr): + # Only allow the pointer to be set with pointers of the compatible + # type or None (NULL). + if not (ptr is None or isinstance(ptr, self.ptr_type)): + raise TypeError('Incompatible pointer type: %s.' % type(ptr)) + self._ptr = ptr + + def __del__(self): + """ + Free the memory used by the C++ object. + """ + if self.destructor and self._ptr: + try: + self.destructor(self.ptr) + except (AttributeError, TypeError): + pass # Some part might already have been garbage collected diff --git a/tests/gis_tests/geos_tests/test_geos.py b/tests/gis_tests/geos_tests/test_geos.py index 176ae3d482..2aee39072d 100644 --- a/tests/gis_tests/geos_tests/test_geos.py +++ b/tests/gis_tests/geos_tests/test_geos.py @@ -14,7 +14,6 @@ from django.contrib.gis.geos import ( LineString, MultiLineString, MultiPoint, MultiPolygon, Point, Polygon, fromfile, fromstr, ) -from django.contrib.gis.geos.base import GEOSBase from django.contrib.gis.geos.libgeos import geos_version_info from django.contrib.gis.shortcuts import numpy from django.template import Context @@ -31,52 +30,6 @@ from ..test_data import TestDataMixin @skipUnless(HAS_GEOS, "Geos is required.") class GEOSTest(SimpleTestCase, TestDataMixin): - def test_base(self): - "Tests out the GEOSBase class." - # Testing out GEOSBase class, which provides a `ptr` property - # that abstracts out access to underlying C pointers. - class FakeGeom1(GEOSBase): - pass - - # This one only accepts pointers to floats - c_float_p = ctypes.POINTER(ctypes.c_float) - - class FakeGeom2(GEOSBase): - ptr_type = c_float_p - - # Default ptr_type is `c_void_p`. - fg1 = FakeGeom1() - # Default ptr_type is C float pointer - fg2 = FakeGeom2() - - # These assignments are OK -- None is allowed because - # it's equivalent to the NULL pointer. - fg1.ptr = ctypes.c_void_p() - fg1.ptr = None - fg2.ptr = c_float_p(ctypes.c_float(5.23)) - fg2.ptr = None - - # Because pointers have been set to NULL, an exception should be - # raised when we try to access it. Raising an exception is - # preferable to a segmentation fault that commonly occurs when - # a C method is given a NULL memory reference. - for fg in (fg1, fg2): - # Equivalent to `fg.ptr` - with self.assertRaises(GEOSException): - fg._get_ptr() - - # Anything that is either not None or the acceptable pointer type will - # result in a TypeError when trying to assign it to the `ptr` property. - # Thus, memory addresses (integers) and pointers of the incorrect type - # (in `bad_ptrs`) will not be allowed. - bad_ptrs = (5, ctypes.c_char_p(b'foobar')) - for bad_ptr in bad_ptrs: - # Equivalent to `fg.ptr = bad_ptr` - with self.assertRaises(TypeError): - fg1._set_ptr(bad_ptr) - with self.assertRaises(TypeError): - fg2._set_ptr(bad_ptr) - def test_wkt(self): "Testing WKT output." for g in self.geometries.wkt_out: diff --git a/tests/gis_tests/geos_tests/test_io.py b/tests/gis_tests/geos_tests/test_io.py index 601301331e..47995bf88c 100644 --- a/tests/gis_tests/geos_tests/test_io.py +++ b/tests/gis_tests/geos_tests/test_io.py @@ -37,7 +37,7 @@ class GEOSIOTest(SimpleTestCase): # Creating a WKTWriter instance, testing its ptr property. wkt_w = WKTWriter() with self.assertRaises(TypeError): - wkt_w._set_ptr(WKTReader.ptr_type()) + wkt_w.ptr = WKTReader.ptr_type() ref = GEOSGeometry('POINT (5 23)') ref_wkt = 'POINT (5.0000000000000000 23.0000000000000000)' diff --git a/tests/gis_tests/test_ptr.py b/tests/gis_tests/test_ptr.py new file mode 100644 index 0000000000..1b8beaf4e7 --- /dev/null +++ b/tests/gis_tests/test_ptr.py @@ -0,0 +1,65 @@ +import ctypes + +from django.contrib.gis.ptr import CPointerBase +from django.test import SimpleTestCase, mock + + +class CPointerBaseTests(SimpleTestCase): + + def test(self): + destructor_mock = mock.Mock() + + class NullPointerException(Exception): + pass + + class FakeGeom1(CPointerBase): + null_ptr_exception_class = NullPointerException + + class FakeGeom2(FakeGeom1): + ptr_type = ctypes.POINTER(ctypes.c_float) + destructor = destructor_mock + + fg1 = FakeGeom1() + fg2 = FakeGeom2() + + # These assignments are OK. None is allowed because it's equivalent + # to the NULL pointer. + fg1.ptr = fg1.ptr_type() + fg1.ptr = None + fg2.ptr = fg2.ptr_type(ctypes.c_float(5.23)) + fg2.ptr = None + + # Because pointers have been set to NULL, an exception is raised on + # access. Raising an exception is preferable to a segmentation fault + # that commonly occurs when a C method is given a NULL reference. + for fg in (fg1, fg2): + with self.assertRaises(NullPointerException): + fg.ptr + + # Anything that's either not None or the acceptable pointer type + # results in a TypeError when trying to assign it to the `ptr` property. + # Thus, memory addresses (integers) and pointers of the incorrect type + # (in `bad_ptrs`) aren't allowed. + bad_ptrs = (5, ctypes.c_char_p(b'foobar')) + for bad_ptr in bad_ptrs: + for fg in (fg1, fg2): + with self.assertRaisesMessage(TypeError, 'Incompatible pointer type'): + fg.ptr = bad_ptr + + # Object can be deleted without a destructor set. + fg = FakeGeom1() + fg.ptr = fg.ptr_type(1) + del fg + + # A NULL pointer isn't passed to the destructor. + fg = FakeGeom2() + fg.ptr = None + del fg + self.assertFalse(destructor_mock.called) + + # The destructor is called if set. + fg = FakeGeom2() + ptr = fg.ptr_type(ctypes.c_float(1.)) + fg.ptr = ptr + del fg + destructor_mock.assert_called_with(ptr)