You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2015/02/25 15:23:01 UTC
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
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.
* subversion/include/private/svn_ra_private.h
* subversion/libsvn_ra/ra_loader.c
(svn_ra__close): Remove -- we do not use it now.
Modified:
subversion/branches/reuse-ra-session/subversion/include/private/svn_ra_private.h
subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
subversion/branches/reuse-ra-session/subversion/libsvn_ra/ra_loader.c
Modified: subversion/branches/reuse-ra-session/subversion/include/private/svn_ra_private.h
URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/include/private/svn_ra_private.h?rev=1662222&r1=1662221&r2=1662222&view=diff
==============================================================================
--- subversion/branches/reuse-ra-session/subversion/include/private/svn_ra_private.h (original)
+++ subversion/branches/reuse-ra-session/subversion/include/private/svn_ra_private.h Wed Feb 25 14:23:00 2015
@@ -65,16 +65,6 @@ svn_ra__dup_session(svn_ra_session_t **n
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-/**
- * Close the conneection managed by @a session and destroy the
- * session. When this function returns, the @a session pointer
- * is no longer valid.
- *
- * @since New in 1.9.
- */
-void
-svn_ra__close(svn_ra_session_t *session);
-
/* Equivalent to svn_ra__assert_capable_server()
for SVN_RA_CAPABILITY_MERGEINFO. */
svn_error_t *
Modified: subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c?rev=1662222&r1=1662221&r2=1662222&view=diff
==============================================================================
--- subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c (original)
+++ subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c Wed Feb 25 14:23:00 2015
@@ -28,6 +28,7 @@
#include "svn_dirent_uri.h"
#include "private/svn_ra_private.h"
#include "svn_private_config.h"
+#include "svn_pools.h"
#include "ra_cache.h"
#include "private/svn_debug.h"
@@ -92,6 +93,9 @@ typedef struct cache_entry_t
/* The actual RA session. */
svn_ra_session_t *session;
+ /* POOL for this RA session and cache entry. */
+ apr_pool_t *session_pool;
+
/* POOL that owns this session. NULL if session is not used. */
apr_pool_t *owner_pool;
@@ -409,12 +413,13 @@ close_ra_session(void *data)
--ra_cache->inactive_count;
}
- /* Close and invalidate the session. */
- cache_entry->session = NULL;
- svn_ra__close(session);
-
RA_CACHE_LOG(("SESSION(%d): Closed\n", cache_entry->id));
RA_CACHE_STATS(++ra_cache->stat.close);
+
+ /* Close and invalidate the session. */
+ svn_pool_destroy(cache_entry->session_pool);
+ cache_entry->session = NULL;
+ cache_entry->session_pool = NULL;
}
else
{
@@ -438,16 +443,10 @@ remove_inactive_entry(svn_client__ra_cac
cache_entry_t *cache_entry,
svn_boolean_t expired)
{
- svn_ra_session_t *const session = cache_entry->session;
-
APR_RING_REMOVE(cache_entry, freelist);
APR_RING_ELEM_INIT(cache_entry, freelist);
--ra_cache->inactive_count;
- /* Close and invalidate the session. */
- cache_entry->session = NULL;
- svn_ra__close(session);
-
RA_CACHE_LOG(("SESSION(%d): Closed (%s)\n", cache_entry->id,
(expired ? "expired" : "expunged")));
RA_CACHE_STATS(
@@ -455,6 +454,11 @@ remove_inactive_entry(svn_client__ra_cac
++ra_cache->stat.expire;
else
++ra_cache->stat.expunge);
+
+ /* Close and invalidate the session. */
+ svn_pool_destroy(cache_entry->session_pool);
+ cache_entry->session = NULL;
+ cache_entry->session_pool = NULL;
}
/* Limit the size of the inactive session list in RA_CACHE and remove
@@ -530,6 +534,71 @@ find_session_by_url(cache_entry_t **cach
return SVN_NO_ERROR;
}
+/* Helper for svn_client__ra_cache_open_session(). Allocates new
+ *CACHE_ENTRY_P and opens new RA session using BASE_URL, UUID,
+ CBTABLE and CALLBACK_BATON arguments. RA session and CACHE_ENTRY
+ will be allocated from SESSION_POOL and linked lifetime of
+ OWNER_POOL. */
+static svn_error_t *
+open_new_session(cache_entry_t **cache_entry_p,
+ const char **corrected_p,
+ svn_client__ra_cache_t *ra_cache,
+ const char *base_url,
+ const char *uuid,
+ svn_ra_callbacks2_t *cbtable,
+ void *callback_baton,
+ apr_pool_t *session_pool,
+ apr_pool_t *owner_pool)
+{
+ cache_entry_t *cache_entry;
+ svn_ra_callbacks2_t *ra_callbacks;
+ svn_ra_session_t *session;
+
+ cache_entry = apr_pcalloc(session_pool, sizeof(*cache_entry));
+ APR_RING_ELEM_INIT(cache_entry, freelist);
+ cache_entry->session_pool = session_pool;
+
+ SVN_ERR(svn_ra_create_callbacks(&ra_callbacks, session_pool));
+ ra_callbacks->open_tmp_file = open_tmp_file;
+ ra_callbacks->get_wc_prop = get_wc_prop;
+ ra_callbacks->set_wc_prop = set_wc_prop;
+ ra_callbacks->push_wc_prop = push_wc_prop;
+ ra_callbacks->invalidate_wc_props = invalidate_wc_props;
+ ra_callbacks->auth_baton = cbtable->auth_baton; /* new-style */
+ ra_callbacks->progress_func = progress_func;
+ ra_callbacks->progress_baton = cache_entry;
+ ra_callbacks->cancel_func = cancel_func;
+ ra_callbacks->get_client_string = get_client_string;
+ ra_callbacks->get_wc_contents = get_wc_contents;
+ ra_callbacks->check_tunnel_func = check_tunnel_func;
+ ra_callbacks->open_tunnel_func = open_tunnel_func;
+ ra_callbacks->tunnel_baton = cache_entry;
+
+ cache_entry->owner_pool = owner_pool;
+ cache_entry->cb_table = cbtable;
+ cache_entry->cb_baton = callback_baton;
+ cache_entry->id = ra_cache->next_id;
+
+ SVN_ERR(svn_ra_open4(&session, corrected_p, base_url, uuid,
+ ra_callbacks, cache_entry,
+ ra_cache->config, session_pool));
+
+ if (corrected_p && *corrected_p)
+ {
+ /* Caller is ready to follow redirection and we got redirection.
+ Just return corrected URL without RA session. */
+ return SVN_NO_ERROR;
+ }
+
+ cache_entry->session = session;
+
+ SVN_ERR(svn_ra_get_repos_root2(session, &cache_entry->root_url,
+ session_pool));
+
+ *cache_entry_p = cache_entry;
+ return SVN_NO_ERROR;
+}
+
/* Convert a public client context pointer to a pointer to the RA
session cache in the private client context. */
static svn_client__ra_cache_t *
@@ -604,49 +673,30 @@ svn_client__ra_cache_open_session(svn_ra
else
{
/* No existing RA session found. Open new one. */
- svn_ra_callbacks2_t *ra_callbacks;
- svn_ra_session_t *session;
-
- cache_entry = apr_pcalloc(ra_cache->pool, sizeof(*cache_entry));
- APR_RING_ELEM_INIT(cache_entry, freelist);
-
- SVN_ERR(svn_ra_create_callbacks(&ra_callbacks, ra_cache->pool));
- ra_callbacks->open_tmp_file = open_tmp_file;
- ra_callbacks->get_wc_prop = get_wc_prop;
- ra_callbacks->set_wc_prop = set_wc_prop;
- ra_callbacks->push_wc_prop = push_wc_prop;
- ra_callbacks->invalidate_wc_props = invalidate_wc_props;
- ra_callbacks->auth_baton = cbtable->auth_baton; /* new-style */
- ra_callbacks->progress_func = progress_func;
- ra_callbacks->progress_baton = cache_entry;
- ra_callbacks->cancel_func = cancel_func;
- ra_callbacks->get_client_string = get_client_string;
- ra_callbacks->get_wc_contents = get_wc_contents;
- ra_callbacks->check_tunnel_func = check_tunnel_func;
- ra_callbacks->open_tunnel_func = open_tunnel_func;
- ra_callbacks->tunnel_baton = cache_entry;
-
- cache_entry->owner_pool = result_pool;
- cache_entry->cb_table = cbtable;
- cache_entry->cb_baton = callback_baton;
- cache_entry->id = ra_cache->next_id;
+ svn_error_t *err;
+ apr_pool_t *session_pool = svn_pool_create(ra_cache->pool);
- SVN_ERR(svn_ra_open4(&session, corrected_p, base_url, uuid,
- ra_callbacks, cache_entry,
- ra_cache->config, ra_cache->pool));
+ err = open_new_session(&cache_entry, corrected_p, ra_cache,
+ base_url, uuid, cbtable, callback_baton,
+ session_pool, result_pool);
+ if (err)
+ {
+ svn_pool_destroy(session_pool);
+ return svn_error_trace(err);
+ }
if (corrected_p && *corrected_p)
{
+ /* Copy corrected url to RESULT_POOL before cleaning
+ SESSION_POOL. */
+ *corrected_p = apr_pstrdup(result_pool, *corrected_p);
+
+ svn_pool_destroy(session_pool);
/* Caller is ready to follow redirection and we got redirection.
Just return corrected URL without RA session. */
return SVN_NO_ERROR;
}
- cache_entry->session = session;
-
- SVN_ERR(svn_ra_get_repos_root2(session, &cache_entry->root_url,
- ra_cache->pool));
-
RA_CACHE_LOG(("SESSION(%d): Open('%s')\n", cache_entry->id, base_url));
RA_CACHE_STATS(++ra_cache->stat.open);
Modified: subversion/branches/reuse-ra-session/subversion/libsvn_ra/ra_loader.c
URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/libsvn_ra/ra_loader.c?rev=1662222&r1=1662221&r2=1662222&view=diff
==============================================================================
--- subversion/branches/reuse-ra-session/subversion/libsvn_ra/ra_loader.c (original)
+++ subversion/branches/reuse-ra-session/subversion/libsvn_ra/ra_loader.c Wed Feb 25 14:23:00 2015
@@ -228,14 +228,6 @@ check_ra_version(const svn_version_t *ra
/* -------------------------------------------------------------- */
-/*** Private Inter-Library Interfaces ***/
-void
-svn_ra__close(svn_ra_session_t *session)
-{
- if (session && session->pool)
- apr_pool_destroy(session->pool);
-}
-
/* -------------------------------------------------------------- */
/*** Public Interfaces ***/
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
Posted by Branko Čibej <br...@wandisco.com>.
On 25.02.2015 18:07, Ivan Zhakov wrote:
> On 25 February 2015 at 17:40, Branko Čibej <br...@wandisco.com> wrote:
>> On 25.02.2015 15:23, ivan@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.
Since r1664078 and related changes, the session really does need its own
pool. Those changes also reduced the overhead allocation from the
session pool itself, using a scratch pool for the temporary bits that's
destroyed before the new session is returned to the caller.
-- Brane
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
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 February 2015 at 17:40, Branko Čibej <br...@wandisco.com> wrote:
> On 25.02.2015 15:23, ivan@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
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
Posted by Branko Čibej <br...@wandisco.com>.
On 25.02.2015 15:23, ivan@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.
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.
-- Brane