Re: Python binding memory management code

On 10/20/05, Max Bowsher <maxb@ukf.net> wrote:
> Garrett Rooney wrote:
> > So I'm trying to review r16806, r16807 and r16842, so we can merge
> > them into the 1.3 branch and do away with the mod_python
> > multithreading crashes that have been reported. Part of the change
> > makes sense (the atexit moving into python land instead of C land, so
> > we can be sure it's still safe to run python code when our cleanups
> > get called), but the rest seems to be a rewrite of the underlying
> > python memory management stuff from C/Swig voodoo into python voodoo.
> > Now as far as that goes, it looks fine, the python code seems to do
> > something similar to what the C/Swig code did, at least to my
> > untrained eye, but what's missing is an overview of what the stuff is
> > generally trying to do.
> >
> > Does any such documentation exist? If not, could we perhaps write
> > some, maybe placing a descriptive comment in proxy_apr.swg would be a
> > good idea, just so the magic that occurs with the python bindings and
> > APR pools can be more easily understood.
The Python/SWIG bindings definitely need better documentation. This is
an important item on our TODO list.

The new code in proxy_apr.swg is designed to "mark pools as invalid"
when their parent pools have been garbage collected. I implemented
this trick by registering a callback with the Python garbage
collector, so that we can be notified when the parent pool is deleted.

See below:
  self._weakparent = weakref.ref(self._parent_pool._is_valid,
            lambda x: _mark_weakpool_invalid(weakself))

When the "self._parent_pool._is_valid" object is deleted, Python calls
the _mark_weakpool_invalid function. The _mark_weakpool_invalid
function marks the current pool as "destroyed", because its parent has
been destroyed.

Pools can be destroyed in one of three ways:
  pool.destroy()
  pool.clear()
  pool.__del__() (Called by Python garbage collector)

All of the above functions destroy the "_is_valid" member object, thus
triggering the _mark_weakpool_invalid function for all children. If
the child pool has already been destroyed when the
_mark_weakpool_invalid function is called, then the
_mark_weakpool_invalid function does nothing.

Garrett, does this explanation help?

> I agree. One thing in particular which I totally don't understand, and would
> like to see explained, is why it is save for make_ob_pool() to totally
> ignore its input.
In Subversion 1.2, make_ob_pool created a Python pool from a C pool.
This pool can be used by Python callback functions to allocate
temporary objects. Any objects created this way would become invalid
when Subversion destroys the associated C pool. Usually, the arguments
to the Python callback functions are also allocated from this pool.

In r16398, I updated the callback functions in the SWIG/Python
bindings to create objects from brand new Python pools. With this
change, the arguments to a callback function are no longer tethered to
a C pool. Nervertheless, we still pass in a "pool" argument to the
Python callback functions, which contains a reference to the original
C pool. If you like, you can allocate variables from this pool, but
you have to be careful: the "C" pool might be destroyed by Subversion
when your callback function exits.

In r16806, I rewrote the SWIG/Python memory management code in Python.
As a result of this change, the SWIG/Python bindings are no longer
able to track pool objects which were created in C: we can only track
pools which were created in Python.

In order to support this new design, we have three possible design decisions:
1. Leave make_ob_pool as is. Modify the Python "Pool" class so that it
can play well with pool objects created in C.
2. Update make_ob_pool to create a Python subpool from the C pool.
3. Update make_ob_pool to create a brand new Python pool, which is
untethered to the provided C pool.

Choices #1 and #2 suffer from the same problem: If the C pool is
deleted by Subversion, Python will not be notified (thus allowing for
segfaults, if the user continues to use the provided pool). So, I
chose route #3. Now, make_ob_pool creates an anonymous Python pool,
ignoring the provided C pool. This pool is just an ordinary Python
pool. If you create an object using this pool, it will act like a
normal Python object, and will be destroyed when the pool and the
object are no longer in scope.

It might be a good idea to get rid of the "pool" argument on the
make_ob_pool function, since the argument is unused. It's easier not
to do so, though, because the Python API requires that functions which
"make" Python objects accept exactly one argument. See details below.

"O&" (object) [converter, anything]
    Convert anything to a Python object through a converter function.
The function is called with anything (which should be compatible with
void *) as its argument and should return a ``new'' Python object, or
NULL if an error occurred.
Source: http://aspn.activestate.com/ASPN/docs/ActivePython/2.2/python/ext/buildValue.html

Hope this helps!

David

--
David James -- http://www.cs.toronto.edu/~james