[gdal-dev] With Python bindings, should geometry objects created by ogr.Geometry() be explicitly destroyed?

Mon Sep 20 14:32:09 EDT 2010

Even Rouault wrote:
> The part where I'm not sure if about the Numpy ABI problems. I wasn't very 
> aware of problems related to it. From what I've seen in the code, it looks 
> like that the python bindings only use the C API/ABI of Numpy.

yup -- there is no C++ in numpy.

 > IMHO. However, I haven't changed anything in this section as I'm not
> 100% sure of my analysis.

aside from C rather than C++, it's all correct.

However, the numpy developers try to keep the ABI compatible as much as 
possible. A little while back a numpy version (I think 1.4.0) was 
released with binary incompatibility, somewhat by accident - it ended up 
getting retracted and an ABI compatible version released in its place. 
However, those folks that re-built something right in those few weeks 
hit the problem.

If you look at the sourceforge download site, you'll see a 1.4.1, but no 
1.4.0, for that reason.

So this is a potential issue, but not a common one.

I'm pretty sure that 1.5 is ABI backward compatible with 1.4.1 and 1.3.* 
at least, and maybe back a lot further.

2.0 may be incompatible, that's why it's called 2.0

-Chris

> Best regards,
> 
> Even
> 
> Le lundi 20 septembre 2010 18:29:26, Jason Roberts a écrit :
>> Even,
>>
>> Thank you very much for your comments. Based on them, I started a new
>> "Python Gotchas" page here http://trac.osgeo.org/gdal/wiki/PythonGotchas,
>> referenced from http://trac.osgeo.org/gdal/wiki/GdalOgrInPython. Please
>> review it to make sure I got everything right.
>>
>> Best regards,
>>
>> Jason
>>
>> -----Original Message-----
>> From: Even Rouault [mailto:even.rouault at mines-paris.org]
>> Sent: Saturday, September 18, 2010 5:52 AM
>> To: gdal-dev at lists.osgeo.org
>> Cc: Jason Roberts
>> Subject: Re: [gdal-dev] With Python bindings, should geometry objects
>> created by ogr.Geometry() be explicitly destroyed?
>>
>> Jason,
>>
>>> I have some Python code that uses OGR geometry objects internally,
>>> creating them like this:
>>>
>>>
>>>
>>> point = ogr.Geometry(ogr.wkbPoint)
>>>
>>>
>>>
>>> Does this code need to explicitly destroy these geometries, like the
>>> following, to avoid leaks, or can it simply allow them to go out of scope
>>> and have Python's reference counting and garbage collector clean them up?
>>>
>>>
>>>
>>> point.Destroy()
>> There's no reason to call Destroy(), at all. Native object gets destroyed
>> when Python object goes out of scope, or when they are assigned to None.
>> So replace foo.Destroy() by foo = None if you really want to control when
>> the underlying C++ object is destroyed. This is indeed generally only
>> interesting for I/O objects, like a GDALDataset or OGRDataSource, when you
>> want to control when the file handle is closed.
>>
>> ds = gdal.GetDriverByName('GTiff').Create('foo.tif', 10, 10, 1)
>> ds.GetRasterBand(1).Fill(255)
>> ds = None # Now, foo.tif is closed and I can safely copy it
>> shutil.copy('foo.tif', 'other.tif')
>>
>>> Does the situation change if I call feature.SetGeometry(point)? What
>>> about feature.SetGeometryDirectly(point)?
>> No, you don't need to call Destroy() either. SetGeometryDirectly() brings a
>> bit of subtelty though : it makes the (C++) feature object the owner of the
>> (C++) geometry object (whereas SetGeometry() just clones the passed
>> geometry). The consequence for Python side is then when feature goes out
>> of scope, C++ feature object dies and the C++ geometry object too, so the
>> Python point object just referes to a dead C++ object.
>>
>> So do *NOT* do that :
>>
>> point = ogr.Geometry(ogr.wkbPoint)
>> feature = ogr.Feature(layer_defn)
>> feature.SetGeometryDirectly(point)
>> feature = None # point becomes implicitely invalid at that point
>> print point.ExportToWkt() # here you die
>>
>> The issue is that the bindings here expose something that is probably
>> manageable for C++ programmers used to take into account object lifetime,
>> but in Python, this can have surprising effect...
>>
>>> I'm sorry for my ignorance here. I found a nice GDAL tutorial that seems
>>> to say they *should* be explicitly destroyed in certain circumstances
>>> (see
>>> http://www.gis.usu.edu/~chrisg/python/2009/lectures/ospy_slides2.pdf,
>>> page 12). But I have not really seen any other examples of this.
>> Destroy() was perhaps necessary with old-gen bindings, but I'm not even
>> sure of that... Perhaps this shouldn't have been exposed at all... But, as
>> mentionned in the slides, it is true that there are situations where you
>> shouldn't call Destroy() at all.
>>
>> Do *NOT* do this :
>>
>> feat = lyr.GetNextFeature()
>> geom = feat.GetGeometryRef()
>> geom.Destroy() # Arg! C++ feature still points at the underlying C++
>> geometry, but is is now dead
>> feat = None # this will destroy the C++ feature, that will try to destroy
>> the C++ geometry, but this one is already destroyed --> segfault !
>>
>>> If you follow gdal-dev, you probably notice that the "to destroy or not
>>> to destroy" question comes up periodically from Python programmers, as
>>> does the problem where someone gets a GDALDataset, gets a band, allows
>>> the GDALDataset to go out of scope, and then expects the band to still
>>> work. These are areas where the GDAL Python bindings to not work like
>>> other Python libraries, causing confusion among Python programmers, who
>>> generally expect reference counting that "just works" and that they only
>>> need to explicitly close something when some kind of I/O is involved.
>> Agreed.
>>
>> I wouldn't say that is a problem of reference counting, but more a problem
>> of ownership and lifetime of C++ objects, that are sometimes destroyed
>> when a Python object get out of scope, but shouldn't since another Python
>> object indirectly still need them to be alive.
>>
>> Do *NOT* do this :
>>
>> feat = lyr.GetNextFeature()
>> geom = feat.GetGeometryRef()
>> feat = None # This kills the C++ feature, and its C++ geometry
>> print(geom.ExportToWkt()) # segfault here. The C++ geometry no longer exist
>>
>> Byt you could do (and this is what the patch in ticket 3592 I mention below
>> does under the hood):
>>
>> feat = lyr.GetNextFeature()
>> geom = feat.GetGeometryRef()
>> geom.please_keep_alive_object_after_equal_sign = feat
>> feat = None # The underlying C++ object is still alive, since geom keeps
>> the python feature object alive
>> print(geom.ExportToWkt())
>>
>>
>> Similarly, do *NOT* do this :
>>
>> ds = gdal.Open('foo.tif')
>> band = ds.GetRasterBand(1)
>> ds = None
>> print(band.Checksum())
>>
>> But you could do :
>>
>> ds = gdal.Open('foo.tif')
>> band = ds.GetRasterBand(1)
>> band.please_keep_alive_object_after_equal_sign = ds
>> ds = None
>> print(band.Checksum())
>>
>>
>> Do *NOT* do this also :
>>
>> print(gdal.Open('foo.tif').GetRasterBand(1).Checksum())
>>
>>> It would be really great if there was FAQ that pointed out all of the
>>> ways that GDAL differs from typical Python libraries. The scenarios I
>>> mention above seem to be two of the most common ones. Would someone be
>>> willing to create it? I would but do not feel competent enough at this
>>> point.
>> If you start a wiki page collecting all the info you found in the mailing
>> list threads, that would be a beginning...
>>
>> Obviously, instead of documenting weird behaviour a better approach would
>> be to fix the code so it behaves as expected...
>>
>> There was an attempt I pursued in http://trac.osgeo.org/gdal/ticket/3592 ,
>> that would solve all the crashes in the above examples, but as you can see
>> in the ticket, it raises backward compatibility issues in some cases
>> (including the GDAL/OGR python autotest suite)... So the question is a bit
>> like : should we inflict pain to existing users (in a few scenarios) to
>> make the life of new users easier... ?
>>
>> But the patch doesn't solve all cases. Unfortunately, there would be still
>> ways of causing segfault from Python. You can break some subtle API
>> contracts, like adding a new field to a layer when features deriving from
>> this layer definition are still active. And it isn't obvious how we could
>> detect such situations to prevent them from happening (see
>> http://trac.osgeo.org/gdal/ticket/3552 )
>>
>> Do *NOT* do that :
>>
>> feature = lyr.GetNextFeature()
>> field_defn = ogr.FieldDefn("foo", ogr.OFTString)
>> lyr.CreateField(field_defn) # now, existing features deriving from this
>> layer are invalid
>> feature.DumpReadable() # segfault
>>
>>> Thanks,
>>>
>>> Jason
> _______________________________________________
> gdal-dev mailing list
> gdal-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/gdal-dev

-- 
Christopher Barker, Ph.D.
Oceanographer

Emergency Response Division
NOAA/NOS/OR&R            (206) 526-6959   voice
7600 Sand Point Way NE   (206) 526-6329   fax
Seattle, WA  98115       (206) 526-6317   main reception

Chris.Barker at noaa.gov