Return-Path: X-Original-To: apmail-subversion-dev-archive@minotaur.apache.org Delivered-To: apmail-subversion-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 81EF2179F2 for ; Wed, 8 Oct 2014 00:27:44 +0000 (UTC) Received: (qmail 4709 invoked by uid 500); 8 Oct 2014 00:27:44 -0000 Delivered-To: apmail-subversion-dev-archive@subversion.apache.org Received: (qmail 4654 invoked by uid 500); 8 Oct 2014 00:27:44 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 4643 invoked by uid 99); 8 Oct 2014 00:27:43 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Oct 2014 00:27:43 +0000 X-ASF-Spam-Status: No, hits=2.9 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_NEUTRAL X-Spam-Check-By: apache.org Received-SPF: neutral (nike.apache.org: local policy) Received: from [216.39.62.66] (HELO nm19-vm9.access.bullet.mail.gq1.yahoo.com) (216.39.62.66) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Oct 2014 00:27:15 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1412728032; bh=+rbwaMUDNGa+eLnl1Pq/Vab0cPlUz9i56C8XbRgNdXQ=; h=Received:Received:Received:DKIM-Signature:X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:From:To:Subject:Date:Message-ID:User-Agent:MIME-Version:Content-Type:Content-Transfer-Encoding:From:Subject; b=W6six5ZwvVcDPzi50Mi/QPyVrTutJf7or1zXozGS0+YkS7ZdCZ/4R21cNpVJlYbWGmCcfHx1YhTWwHUOixc0p+bYzjuHNzXj1vE/aLWH2Z3amoYcUkaWHruwyJNKB7DhGgrS1WL0GcpEkmTlYcUerumz0jXgOMz8FtvJPoq8Prs= DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s1024; d=att.net; b=D5G4Wvc23mIwgRXhESCVnEQ8HWVUnoYmkQcxKZ2I52illqVb6vic+ToRKvazo2HX5LKfSCM2/SVWJUiIloq68sj3EdwezPcuUdLjSF5s/u/opPCdp7Th4MiFgtID5RvxGQAnwoRebUJFDGDxy8lBGP2RrHt4RNYN10kvpOB7d90=; Received: from [216.39.60.176] by nm19.access.bullet.mail.gq1.yahoo.com with NNFMP; 08 Oct 2014 00:27:12 -0000 Received: from [67.195.23.148] by tm12.access.bullet.mail.gq1.yahoo.com with NNFMP; 08 Oct 2014 00:27:12 -0000 Received: from [127.0.0.1] by smtp120.sbc.mail.gq1.yahoo.com with NNFMP; 08 Oct 2014 00:27:12 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1412728032; bh=+rbwaMUDNGa+eLnl1Pq/Vab0cPlUz9i56C8XbRgNdXQ=; h=X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:From:To:Subject:Date:Message-ID:User-Agent:MIME-Version:Content-Type:Content-Transfer-Encoding; b=QzF48FF2k9zDptUtYoWZJQ/jnF+I05yGXMIcbbt3vxojykE1TBQfBqlzvR3fRBWLqWM5malYxVP1r1ZOXwcdWHiK/GFg8POWSMgYAR5tloElLeV80/Ld3mPy5Q6vvy/uaFvDZ+J40E8j3w+jY0B9A10hagE288NSm3tWmcCWO5Q= X-Yahoo-Newman-Id: 56817.84070.bm@smtp120.sbc.mail.gq1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: w70TOtUVM1nRbSY9HQ6KGoD6Z7qDyt5wEQlaZMy3h6dehER Akq.4XtWH1F1Me5E4jrq7t38bB1MDKX5i9969gdRT_qMJy0lyAkvVRUZmU4N YaEuZDmz7yIZUZTOinGEmid5CxuS6WUxC.N68Jiv1MdngxAMCFETlzcggyHy pKU7tv1eo25VRf.2yhQHMzLuEi7RTpfwtm0juYK2p5NUVdsneMXfv_6fLGgX FlH_EI.mX7f9G_KjXqfb0YEZg.0qVXdIuXP1p7qmT3.98e5ksb8L_jrc_hOO aHpza4W1OP6pyg2lgpSC92wuoW_xC6NwUSZ3PI54DQkDtAlj9vUOmEnR8.9M UzvCQZ8cn00OsCQDG6vaYqB9SARTzpiOxPzJ2.fd2syMO_fpyRn_NiNc8Xti r7dFrwxg3c2TBtJvgjHghJjENFlZ0E_euZHFubtdpmD23hbZxRAxl4yq0TT_ R1PeSjlAx.bzsHzqu2QwI458OmWKO_g57Wffh97vg4cK2IJ0IKb0N7Xh7KyN dXa97fVJ1M2o3obZPretavI5SbX9TQO..A9TM1cz0JfzOd68JezI6ys9uWTz XFhdYlAXhounxD4hJLuq7wtxKQdIy3axQnsKCTR2vAw-- X-Yahoo-SMTP: 0h0Q7euswBD_g.kcEqbzJWRFfrba801gq1M1 From: Alexey Neyman To: Subversion Development Subject: Pool usage in Python bindings Date: Tue, 07 Oct 2014 17:27:09 -0700 Message-ID: <2678554.1FMrfMabGH@mistral> User-Agent: KMail/4.13.3 (Linux/3.13.0-36-generic; KDE/4.13.3; x86_64; ; ) MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="nextPart1601041.ERHkCIC5T3" Content-Transfer-Encoding: 7Bit X-Virus-Checked: Checked by ClamAV on apache.org This is a multi-part message in MIME format. --nextPart1601041.ERHkCIC5T3 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Hi, I have encountered a problem with Subversion's Python bindings. For example, the following simple program does not work: [[[ #!/usr/bin/python from svn import repos, core if __name__ == '__main__': stream = core.Stream(core.svn_stream_open_readonly("dump")) print stream.read() stream.close() ]]] $ ./0.py Traceback (most recent call last): File "./0.py", line 7, in print stream.read() File "/usr/lib/python2.7/dist-packages/svn/core.py", line 212, in read data = svn_stream_read(self._stream, SVN_STREAM_CHUNK_SIZE) File "/usr/lib/python2.7/dist-packages/libsvn/core.py", line 5362, in svn_stream_read return _core.svn_stream_read(*args) File "/usr/lib/python2.7/dist-packages/libsvn/core.py", line 5585, in assert_valid assert self.__dict__["_is_valid"](), "Variable has already been deleted" AssertionError: Variable has already been deleted The problem is that the function to open a stream takes two pool arguments: result_pool and scratch_pool. In the bindings, the following code is generated to wrap svn_stream_open_readonly() [formatted for readability]: [[[ if (svn_swig_py_get_pool_arg(args, SWIGTYPE_p_apr_pool_t, &_global_py_pool, &_global_pool)) SWIG_fail; arg3 = _global_pool; if (svn_swig_py_get_pool_arg(args, SWIGTYPE_p_apr_pool_t, &_global_py_pool, &_global_pool)) SWIG_fail; arg4 = _global_pool; ... result = (svn_error_t *)svn_stream_open_readonly(arg1, (char const *)arg2,arg3,arg4); ... Py_XDECREF(_global_py_pool); Py_XDECREF(_global_py_pool); ]]] In case the pool arguments are passed as default (None), each of the calls to svn_swig_py_get_pool_arg() will create a subpool of the application pool and assign it to _global_pool. Then, the 2nd subpool will be decremented twice at the end of the function, while the 1st one will leak. If the caller passes both pool arguments, it is even worse: both arg3 and arg4 will be initialized by svn_swig_get_pool_arg() to point to the scratch pool - it always checks the last argument of the function to be a valid pool and if it is - uses that to initialize the _global_pool: [[[ int argnum = PyTuple_GET_SIZE(args) - 1; if (argnum >= 0) { PyObject *input = PyTuple_GET_ITEM(args, argnum); if (input != Py_None && PyObject_HasAttrString(input, markValid)) { *pool = svn_swig_py_must_get_ptr(input, type, argnum+1); ]]] Obviously, the caller would expect the result from the wrapped function to be allocated in the result pool - it would be unexpected to see the returned variables to disappear when the *scratch* pool is freed. Even if the svn_swig_py_get_pool_arg had recovered both pool pointers correctly, an attempt to pass two pools would fail in the validity check in the wrapper function. Obviously, if one passes two distinct pools as arguments, only one of them can be equal to the _global_pool: [[[ if (obj1) { /* Verify that the user supplied a valid pool */ if (obj1 != Py_None && obj1 != _global_py_pool) { SWIG_Python_TypeError(..., obj1); ... if (obj2) { /* Verify that the user supplied a valid pool */ if (obj2 != Py_None && obj2 != _global_py_pool) { SWIG_Python_TypeError(..., obj2); ]]] 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? Now, fixing this is somewhat complicated. The typemaps for 'apr_pool_t *' use %typemap(default) to obtain the object passed in, not the %typemap(in). There's a reason for this: the _global_pool may be then used to perform the memory allocations needed to convert other arguments - which, according to Subversion's coding style, always precede the pool arguments passed last. For the %typemap(default), however, SWIG does not set the $input macro - so the $svn_argnum 'hack' cannot be used to determine the argument number from which the pool should be obtained. The allocations from thus created _global_pool can be either temporary, needed to convert a Python object into an appropriate C/APR type (e.g. svn_swig_py_seq_to_array to convert a list to array) or more permanent (such as svn_swig_py_setup_ra_callbacks - which should be kept for the lifetime of the returned svn_ra_session_t - the svn_ra_local__open() stashes the passed in 'callbacks' pointer for later use). 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)) --nextPart1601041.ERHkCIC5T3 Content-Transfer-Encoding: 7Bit Content-Type: text/html; charset="us-ascii"

Hi,

 

I have encountered a problem with Subversion's Python bindings. For example,

the following simple program does not work:

 

[[[

#!/usr/bin/python

from svn import repos, core

if __name__ == '__main__':

stream = core.Stream(core.svn_stream_open_readonly("dump"))

print stream.read()

stream.close()

]]]

 

$ ./0.py

Traceback (most recent call last):

File "./0.py", line 7, in <module>

print stream.read()

File "/usr/lib/python2.7/dist-packages/svn/core.py", line 212, in read

data = svn_stream_read(self._stream, SVN_STREAM_CHUNK_SIZE)

File "/usr/lib/python2.7/dist-packages/libsvn/core.py", line 5362, in svn_stream_read

return _core.svn_stream_read(*args)

File "/usr/lib/python2.7/dist-packages/libsvn/core.py", line 5585, in assert_valid

assert self.__dict__["_is_valid"](), "Variable has already been deleted"

AssertionError: Variable has already been deleted

 

The problem is that the function to open a stream takes two pool arguments:

result_pool and scratch_pool. In the bindings, the following code is

generated to wrap svn_stream_open_readonly() [formatted for readability]:

 

[[[

if (svn_swig_py_get_pool_arg(args, SWIGTYPE_p_apr_pool_t,

&_global_py_pool, &_global_pool))

SWIG_fail;

arg3 = _global_pool;

if (svn_swig_py_get_pool_arg(args, SWIGTYPE_p_apr_pool_t,

&_global_py_pool, &_global_pool))

SWIG_fail;

arg4 = _global_pool;

...

result = (svn_error_t *)svn_stream_open_readonly(arg1,

(char const *)arg2,arg3,arg4);

...

Py_XDECREF(_global_py_pool);

Py_XDECREF(_global_py_pool);

]]]

 

In case the pool arguments are passed as default (None), each of the calls to

svn_swig_py_get_pool_arg() will create a subpool of the application pool and

assign it to _global_pool. Then, the 2nd subpool will be decremented twice

at the end of the function, while the 1st one will leak.

 

If the caller passes both pool arguments, it is even worse: both arg3 and

arg4 will be initialized by svn_swig_get_pool_arg() to point to the scratch

pool - it always checks the last argument of the function to be a valid pool

and if it is - uses that to initialize the _global_pool:

 

[[[

int argnum = PyTuple_GET_SIZE(args) - 1;

 

if (argnum >= 0)

{

PyObject *input = PyTuple_GET_ITEM(args, argnum);

if (input != Py_None && PyObject_HasAttrString(input, markValid))

{

*pool = svn_swig_py_must_get_ptr(input, type, argnum+1);

]]]

 

Obviously, the caller would expect the result from the wrapped function

to be allocated in the result pool - it would be unexpected to see the

returned variables to disappear when the *scratch* pool is freed.

 

Even if the svn_swig_py_get_pool_arg had recovered both pool pointers

correctly, an attempt to pass two pools would fail in the validity check

in the wrapper function. Obviously, if one passes two distinct pools as

arguments, only one of them can be equal to the _global_pool:

 

[[[

if (obj1) {

/* Verify that the user supplied a valid pool */

if (obj1 != Py_None && obj1 != _global_py_pool) {

SWIG_Python_TypeError(..., obj1);

...

if (obj2) {

/* Verify that the user supplied a valid pool */

if (obj2 != Py_None && obj2 != _global_py_pool) {

SWIG_Python_TypeError(..., obj2);

]]]

 

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?

 

Now, fixing this is somewhat complicated. The typemaps for 'apr_pool_t *'

use %typemap(default) to obtain the object passed in, not the %typemap(in).

There's a reason for this: the _global_pool may be then used to perform

the memory allocations needed to convert other arguments - which, according

to Subversion's coding style, always precede the pool arguments passed last.

For the %typemap(default), however, SWIG does not set the $input macro -

so the $svn_argnum 'hack' cannot be used to determine the argument number

from which the pool should be obtained.

 

The allocations from thus created _global_pool can be either temporary,

needed to convert a Python object into an appropriate C/APR type (e.g.

svn_swig_py_seq_to_array to convert a list to array) or more permanent

(such as svn_swig_py_setup_ra_callbacks - which should be kept for the

lifetime of the returned svn_ra_session_t - the svn_ra_local__open()

stashes the passed in 'callbacks' pointer for later use).

 

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))

 

Additionally, if there are functions that only take a scratch_pool

argument, but still need a permanent pool for some allocations, they

would beed 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 persisten 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.

 

 

Comments/opinions/alternative approaches welcome.

 

Regards,

Alexey.

 

--nextPart1601041.ERHkCIC5T3--