Issue 2732 (was Unhandled exception when stopping apache)

["D.J. Heap" <dj...@gmail.com>]

On 3/24/07, D.J. Heap <dj...@gmail.com> wrote:
[snip]
>
> Thanks, it looks good.  It appears to be a shutdown/cleanup ordering
> problem in the bdb stuff after a quick first glance.  I know there was
> a lot of pain and work on that issue a while ago -- should be fun to
> figure out.  I'll try to work on it as soon as I can but it's not very
> high on my list at the moment.


I've been debugging this and narrowed it down quite a bit.  I'm
puzzled how it works on other OS' at all, though, so I think I'm
mis-understanding something.

In libsvn_fs_base\bdb\env.c the bdb_init_cb function allocates the bdb
cache stuff in the global pool (I think?) and registers a cleanup
callback to clear_cache.

Then in final process termination (after unloading modules) when
Apache cleans out the global pool
(apr\memory\unix\apr_pools.c:apr_pool_terminate), it tries to call the
clear_cache function -- but it's already been unloaded by then and so
goes boom.

Am I mis-understanding the sequence of things or does it work
differently on unixy OS'?

The crash goes away if I comment out the clear_cache cleanup registration.

DJ

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

Re: Issue 2732 (was Unhandled exception when stopping apache)

["D.J. Heap" <dj...@gmail.com>]

On 4/1/07, D.J. Heap <dj...@gmail.com> wrote:
[snip]
> I've attached a thrown-together-in-5-mins minmal patch (not
> style-formatted correctly, no doc updates, didn't change the init
> function names, etc) that appears to resolve the issue from inital
> testing.  Does the basic approach look viable?  If so, I'll refine it
> and post it for more thorough review.
>


That patch had one issue dealing with initting common_pool (it needs
to be initted earlier than it was in the 'best effort' case) -- it's
fixed in this version (formatting, docs, etc still not done, though).

DJ

Re: Issue 2732 (was Unhandled exception when stopping apache)

["D.J. Heap" <dj...@gmail.com>]

On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
[snip]
> > If my understanding is correct, svn_fs_bdb__init is used to init the
> > whole module, and base_serialized_init is used to init an svn_fs_t.
> > It seems like that is still early enough to init the cache, though,
> > isn't it?
>
> Nope ...
>
> You need an open BDB environment before you can create an svn_fs_t, and
> you need an environment cache before you can safely open an environment.
>
> Why d'you think I had all the migraines when I wrote the initial version
> of that cache? :)


Ah, yes, I see it is called *after* the open/create.

So, we need to rev svn_fs_*__init to take another fs-global-pool
parameter and change the loader to use it.  We'd probably have to
change the function name or something so old libraries don't get
called with unexpected parameters.  They aren't public symbols and our
version checking doesn't allow using old modules anyway, so just
changing the name may be sufficient?

This is assuming the client has called svn_fs_initialize -- which
seems to me like it is already basically a requirement.  The docs say
it will just leak, but actually it invokes 'undefined' behavior which
appears to do nothing sometimes but crashes at other times.

Any other ideas on a simpler way to fix this?

I've attached a thrown-together-in-5-mins minmal patch (not
style-formatted correctly, no doc updates, didn't change the init
function names, etc) that appears to resolve the issue from inital
testing.  Does the basic approach look viable?  If so, I'll refine it
and post it for more thorough review.

DJ

Re: Issue 2732 (was Unhandled exception when stopping apache)

[Branko Čibej <br...@xbc.nu>]

D.J. Heap wrote:
> On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
> [snip]
>>
>> It would require a libsvn_fs initialization function that the all FS
>> users are required to call ... which requires a change of contract that
>> we can't enforce in 1.x. That's the main reason why the BDB pool init
>> and teardown code is so horribly convoluted. :\
>>
>
>
> Yeah...it looks like it's already 'encouraged' if not required,
> though, and then we do a 'best-effort' if we detect that it hasn't
> been called.
>
> Hmm, maybe there's a much simpler answer:
>
> Move the cache initting code from svn_fs_bdb__init to
> base_serialized_init where we already have the common_pool as a
> parameter.
>
> If my understanding is correct, svn_fs_bdb__init is used to init the
> whole module, and base_serialized_init is used to init an svn_fs_t.
> It seems like that is still early enough to init the cache, though,
> isn't it?

Nope ...

You need an open BDB environment before you can create an svn_fs_t, and
you need an environment cache before you can safely open an environment.

Why d'you think I had all the migraines when I wrote the initial version
of that cache? :)

-- Brane

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

Re: Issue 2732 (was Unhandled exception when stopping apache)

["D.J. Heap" <dj...@gmail.com>]

On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
[snip]
>
> It would require a libsvn_fs initialization function that the all FS
> users are required to call ... which requires a change of contract that
> we can't enforce in 1.x. That's the main reason why the BDB pool init
> and teardown code is so horribly convoluted. :\
>


Yeah...it looks like it's already 'encouraged' if not required,
though, and then we do a 'best-effort' if we detect that it hasn't
been called.

Hmm, maybe there's a much simpler answer:

Move the cache initting code from svn_fs_bdb__init to
base_serialized_init where we already have the common_pool as a
parameter.

If my understanding is correct, svn_fs_bdb__init is used to init the
whole module, and base_serialized_init is used to init an svn_fs_t.
It seems like that is still early enough to init the cache, though,
isn't it?

DJ

Re: Issue 2732 (was Unhandled exception when stopping apache)

[Branko Čibej <br...@xbc.nu>]

D.J. Heap wrote:
> On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
> [snip]
>> I suppose we could, if such hooks are available -- but then we'd have to
>> propagate that pool somehow to the libsvn_fs_bdb cache.
>
>
> I *think* they are there, and the common_pool already uses them from
> what I can tell -- in libsvn_fs\fs-loader.c, svn_fs_initialize and
> uninit seem to get called at the times I would expect.
>
> Propagating it to the bdb cache is the part that I'm not sure how to
> do.  Would it require a new vtable entry (init2 that takes a pool) and
> then checking the module version to make sure it implemented that
> entry...or something like that?

It would require a libsvn_fs initialization function that the all FS
users are required to call ... which requires a change of contract that
we can't enforce in 1.x. That's the main reason why the BDB pool init
and teardown code is so horribly convoluted. :\

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

Re: Issue 2732 (was Unhandled exception when stopping apache)

["D.J. Heap" <dj...@gmail.com>]

On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
[snip]
> I suppose we could, if such hooks are available -- but then we'd have to
> propagate that pool somehow to the libsvn_fs_bdb cache.


I *think* they are there, and the common_pool already uses them from
what I can tell -- in libsvn_fs\fs-loader.c, svn_fs_initialize and
uninit seem to get called at the times I would expect.

Propagating it to the bdb cache is the part that I'm not sure how to
do.  Would it require a new vtable entry (init2 that takes a pool) and
then checking the module version to make sure it implemented that
entry...or something like that?

DJ

Re: Issue 2732 (was Unhandled exception when stopping apache)

[Branko Čibej <br...@xbc.nu>]

D.J. Heap wrote:
> On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
> [snip]
>> >
>> > I'm not deadly on the details, but is that pretty much the only way to
>> > actually fix this?   We have a common_pool in the loader that appears
>> > to be trying to address this same kind of issue -- is there some way
>> > to pass it down so the bdb cache can use that?  Or is that pool
>> > inappropriate for some reason?
>>
>> In your case, we don't control the pool that "owns" mod_dav_svn.so --
>> HTTPD does. I suppose we could try changing the way HTTPD handles module
>> unloading.
>
>
> Right, but we know when mod_dav_svn.so is loaded and unloaded don't
> we?  Couldn't we make our own global (or child) pool at init and clean
> it up in uninit?  I thought I saw the requisite entry points in
> mod_dav_svn.so, but maybe I'm mistaken.

I suppose we could, if such hooks are available -- but then we'd have to
propagate that pool somehow to the libsvn_fs_bdb cache.

-- Brane

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

Re: Issue 2732 (was Unhandled exception when stopping apache)

["D.J. Heap" <dj...@gmail.com>]

On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
[snip]
> >
> > I'm not deadly on the details, but is that pretty much the only way to
> > actually fix this?   We have a common_pool in the loader that appears
> > to be trying to address this same kind of issue -- is there some way
> > to pass it down so the bdb cache can use that?  Or is that pool
> > inappropriate for some reason?
>
> In your case, we don't control the pool that "owns" mod_dav_svn.so --
> HTTPD does. I suppose we could try changing the way HTTPD handles module
> unloading.


Right, but we know when mod_dav_svn.so is loaded and unloaded don't
we?  Couldn't we make our own global (or child) pool at init and clean
it up in uninit?  I thought I saw the requisite entry points in
mod_dav_svn.so, but maybe I'm mistaken.

DJ

Re: Issue 2732 (was Unhandled exception when stopping apache)

[Branko Čibej <br...@xbc.nu>]

D.J. Heap wrote:
> On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
> [snip]
>>
>> I think we decided once that this was a bug in DSO handling and really
>> has to be fixed in APR -- or at least, that APR should give apps a way
>> to delay the actual module unload until after pool cleanup. And I can't
>> imagine how it would work on other platforms, either. Unless it's
>> (again) about "freed" memory still having the original contents ... I
>> wouldn't be at all surprised if a DSOs memory remained mapped to the
>> proces some indefinite time after unload, on some systems (it would
>> scare me, though :).
>>
>
>
> Ah, I've seen that kind of thing before and that would explain it.
>
> I'm not deadly on the details, but is that pretty much the only way to
> actually fix this?   We have a common_pool in the loader that appears
> to be trying to address this same kind of issue -- is there some way
> to pass it down so the bdb cache can use that?  Or is that pool
> inappropriate for some reason?

In your case, we don't control the pool that "owns" mod_dav_svn.so --
HTTPD does. I suppose we could try changing the way HTTPD handles module
unloading.

-- Brane

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

Re: Issue 2732 (was Unhandled exception when stopping apache)

["D.J. Heap" <dj...@gmail.com>]

On 4/1/07, Branko Čibej <br...@xbc.nu> wrote:
[snip]
>
> I think we decided once that this was a bug in DSO handling and really
> has to be fixed in APR -- or at least, that APR should give apps a way
> to delay the actual module unload until after pool cleanup. And I can't
> imagine how it would work on other platforms, either. Unless it's
> (again) about "freed" memory still having the original contents ... I
> wouldn't be at all surprised if a DSOs memory remained mapped to the
> proces some indefinite time after unload, on some systems (it would
> scare me, though :).
>


Ah, I've seen that kind of thing before and that would explain it.

I'm not deadly on the details, but is that pretty much the only way to
actually fix this?   We have a common_pool in the loader that appears
to be trying to address this same kind of issue -- is there some way
to pass it down so the bdb cache can use that?  Or is that pool
inappropriate for some reason?

DJ

Re: Issue 2732 (was Unhandled exception when stopping apache)

[Branko Čibej <br...@xbc.nu>]

D.J. Heap wrote:
> On 3/24/07, D.J. Heap <dj...@gmail.com> wrote:
> [snip]
>>
>> Thanks, it looks good.  It appears to be a shutdown/cleanup ordering
>> problem in the bdb stuff after a quick first glance.  I know there was
>> a lot of pain and work on that issue a while ago -- should be fun to
>> figure out.  I'll try to work on it as soon as I can but it's not very
>> high on my list at the moment.
>
>
> I've been debugging this and narrowed it down quite a bit.  I'm
> puzzled how it works on other OS' at all, though, so I think I'm
> mis-understanding something.
>
> In libsvn_fs_base\bdb\env.c the bdb_init_cb function allocates the bdb
> cache stuff in the global pool (I think?) and registers a cleanup
> callback to clear_cache.
>
> Then in final process termination (after unloading modules) when
> Apache cleans out the global pool
> (apr\memory\unix\apr_pools.c:apr_pool_terminate), it tries to call the
> clear_cache function -- but it's already been unloaded by then and so
> goes boom.
>
> Am I mis-understanding the sequence of things or does it work
> differently on unixy OS'?
>
> The crash goes away if I comment out the clear_cache cleanup registration.

I think we decided once that this was a bug in DSO handling and really
has to be fixed in APR -- or at least, that APR should give apps a way
to delay the actual module unload until after pool cleanup. And I can't
imagine how it would work on other platforms, either. Unless it's
(again) about "freed" memory still having the original contents ... I
wouldn't be at all surprised if a DSOs memory remained mapped to the
proces some indefinite time after unload, on some systems (it would
scare me, though :).

-- Brane

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