Reuse of ra_dav sessions considered harmful (Was Re: svn trunk r17972: FAIL (x86_64-unknown-linux-gnu shared ra_dav fsfs))

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/4/06, svntest@jaa.iki.fi <sv...@jaa.iki.fi> wrote:

> At least one test FAILED, checking /svntest/obj-sh/tests.log
> FAIL:  svnsync_tests.py 1: copy and modify
> FAIL:  svnsync_tests.py 2: copy from previous version and modify
> FAIL:  svnsync_tests.py 3: copy from previous version
> FAIL:  svnsync_tests.py 5: tag empty trunk
> FAIL:  svnsync_tests.py 7: tag trunk containing a file (#2)
> FAIL:  svnsync_tests.py 8: tag trunk containing a file
> FAIL:  svnsync_tests.py 9: tag with a modified file
> At least one test was SKIPPED, checking /svntest/obj-sh/tests.log

Ok, so it turns out that these are caused by a problem in ra_dav.  If
you create an RA session in one pool (say, for example, the parent
pool), then try  to run commits in a loop using a subpool to hold the
commit editors (say, just like svnsync tries to do) you will crash,
because svn_ra_dav__get_commit_editor uses ne_hook_create_request and
ne_hook_pre_send to create two hooks, which use batons allocated out
of the subpool.  These hooks never go away (and in fact I can't find
any API that even lets you make them go away in the current version of
neon), so the next time through the loop you try to call them again,
except that the baton is now pointing off into memory that's been
reused, and you crash.

I don't see a good way to correct this at this point.  We can't (as it
stands) safely reuse that neon session, because there's no way to
remove its hooks.  The alternatives I can think of either involve
making the hook smart enough to tell if their batons are still good
(they store pointers into a longer lived data structure that holds the
pointer to their real batons, and they check if those are NULL before
doing anything), but that's ugly.  Or we can recreate the session each
time through, I haven't looked to see how much of a pain that will be
to handle in ra_dav.  Or I make svnsync reopen its connection each
time through the loop, which sucks because it makes all the other ra
layers pay for ra_dav's sucking.

On top of that there may be another problem lurking underneath this,
since when I force the issue by allocating the commit editor in the
parent pool I get other strange problems, but I still haven't really
debugged that yet.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Reuse of ra_dav sessions considered harmful

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/4/06, Philip Martin <ph...@codematters.co.uk> wrote:
> Garrett Rooney <ro...@electricjellyfish.net> writes:
>
> > because svn_ra_dav__get_commit_editor uses ne_hook_create_request and
> > ne_hook_pre_send to create two hooks, which use batons allocated out
> > of the subpool.  These hooks never go away (and in fact I can't find
> > any API that even lets you make them go away in the current version of
> > neon), so the next time through the loop you try to call them again,
> > except that the baton is now pointing off into memory that's been
> > reused, and you crash.
>
> I've seen something like that before, see setup_neon_request_hook and
> r14295.  Perhaps you could use a similar approach?

That seems like about the best we can do with the current APIs.  One
thing that occurs to me about the way the lock code is doing this
though, perhaps it should be setting lrb->pool to the current pool, as
opposed to the session pool, so as to avoid leaking memory inside the
hook callbacks.

Thanks for the pointer!

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Reuse of ra_dav sessions considered harmful

[Philip Martin <ph...@codematters.co.uk>]

Garrett Rooney <ro...@electricjellyfish.net> writes:

> because svn_ra_dav__get_commit_editor uses ne_hook_create_request and
> ne_hook_pre_send to create two hooks, which use batons allocated out
> of the subpool.  These hooks never go away (and in fact I can't find
> any API that even lets you make them go away in the current version of
> neon), so the next time through the loop you try to call them again,
> except that the baton is now pointing off into memory that's been
> reused, and you crash.

I've seen something like that before, see setup_neon_request_hook and
r14295.  Perhaps you could use a similar approach?

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org