[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1662222 - in /subversion/branches/reuse-ra-session/subversion: include/private/svn_ra_private.h libsvn_client/ra_cache.c libsvn_ra/ra_loader.c

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Wed, 25 Feb 2015 20:07:36 +0300

On 25 February 2015 at 17:40, Branko Čibej <brane_at_wandisco.com> wrote:
> On 25.02.2015 15:23, ivan_at_apache.org wrote:
>> Author: ivan
>> Date: Wed Feb 25 14:23:00 2015
>> New Revision: 1662222
>>
>> URL: http://svn.apache.org/r1662222
>> Log:
>> On the 'reuse-ra-session' branch: Resolve memory leak.
>>
>> * subversion/libsvn_client/ra_cache.c
>> (): Include "svn_pools.h".
>>
>> (cache_entry_t): Add SESSION_POOL member to have an option to destroy
>> CACHE_ENTRY structure itself.
>>
>> (close_ra_session, remove_inactive_entry): Destroy SESSION_POOL
>> instead of closing RA session -- we have other resources allocated
>> for RA session, like RA callback baton, cache_entry etc.
>>
>> (open_new_session): New helper, factored out from
>> svn_client__ra_cache_open_session().
>>
>> (svn_client__ra_cache_open_session): Create separate subpool for
>> each CACHE_ENTRY and destroy it on error.
>
> So ... from this point on, we create *two* subpools for each session
> (svn_ra_open creates one, and with the above change, we create the
> other). That seems wasteful.
>
That's correct. But RA session relatively expensive object (it
maintain TCP connections etc) and one extra subpool should not be big
issue IMO.

> I'd prefer to have a way to tell svn_ra_open to /not/ create a subpool
> for the session, perhaps by exposing a private ra-open function where we
> can pass in the actual session pool, and to keep svn_ra__close.
>
We still need separate subpool to allocate CACHE_ENTRY itself,
callbacks baton etc. And inventing private API to just save one
subpool looks like a bad idea for me.

I'd prefer to drop SESSPOOL in ra-loader.c if we consider that two
subpools per RA session is serious problem: it was added in r984280
when HTTP redirects was implemented. I think that the caller should be
responsible to clear pool if it's going to follow redirects.

-- 
Ivan Zhakov
Received on 2015-02-25 18:09:10 CET

This is an archived mail posted to the Subversion Dev mailing list.