Fixed #25938 -- Factored out CPointerBase base class for GEOSBase/GDALBase.

16 files changed, 131 insertions, 194 deletions

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
-
-
-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__)
+from django.contrib.gis.ptr import CPointerBase
 
-    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)