Re: [PATCH] svn_client_ctx_t callback fixes, mark 3

On 6/4/06, Walter Mundt <emage@spamcop.net> wrote:
> Further refinement/discussion and the addition of some reference count
> management code has resulted in another version of this patch.
> [...]
> Third time's the charm? ;)

That's for sure! Oh man, this is a fantastic patch. I added a bunch of
extra documentation to explain what you did, and committed this in
r19930. Details below.

> [[[
> Add necessary plumbing to Python bindings to allow the callbacks defined
> in svn_client_ctx_t to function.
>
> [ In subversion/bindings/swig ]
>
> * include/proxy_apr.swg:
> (apr_pool_t::_add_owned_ref, apr_pool_t::_remove_owned_ref): New
> pool "owned reference" management functions.
> (apr_pool_t::destroy): Now clears all "owned reference" items.

These functions keep track of Python objects which are needed by this
pool. In this patch, we use memory pools to keep track of callback
functions and batons.

Here's an example of why this is necessary. When you type
"client_ctx.log_msg_baton2 = some_object", the PY_AS_VOID typemap
converts "some_object" into a simple pointer, and stores it inside the
client context as the log message baton. OK, that's great -- but how
do we ensure that the memory allocated by Python is still around when
the log message callback dereferences that pointer? That's where
"_add_owned_ref" comes in.

The PY_AS_VOID typemap marks "some_object" as owned by the same pool
as the client_ctx object. In other words, it uses _add_owned_ref to
add a reference to "some_object" from the pool. Therefore, as long as
the pool that owns "client_ctx" is around, "some_object" will be there
too. Therefore, we can be sure that some_object will be there when we
need it.

Some folks might ask: Why not just increment the reference count of
"some_object" in the PY_AS_VOID typemap?

Well sure, incrementing the reference count of "some_object" would
make sure that we keep it around, but then, unless we have a
corresponding call to decrement the reference count when the object is
no longer needed, we'll have a memory leak.

By simply adding a reference to "some_object" into the Python pool
object, we let Python keep track of the references. When Python kills
the pool, it'll kill the reference to "some_object" as well, therefore
freeing up the memory.

Great work! I added my explanation above into proxy_apr.swg so as to
help document the new behaviour.

> * python/libsvn_swig_py/swigutil_py.c:
> (proxy_get_pool): New static function for getting the parent pool of
> a proxy object. Logic extracted from svn_swig_MustGetPtr.
> (svn_swig_MustGetPtr): Extracted proxy_get_pool function from here.

Nice refactoring. This makes the code in swigutil_py.c easier to read.

> (svn_swig_py_pool_set_owned_ref): New helper function to manage pool
> "owned references."

To quote the documentation: This function "replaces an 'owned
reference' allocated in a pool from oldRef to newRef. If oldRef is
non-NULL and present in the parent pool of proxy, it is removed."

The replace operation in svn_swig_py_pool_set_owned_ref function is
atomic and thread-safe, assuming that the caller is holding the Python
global interpreter lock. (Is this a safe assumption to make?)

> * python/tests/client.py:
> Fixed up doc comments throughout.
> (SubversionRepositoryTestCase): Renamed test case class to
> SubversionClientTestCase (original name is a probable typo)
> (test_mkdir_url, test_log3_url): New tests.
> (test_client_ctx_baton_lifetime): New test for PY_AS_VOID reference
> counting semantics.

Nice tests! I added a few explanatory comments to the test case so as
to explain what you tested and why.

> + self.change_auther = None
Oops. This should be "self.change_author". I fixed this before committing.

> /* -----------------------------------------------------------------------
> + Mapper to automatically turn Python objects into void* batons on assignment
> +*/
> +
> +#ifdef SWIGPYTHON
> +%typemap(in) void *PY_AS_VOID (PyObject *newRef) {
> + newRef = $input;
> + if ($input == Py_None) {
> + $1 = newRef = NULL;
> + } else {
> + newRef = $input;
> + $1 = (void *)$input;
> + }
> + if (svn_swig_py_pool_set_owned_ref(obj0, (PyObject *)arg1->$1_name, newRef)) {
> + SWIG_fail;
> + }
> +}
> +#endif

What do "obj0" and "arg1->$1_name" refer to? Why are these hacks
necessary? It would be nice to follow up with a comment to explain
what's going on here.

Overall, great work, Walter! The extra work you took here to implement
completely correct memory management for the callback functions will
pay off big time in the long run.

Thanks,

David

-- 
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org