You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ka...@apache.org on 2011/05/13 14:22:16 UTC
svn commit: r1102690 - in /subversion/trunk/subversion:
libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py
Author: kameshj
Date: Fri May 13 12:22:15 2011
New Revision: 1102690
URL: http://svn.apache.org/viewvc?rev=1102690&view=rev
Log:
Fix for issue3870 "File descriptor leaks during svnsync".
Before this fix, all of destination delta editor's interfaces
are called with *LONG* living pool(dst_rev_pool living for one full revision).
This makes it a memory bloat and bloat of other OS resources like
file descriptors to live as long the dst_rev_pool.
* subversion/libsvn_ra_serf/replay.c
(replay_context_t.file_pool): New pool of file scope.
(start_replay): clear the file_pool.
Use file_pool for dest editor's file operations.
(end_replay): Use file_pool for dest editor's file operations.
(svn_ra_serf__replay, svn_ra_serf__replay_range):
Create a new pool 'replay_ctx->file_pool'.
* subversion/tests/cmdline/svnsync_tests.py
(fd_leak_sync_from_serf_to_local): Remove XFail marker.
Patch by: kameshj
Arwin Arni <arwin{_AT_}collab.net>
Modified:
subversion/trunk/subversion/libsvn_ra_serf/replay.c
subversion/trunk/subversion/tests/cmdline/svnsync_tests.py
Modified: subversion/trunk/subversion/libsvn_ra_serf/replay.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/replay.c?rev=1102690&r1=1102689&r2=1102690&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/replay.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/replay.c Fri May 13 12:22:15 2011
@@ -93,6 +93,8 @@ typedef struct prop_info_t {
typedef struct replay_context_t {
apr_pool_t *src_rev_pool;
apr_pool_t *dst_rev_pool;
+ /*file_pool is cleared after completion of each file. */
+ apr_pool_t *file_pool;
/* Are we done fetching this file? */
svn_boolean_t done;
@@ -336,6 +338,7 @@ start_replay(svn_ra_serf__xml_parser_t *
const char *file_name, *rev;
replay_info_t *info;
+ svn_pool_clear(ctx->file_pool);
file_name = svn_xml_get_attr_value("name", attrs);
if (!file_name)
{
@@ -353,7 +356,7 @@ start_replay(svn_ra_serf__xml_parser_t *
SVN_ERR(ctx->editor->open_file(file_name, info->parent->baton,
SVN_STR_TO_REV(rev),
- ctx->dst_rev_pool, &info->baton));
+ ctx->file_pool, &info->baton));
}
else if ((state == OPEN_DIR || state == ADD_DIR) &&
strcmp(name.name, "add-file") == 0)
@@ -362,6 +365,7 @@ start_replay(svn_ra_serf__xml_parser_t *
svn_revnum_t rev;
replay_info_t *info;
+ svn_pool_clear(ctx->file_pool);
file_name = svn_xml_get_attr_value("name", attrs);
if (!file_name)
{
@@ -380,7 +384,7 @@ start_replay(svn_ra_serf__xml_parser_t *
SVN_ERR(ctx->editor->add_file(file_name, info->parent->baton,
copyfrom, rev,
- ctx->dst_rev_pool, &info->baton));
+ ctx->file_pool, &info->baton));
}
else if ((state == OPEN_FILE || state == ADD_FILE) &&
strcmp(name.name, "apply-textdelta") == 0)
@@ -400,7 +404,7 @@ start_replay(svn_ra_serf__xml_parser_t *
}
SVN_ERR(ctx->editor->apply_textdelta(info->baton, checksum,
- info->pool,
+ ctx->file_pool,
&textdelta,
&textdelta_baton));
@@ -417,7 +421,7 @@ start_replay(svn_ra_serf__xml_parser_t *
checksum = svn_xml_get_attr_value("checksum", attrs);
SVN_ERR(ctx->editor->close_file(info->baton, checksum,
- ctx->dst_rev_pool));
+ ctx->file_pool));
svn_ra_serf__xml_pop_state(parser);
}
@@ -439,7 +443,6 @@ start_replay(svn_ra_serf__xml_parser_t *
info = push_state(parser, ctx, CHANGE_PROP);
- info->name = apr_pstrdup(ctx->dst_rev_pool, prop_name);
if (svn_xml_get_attr_value("del", attrs))
info->del_prop = TRUE;
@@ -447,9 +450,15 @@ start_replay(svn_ra_serf__xml_parser_t *
info->del_prop = FALSE;
if (state == OPEN_FILE || state == ADD_FILE)
- info->change = ctx->editor->change_file_prop;
+ {
+ info->name = apr_pstrdup(ctx->file_pool, prop_name);
+ info->change = ctx->editor->change_file_prop;
+ }
else
- info->change = ctx->editor->change_dir_prop;
+ {
+ info->name = apr_pstrdup(ctx->dst_rev_pool, prop_name);
+ info->change = ctx->editor->change_dir_prop;
+ }
}
@@ -527,7 +536,10 @@ end_replay(svn_ra_serf__xml_parser_t *pa
tmp_prop.data = info->data;
tmp_prop.len = info->len;
- prop_val = svn_base64_decode_string(&tmp_prop, ctx->dst_rev_pool);
+ if (strcmp(name.name, "change-file-prop") == 0)
+ prop_val = svn_base64_decode_string(&tmp_prop, ctx->file_pool);
+ else
+ prop_val = svn_base64_decode_string(&tmp_prop, ctx->dst_rev_pool);
}
SVN_ERR(info->change(info->parent->baton, info->name, prop_val,
@@ -635,6 +647,7 @@ svn_ra_serf__replay(svn_ra_session_t *ra
replay_ctx = apr_pcalloc(pool, sizeof(*replay_ctx));
replay_ctx->src_rev_pool = pool;
+ replay_ctx->file_pool = svn_pool_create(pool);
replay_ctx->editor = editor;
replay_ctx->editor_baton = edit_baton;
replay_ctx->done = FALSE;
@@ -751,6 +764,7 @@ svn_ra_serf__replay_range(svn_ra_session
replay_ctx = apr_pcalloc(ctx_pool, sizeof(*replay_ctx));
replay_ctx->src_rev_pool = ctx_pool;
+ replay_ctx->file_pool = svn_pool_create(pool);
replay_ctx->revstart_func = revstart_func;
replay_ctx->revfinish_func = revfinish_func;
replay_ctx->replay_baton = replay_baton;
Modified: subversion/trunk/subversion/tests/cmdline/svnsync_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svnsync_tests.py?rev=1102690&r1=1102689&r2=1102690&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svnsync_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svnsync_tests.py Fri May 13 12:22:15 2011
@@ -946,7 +946,6 @@ def delete_revprops(sbox):
@Issue(3870)
@SkipUnless(svntest.main.is_posix_os)
-@XFail(svntest.main.is_ra_type_dav_serf)
def fd_leak_sync_from_serf_to_local(sbox):
"fd leak during sync from serf to local"
import resource
Re: svn commit: r1102690 - in /subversion/trunk/subversion:
libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Fri, May 13, 2011 at 7:53 AM, Kamesh Jayachandran <ka...@collab.net> wrote:
> As per delta editor contract... one can not open two files at once on the
> same editor drive, so no issue.
Don't get C-Mike started...but, no ra_serf does violate that one. =) -- justin
Re: svn commit: r1102690 - in /subversion/trunk/subversion: libsvn_ra_serf/replay.c
tests/cmdline/svnsync_tests.py
Posted by Kamesh Jayachandran <ka...@collab.net>.
On 05/13/2011 07:51 PM, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: kameshj@apache.org [mailto:kameshj@apache.org]
>> Sent: vrijdag 13 mei 2011 14:22
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1102690 - in /subversion/trunk/subversion:
>> libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py
>>
>> Author: kameshj
>> Date: Fri May 13 12:22:15 2011
>> New Revision: 1102690
>>
>> URL: http://svn.apache.org/viewvc?rev=1102690&view=rev
>> Log:
>> Fix for issue3870 "File descriptor leaks during svnsync".
>>
>> Before this fix, all of destination delta editor's interfaces
>> are called with *LONG* living pool(dst_rev_pool living for one full revision).
>>
>> This makes it a memory bloat and bloat of other OS resources like
>> file descriptors to live as long the dst_rev_pool.
> I'm pretty sure this fixes this issue, but shouldn't the file pool live inside a revision pool? (Or maybe in a directory pool, that lives in, ... etc. etc.)
That is how it is in neon.
Current State of serf's *state* handling do not have room for that
hierarchy or revision->directory->file.
I thought to implement similar hierarchy as a part of this patch.
But having closely looked at the neon's code with respect to the file
handling portions of editor drive, I felt simple file_pool that gets
cleared upon before starting a new file seemed a simpler approach.
Anyways as I stated in the original mail... Similar problems could
remain for directories having huge number of sub directories.... anyway
I will write a testcase and fix on a permanent problem.
> Without looking at the code I would have expected that serf might operate on multiple files at once. (But I assume our test suite would have caught that by now)
>
> Bert
>
As per delta editor contract... one can not open two files at once on
the same editor drive, so no issue.
With regards
Kamesh Jayachandran
RE: svn commit: r1102690 - in /subversion/trunk/subversion: libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: kameshj@apache.org [mailto:kameshj@apache.org]
> Sent: vrijdag 13 mei 2011 14:22
> To: commits@subversion.apache.org
> Subject: svn commit: r1102690 - in /subversion/trunk/subversion:
> libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py
>
> Author: kameshj
> Date: Fri May 13 12:22:15 2011
> New Revision: 1102690
>
> URL: http://svn.apache.org/viewvc?rev=1102690&view=rev
> Log:
> Fix for issue3870 "File descriptor leaks during svnsync".
>
> Before this fix, all of destination delta editor's interfaces
> are called with *LONG* living pool(dst_rev_pool living for one full revision).
>
> This makes it a memory bloat and bloat of other OS resources like
> file descriptors to live as long the dst_rev_pool.
I'm pretty sure this fixes this issue, but shouldn't the file pool live inside a revision pool? (Or maybe in a directory pool, that lives in, ... etc. etc.)
Without looking at the code I would have expected that serf might operate on multiple files at once. (But I assume our test suite would have caught that by now)
Bert