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

[Even Rouault]

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