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