You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by mt...@apache.org on 2008/07/17 07:45:57 UTC
svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Author: mturk
Date: Wed Jul 16 22:45:57 2008
New Revision: 677505
URL: http://svn.apache.org/viewvc?rev=677505&view=rev
Log:
Use apr_pool_pre_cleanup_register for reslist cleanup
Modified:
apr/apr-util/trunk/misc/apr_reslist.c
Modified: apr/apr-util/trunk/misc/apr_reslist.c
URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/misc/apr_reslist.c?rev=677505&r1=677504&r2=677505&view=diff
==============================================================================
--- apr/apr-util/trunk/misc/apr_reslist.c (original)
+++ apr/apr-util/trunk/misc/apr_reslist.c Wed Jul 16 22:45:57 2008
@@ -277,8 +277,15 @@
return rv;
}
- apr_pool_cleanup_register(rl->pool, rl, reslist_cleanup,
- apr_pool_cleanup_null);
+ /* Register a pool pre_cleanup.
+ * This will ensure that reslist_cleanup is run BEFORE
+ * any of the eventual child pools of this pool.
+ * If an child pool was created inside apr_reslist_destructor,
+ * this child pool can safely call apr_pool_destroy inside
+ * apr_reslist_destructor thus safely detaching himself.
+ */
+ apr_pool_pre_cleanup_register(rl->pool, rl, reslist_cleanup,
+ apr_pool_cleanup_null);
*reslist = rl;
Re: reslist API future (was Re: svn commit: r677505)
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Joe Orton" <jo...@redhat.com>:
> I'd rather the decision was made by those who are more familiar with
> this code than I. The fact that this change apparently does break an
> existing caller does seem to back up the "API change" argument:
> http://markmail.org/message/xjgaampnoru67pxr
I wasn't aware of that one, but yeah, if it things like this a
possible, we should most definitely not backport. (I'm guessing the
failure here is not related the fix in pre_cleanup implementation
(r678139), given very recent dates in the mail thread).
> I think that makes sense, so long as you're sure that the respool design
> will solve the set of problems with reslist that people have seen; it
> would be good to hear from Mladen and Chris on this.
Not sure at all. That's why I put your ideas into a patch, so that
everyone can test :-)
--
Bojan
reslist API future (was Re: svn commit: r677505)
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jul 29, 2008 at 09:44:38AM +1000, Bojan Smojver wrote:
> On Mon, 2008-07-28 at 23:51 +0100, Joe Orton wrote:
> > So I'd say it's either a new API guarantee (can't be done in 1.3.x) or
> > it's simply an API change (can't be done in 1.x).
>
> Is that your -1 to backporting?
I'd rather the decision was made by those who are more familiar with
this code than I. The fact that this change apparently does break an
existing caller does seem to back up the "API change" argument:
http://markmail.org/message/xjgaampnoru67pxr
> Going forward (1.4.x and above), we could also simply include respool
> (patch sent to the list), that implements sub-pools by default and it
> doesn't have a destructor, along the lines of what you explained wrt.
> "design pattern". Then people can continue using reslist as is and use
> respool for new stuff, without any API/ABI breakage.
I think that makes sense, so long as you're sure that the respool design
will solve the set of problems with reslist that people have seen; it
would be good to hear from Mladen and Chris on this.
Regards, Joe
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-28 at 23:51 +0100, Joe Orton wrote:
> It does seem like existing callers could reasonably depend on the
> existing behaviour, and that changing it would break them.
The situation is quite complicated. Tricks like the ones from mod_dbd
(or malloc()/free() container + zero sub-pool pointer) will continue to
work. But there could also be differences in behaviour depending on the
values of min, smax, hmax and ttl, which affects how things are killed.
Dunno. Should we err on the side of caution?
> So I'd say it's either a new API guarantee (can't be done in 1.3.x) or
> it's simply an API change (can't be done in 1.x).
Is that your -1 to backporting?
> Also note the extra confusion that apr_pool_pre_cleanup_register will
> have different behaviour in APR 1.3.0 to releases after r678139, and
> that would affect the APR-util API.
Yeah, that's really unfortunate :-(
Going forward (1.4.x and above), we could also simply include respool
(patch sent to the list), that implements sub-pools by default and it
doesn't have a destructor, along the lines of what you explained wrt.
"design pattern". Then people can continue using reslist as is and use
respool for new stuff, without any API/ABI breakage.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Jul 28, 2008 at 10:30:20AM +1000, Bojan Smojver wrote:
> The first one may upset some software out there (probably some very
> corner cases - I can't actually think of an example where it would, to
> be honest), although it won't httpd's mod_dbd for sure.
It does seem like existing callers could reasonably depend on the
existing behaviour, and that changing it would break them.
So I'd say it's either a new API guarantee (can't be done in 1.3.x) or
it's simply an API change (can't be done in 1.x).
Also note the extra confusion that apr_pool_pre_cleanup_register will
have different behaviour in APR 1.3.0 to releases after r678139, and
that would affect the APR-util API.
joe
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-28 at 10:30 +1000, Bojan Smojver wrote:
> - implement when no_threads (patch also available)
Hmm, on second thought, it may be against the rules to backport this to
1.3.x.
If someone were to compile a program against 1.3.3 on a platform with no
threads, this program would not run against 1.3.2 on the same platform,
as this non-threaded library would be missing symbols that 1.3.3 now
has.
It should be OK to this for trunk though.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sun, 2008-07-27 at 19:19 -0500, William A. Rowe, Jr. wrote:
> Or more to the point, when is 1.3.x ready to ship w.r.t. reslist? You'll
> note there hasn't been a 1.3.3 tag since I've been watching all of this
> activity, and didn't want to rush anyone.
There are several things that we can do with reslist:
- backport pre_cleanup
- enforce limits stricter (patch available)
- implement when no_threads (patch also available)
The last two need to be cleared by folks on the list, but I don't think
they will be a subject of a big debate.
The first one may upset some software out there (probably some very
corner cases - I can't actually think of an example where it would, to
be honest), although it won't httpd's mod_dbd for sure. Also, there were
"design pattern" objections to it. So, we need to clear these two
objections before backporting.
> But it is past time to cut 1.3, so give me a clue how much more work is
> needed and how long y'all think it will take to come to consensus on the
> 1.3 branch.
I'm essentially waiting for folks to chime in on the whole pre_cleanup
issue after discussion dust has settled (and the two new patches). Once
that's done, we can roll.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Bojan Smojver wrote:
> On Thu, 2008-07-17 at 05:45 +0000, mturk@apache.org wrote:
>
>> Use apr_pool_pre_cleanup_register for reslist cleanup
>
> Coming back to the original commit message and given the discussion on
> the list, are we backporting this to 1.3.x or not?
Or more to the point, when is 1.3.x ready to ship w.r.t. reslist? You'll
note there hasn't been a 1.3.3 tag since I've been watching all of this
activity, and didn't want to rush anyone.
But it is past time to cut 1.3, so give me a clue how much more work is
needed and how long y'all think it will take to come to consensus on the
1.3 branch.
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-28 at 12:27 +0200, Mladen Turk wrote:
> I plan to thoroughly test those changes against httpd this week.
> So lets wait for a while. If that stops tagging 1.3.x, we can
> postpone that for next tag.
OK. I'm also hoping we'll hear from others.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> On Thu, 2008-07-17 at 05:45 +0000, mturk@apache.org wrote:
>
>> Use apr_pool_pre_cleanup_register for reslist cleanup
>
> Coming back to the original commit message and given the discussion on
> the list, are we backporting > On Thu, 2008-07-17 at 05:45 +0000, mturk@apache.org wrote:
this to 1.3.x or not?
>
I plan to thoroughly test those changes against httpd this week.
So lets wait for a while. If that stops tagging 1.3.x, we can
postpone that for next tag.
Regards
--
^(TM)
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Thu, 2008-07-17 at 05:45 +0000, mturk@apache.org wrote:
> Use apr_pool_pre_cleanup_register for reslist cleanup
Coming back to the original commit message and given the discussion on
the list, are we backporting this to 1.3.x or not?
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Thu, 2008-07-17 at 17:30 +1000, Bojan Smojver wrote:
> I get what you're trying to do with the patch, but can't the destructor
> just allocate memory from that pool and not create a child pool?
Ah, never mind me... Destructor can get called many times, hence the
child pool.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Thu, 2008-07-17 at 05:45 +0000, mturk@apache.org wrote:
> + * If an child pool was created inside apr_reslist_destructor
I get what you're trying to do with the patch, but can't the destructor
just allocate memory from that pool and not create a child pool?
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Sander Striker <s....@striker.nl>.
On Thu, Jul 17, 2008 at 12:25 PM, Mladen Turk <mt...@apache.org> wrote:
> Bojan Smojver wrote:
>> Quoting mturk@apache.org:
>>
>>> Use apr_pool_pre_cleanup_register for reslist cleanup
>>
>> After thinking about this a bit more, this patch may introduce a
>> somewhat unusual (possibly even dangerous) behaviour, as it changes the
>> way cleanups normally work with APR pools.
>>
>
> It only forces calling resource destructors before
> its subpools are destroyed.
> The operation is exactly the same as if one would explicitly
> call destructors for each resource before calling pool_destroy.
> So the pool P is not destroyed before its subpools, only
> the callback is called before subpools are destroyed.
Yes, which is a difference in ordering, as this cleanup on the parent
pool, is called prior to cleanups on all child pools. I can see what
Bojan is getting at.
> The standard cleanup on pool_clear(destroy) first destroys
> all subpools and then call register cleanups.
> This forces (just look at mod_proxy connection pool code)
> the user to make some tricks so it can figure out weather
> the destructor was called by invalidate or by pool destroy.
> If it was by pool destroy then it must skip any subpool
> destroy calls, because those would cause core.
Cheers,
Sander
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Bojan Smojver" <bo...@rexursive.com>:
> The above will lead in the subsequent sub-pool destruction (through the
> regular pool machinery) to segfaults (because that cleanup will refer
> to already destroyed resource).
Of course, depending on resource list creation parameters, the above
may cause problems in the current implementation as well, or may
(accidentally) work.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Bojan Smojver" <bo...@rexursive.com>:
> PPS. The situation I was referring to in my previous e-mails cannot
> actually happen. R always outlives S.
This e-mail was written before pre_cleanup bug in apr_pools.c was
fixed, hence my PPS. However, let me reiterate one point: if people
hang cleanups that depend on the resource off the sub-pool created in
the constructor, but don't destroy the sub-pool in the destructor,
they can be in trouble.
In other words:
constructor:
create sub-pool
create resource (non-pool space)
register cleanup in sub-pool, dependent on resource
destructor:
destroy resource
The above will lead in the subsequent sub-pool destruction (through
the regular pool machinery) to segfaults (because that cleanup will
refer to already destroyed resource).
So, the writers should be aware that that they _must_ call
apr_pool_destroy() in the destructor on any sub-pool they created in
the constructor.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Mladen Turk" <mt...@apache.org>:
> Of course if you can do that.
You can always do that, because it only applies to the resource
container, which must survive in order for de() to check for r->p
(where r is resource container and p the sub-pool in question).
PS. The code works (by accident) even when r is allocated from the
sub-pool because values don't get overwritten by the time de() is
called to check if r->p is NULL.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Mladen Turk" <mt...@apache.org>:
> But not with pre_cleanup. That's the whole point.
> You don't need additional cleanup neither malloc/free.
> Simply call the apr_pool_destroy at the end of the destructor,
> and it'll work.
I'm getting infinite recursion in the latest test.c if I remove pool
pointer reset with the pre_cleanup (in other words, inside res_clean,
if r->p=NULL is removed).
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2008-07-19 at 13:55 +0200, Mladen Turk wrote:
> Me as well. There was an bug in apr_pool.
> Now it works as expected.
Yep, I can confirm that too, with both of my test programs. Thanks for
the fix!
BTW, shouldn't the comment say "constructor" in this sentence of
apr_reslist.c:
"If an child pool was created inside apr_reslist_destructor"
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
>
> If I change the code to this:
> apr_reslist_create(&list,0,10,10,0,con,de,NULL,pool);
>
> I get double free.
>
Me as well. There was an bug in apr_pool.
Now it works as expected.
Regards
--
^(TM)
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2008-07-19 at 06:12 +0200, Mladen Turk wrote:
> But not with pre_cleanup.
Could you try running this? I get an infinite loop (CPU pegged at 100%)
even with pre_cleanup.
If I change the code to this:
apr_reslist_create(&list,0,10,10,0,con,de,NULL,pool);
I get double free.
Either that pre_cleanup thing doesn't work as expected, or I'm going
nuts. I tried to simplify the example to avoid bugs, but maybe I've been
looking at it for too long...
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> On Sat, 2008-07-19 at 09:08 +1000, Bojan Smojver wrote:
>
>> This should be better.
>
> Third time's the charm? I keep forgetting that by the time destructor is
> called, pool may be dead and buried already...
>
But not with pre_cleanup. That's the whole point.
You don't need additional cleanup neither malloc/free.
Simply call the apr_pool_destroy at the end of the destructor,
and it'll work.
Regards
--
^(TM)
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2008-07-19 at 09:08 +1000, Bojan Smojver wrote:
> This should be better.
Third time's the charm? I keep forgetting that by the time destructor is
called, pool may be dead and buried already...
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2008-07-19 at 08:59 +1000, Bojan Smojver wrote:
> Does this solve it?
Eh, sorry - not being careful enough with that cleanup registration.
This should be better.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-07-18 at 12:34 +0200, Mladen Turk wrote:
> For example apr_memcache.c leeks memory because
> it doesn't call apr_pool_destroy in destructor
> although it creates one in constructor.
> (Probably because with standard cleanup it cores)
Does this solve it?
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> On Fri, 2008-07-18 at 16:30 +1000, Bojan Smojver wrote:
>> *r=apr_pcalloc(rpool,sizeof(**r));
>
> This should really be malloc()'ed and free in de().
>
Of course if you can do that. However things like
sockets and other APR objects depends on pools.
With standard cleanup they'll be destroyed before
de() is run.
pre_cleanup address those things. It should be used
in all cases where apr_object creates child pools.
For example apr_memcache.c leeks memory because
it doesn't call apr_pool_destroy in destructor
although it creates one in constructor.
(Probably because with standard cleanup it cores)
Regards
--
^(TM)
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-07-18 at 16:30 +1000, Bojan Smojver wrote:
> *r=apr_pcalloc(rpool,sizeof(**r));
This should really be malloc()'ed and free in de().
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-07-18 at 15:59 +1000, Bojan Smojver wrote:
> Hmm, not safe after all...
OK, I'm going to stop spamming the list now. I promise...
This appears to work with both pre and regular cleanup. The trick is to
zero the pool pointer in the resource cleanup function, so that
destructor can make a choice depending on what happened with the
sub-pool already.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-07-18 at 14:51 +1000, Bojan Smojver wrote:
> What do you mean by random?
Called like this, it crashes (infinite recursion):
apr_reslist_create(&list,0,10,10,0,con,de,NULL,pool);
Hmm, not safe after all...
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-07-18 at 06:25 +0200, Mladen Turk wrote:
> With regular cleanup this should core.
It doesn't here, not even when I do millions loops (i.e. with your k
index and explicit apr_pool_destroy(pool));
> Try to random acquire/release resources and do few loops.
What do you mean by random? All resources must be released back to the
list before destroying the list.
The attached prints "data" 5.5 million times and doesn't segfault. This
is APU with regular cleanup on reslist.
My understanding is that your patch affects creation of sub-pools only
in the destructor. No?
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
>
> static apr_status_t de(void *res,void *parm,apr_pool_t *pool){
> struct res *r=res;
>
> apr_pool_destroy(r->p);
>
With regular cleanup this should core.
Try to random acquire/release resources and do few loops.
The reason is because you are calling destroy on already
destroyed pool.
> int main(int argc,char **argv){
> int i,j;
int k;
> struct res *r[10];
> apr_pool_t *pool;
> apr_reslist_t *list;
>
> apr_initialize();
for (k = 0; k < 10; k++) {
> apr_pool_create(&pool,NULL);
>
> apr_reslist_create(&list,0,0,10,0,con,de,NULL,pool);
>
> for(j=0;j<1000;j++){
> for(i=0;i<10;i++)
> apr_reslist_acquire(list,(void **)&r[i]);
>
> for(i=0;i<10;i++)
> apr_reslist_release(list,r[i]);
> }
>
apr_pool_destroy(pool);
}
> apr_terminate();
> return 0;
> }
Regards
--
^(TM)
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-07-18 at 11:20 +1000, Bojan Smojver wrote:
> The attached program crashes (double free) with or without that
> change.
He, he... It's always good to go through code again. There was a bug in
the test program that caused it to crash. The attached program runs fine
with pre cleanup and regular cleanup.
Apologies for barking up the wrong tree and wasting everyone's time :-(
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Thu, 2008-07-17 at 13:59 +0200, Mladen Turk wrote:
> So the destructor *must not* call apr_pool_destroy(subpool)
> if the parent pool is inside apr_pool_destroy (tricky).
You're right. In fact, this is the situation regardless of how the
cleanup is registered (pre or not). The attached program crashes (double
free) with or without that change.
PS. I don't have any apps that actually do this, but some people may
think it's OK to do this kind of thing, which makes me worried.
PPS. The situation I was referring to in my previous e-mails cannot
actually happen. R always outlives S.
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> Quoting "Mladen Turk" <mt...@apache.org>:
>
>> So the pool P is not destroyed before its subpools, only
>> the callback is called before subpools are destroyed.
>
> True, but that's not what I see as the problem.
>
> Imagine R is a database connection and S a prepared statement that
> depends on R (i.e. uses structures R points to during destruction). If R
> is destroyed (i.e. pointer to it points nowhere after that), any attempt
> to destroy S will give segfaults.
>
> It is a reasonable APR application design to expect that if S registered
> a cleanup with the sub-pool of the pool from which R was allocated, that
> this cleanup will run OK. But, because R was already destroyed, this is
> not true any more.
>
> Can't you just use malloc()/free() in destructor for temporary memory
> needs? Both should be localised to the destructor...
>
OK. Seems this is somehow unclear.
What you are saying is not the case because R is not destroyed
neither points to the invalid address. The callback is called
and the destructor is executed.
The apr_reslist is special kind of object that has two major
things and those are constructor and destructor.
If constructor creates a subpool from the reslist pool
then this subpool will be destroyed before destructor is called.
So the destructor *must not* call apr_pool_destroy(subpool)
if the parent pool is inside apr_pool_destroy (tricky).
What this does is call destructor for each resource before any
subpool is destroyed. Now, the destructor must destroy anything
constructor created. If it was prepared statement, connection, ...
Without that it is impossible to have subpool destroy inside destructor
callback, because it will be called on zombie pool (already destroyed)
With your case if your destructor does nothing, there will be no
change at all.
If you application works when you explicitly close all R and then
call pool destroy it will with this. If not, well :)
Usually the application calls:
apr_reslist_create(pool)
... do something with reslist
apr_reslist_destroy
reslist_destroy_callback
for each r in resources call desctructor(r)
...
apr_pool_destroy(pool)
destory all subpools
Now if somone hits CTRL+C then everything changes to:
apr_reslist_create(pool)
... do something with reslist
apr_pool_destroy(pool)
destroy all subpools
reslist_destroy_callback
for each r in resources call desctructor(r)
So destructor needs an mechanism to figure out
how it was called and if it's OK to an operation
on the pool.
Regards
--
^(TM)
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Mladen Turk" <mt...@apache.org>:
> So the pool P is not destroyed before its subpools, only
> the callback is called before subpools are destroyed.
True, but that's not what I see as the problem.
Imagine R is a database connection and S a prepared statement that
depends on R (i.e. uses structures R points to during destruction). If
R is destroyed (i.e. pointer to it points nowhere after that), any
attempt to destroy S will give segfaults.
It is a reasonable APR application design to expect that if S
registered a cleanup with the sub-pool of the pool from which R was
allocated, that this cleanup will run OK. But, because R was already
destroyed, this is not true any more.
Can't you just use malloc()/free() in destructor for temporary memory
needs? Both should be localised to the destructor...
--
Bojan
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> Quoting mturk@apache.org:
>
>> Use apr_pool_pre_cleanup_register for reslist cleanup
>
> After thinking about this a bit more, this patch may introduce a
> somewhat unusual (possibly even dangerous) behaviour, as it changes the
> way cleanups normally work with APR pools.
>
It only forces calling resource destructors before
its subpools are destroyed.
The operation is exactly the same as if one would explicitly
call destructors for each resource before calling pool_destroy.
So the pool P is not destroyed before its subpools, only
the callback is called before subpools are destroyed.
The standard cleanup on pool_clear(destroy) first destroys
all subpools and then call register cleanups.
This forces (just look at mod_proxy connection pool code)
the user to make some tricks so it can figure out weather
the destructor was called by invalidate or by pool destroy.
If it was by pool destroy then it must skip any subpool
destroy calls, because those would cause core.
Regards
--
^TM
Re: svn commit: r677505 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting mturk@apache.org:
> Use apr_pool_pre_cleanup_register for reslist cleanup
After thinking about this a bit more, this patch may introduce a
somewhat unusual (possibly even dangerous) behaviour, as it changes
the way cleanups normally work with APR pools.
Imagine a resource R, that in its constructor (or during its use)
creates a number of sub-pools, of the pool to which R belongs, for its
sub-resources S. It is a reasonable assumption that R will leave
longer than any S, because sub-pools will be cleaned before the pool
in which R lives. Hence, it would be reasonable to attach destruction
of S to its sub-pool cleanups. This is how most of APR APIs work.
However, because we are destroying R before sub-pools in which S's
live, when the time comes for any S to be destroyed, any reference to
R is invalid and must not be used, which breaks the assumption that R
lives longer than S. Hence, we are forcing people to create destructor
of R in a manner destroys all S sub-resources explicitly, rather than
letting writers rely on normal pool cleanup machinery.
Thoughts?
--
Bojan