On Wednesday, October 08, 2014 07:56:50 AM Daniel Shahaf wrote:
> Alexey Neyman wrote on Tue, Oct 07, 2014 at 17:27:09 -0700:
> > I have encountered a problem with Subversion's Python bindings. For
> > example,
> > the following simple program does not work:
> Which version of the bindings? trunk?
>
> > Am I correct in assuming that the division of a single 'pool' argument
> > into distinct 'result_pool' and 'scratch_pool' is rather recent change
> > and the bindings were not updated for that?
>
> The first instances are a few years old (1.6.0 svn_wc.h has dual pools).
> New instances of dual pools happen when we create an entirely new public
> API, or when we rev a single-pool API (i.e., generally, a pre-1.6 API),
> in which case we make the revved API use dual pools.
>
> > Given all that, I could think of two approaches to fixing this issue.
> >
> > 1. Hackish, but simple.
> >
> > Change the call signature of the functions receiving one or two pool
> > arguments: make them always receive a single argument for any pools
> > it may have, which may be either None (meaning "use application pool"),
> > a pool object (which will be used for both the result and scratch
> > allocations) or a (result_pool, scratch_pool) tuple.
> >
> > This approach has two obvious drawbacks: first, it changes the
> > number of arguments on the existing interfaces (but this may not
> > be a bad thing, actually, given that it makes the breakage more
> > explicit - rather than subtly using the scratch pool as described above).
> > Second, it makes the argument lists for functions taking two pool
> > arguments not match their C counterparts. E.g., svn_stream_open_readonly
> >
> > would be invoked as:
> > // C
> > res = svn_stream_open_readonly(&stream, "path",
> >
> > result_pool, scratch_pool);
> >
> > # Python
> > stream = core.svn_stream_open_readonly("path", \
> >
> > (result_pool, scratch_pool))
>
> You said you had two approaches, but you only list one?
>
> Daniel
> (I don't have time to digest your mail fully right now, although I get
> the big picture.)
Err, was the email truncated in your mailbox somehow? Right below the quoted piece was
the following:
Additionally, if there are functions that only take a scratch_pool
argument, but still need a permanent pool for some allocations, they
would need to be called with such tuple explicitly - at least with
a (None, scratch_pool) tuple - even though the C counterpart only
has a single pool argument. This is somewhat counterintuitive and
error-prone, I think.
2. Less hackish, but requiring more effort.
Do not use the passed-in pools for allocations in the wrapper
itself. Instead, implement svn_swig_py_get_scratch_pool() and
svn_swig_py_get_persistent_pool() - to create subpools of the
application pool - and use these subpool in the typemaps for other
types that need memory for conversion. The scratch pool will then
be freed at the end of the wrapper.
Persistent pools would originally be kept indefinitely. This is
a leak, but it is probably better than what we do currently
(using scratch pool for such allocations). Then, we could go
over the interfaces using such persistent pools one by one,
determine the required lifetime of such pool - record it in the
Python object with the same lifetime and destroy the pool in
that object's destructor.
I am leaning towards the second approach.
PS. I use svn_ra_session_t as an example where a semi-permanent
allocation is necessary. However, I couldn't find the interface
that destroys/closes the svn_ra_session_t. Is it correct that the
svn_ra_session_t is always destroyed by destroying the corresponding
session pool? If so, the bindings for this type need to extend it
with a destructor function to call svn_pool_destroy(session->pool),
or it will leak the memory if the application pool was used to
create it.
Received on 2014-10-08 10:26:13 CEST