You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Mladen Turk <mt...@apache.org> on 2008/07/21 09:24:09 UTC
Changing the order of cleanup for some core objects
Hi,
For you that have track the discussion Bojan, Sander and myself took
about apr_reslist and pre_cleanup_register this will be familiar.
For others I'll give a quick overview of the problem and
API usage diversity. As an example, I'll use the socket API.
If you are creating the socket server you'll need at least the
listening socket and then accept the connections creating more
sockets. Since the accepted sockets gets created and destroyed
during the server lifetime, user must ensure the memory doesn't
leek, by destroying at least the accepted socket memory used.
Since we use pools, this can be achieved only by creating a separate
pool for each accepted socked that will be destroyed once the
socket is not needed any more. All that is pretty straighforward
and used in many implementations.
Now the problem arises with the order of cleanups.
Socket created with S = apr_socket_create(P) registers its
cleanup for pool P. The call for apr_socket_close(S) merely
calls that cleanup causing the underlaying OS socket to get closed.
...
S = apr_socket_create(P)
... do something
... with socket
apr_socket_close(S) -> calls socket_cleanup(S)
...
However if the apr_pool_destroy(P) gets called before
apr_socket_close call (somebody rise the signal, etc..)
the apr_pool_destroy call will cause the socket_cleanup(S)
call and the apr_socket_close(S) will be no-op , and everything
will behave as expected.
Now, lets assume that socket S accepts connections. To keep
the memory usage constant the accept loop must clear all the memory
used so far:
...
S = apr_socket_create(P)
while (alive) {
C = apr_pool_create(P)
A = apr_socket_accept(S, C)
... do something
... with accepted socket A
apr_socket_close(A)
apr_pool_destroy(C) // No memory leaks
}
apr_socket_close(S)
...
So here we create sub-pool (C) of the pool (P)
and after we've done we destroy that pool to keep
the memory usage constant.
However if the signal arises (somebody hit CTRL+C)
the whole sort of problems comes in due to the fact
that upper algorithm doesn't work any more, and the
order of objects destruction occurs.
For example if we call apr_socket_close(S) while
the code had A sockets alive, any operation on the
socket A would fail with some OS specific return value.
... A = apr_socket_accept(S, C)
apr_socket_close(S) -> rv = apr_socket_read(A)
... if (rv == ERROR)
... apr_socket_close(A)
... apr_pool_destroy(C)
... ...
However if instead apr_socket_close the interruption
was done by apr_pool_destroy(P) (socket S will be
closed by the cleanup callback) we cannot use the upper
code any more because:
... A = apr_socket_accept(S, C)
apr_pool_destroy(S) -> rv = apr_socket_read(A)
... if (rv == ERROR)
... apr_socket_close(A)
... apr_pool_destroy(C) => !CORE
The reason why the legitimate code cores is because the
pool from which the socket A was allocated was destroyed
before the actual cleanup for socket S was called.
The user in this case must figure out the reason of
the error code from the apr_socket_read and in the
case the call failed because of the apr_pool_destroy made
on the parent pool it *must not* call its pool destroy
because it was destroyed already so you have double-free
problem.
This is usually resolved either by some global variable,
by additional cleanup for each accepted socket (complete
waste of resources) or something even more innovative ;)
Using pre_cleanup for some objects that presumably have
child pools can resolve some of those issues, but it
definitely needs a thorough evaluation.
Comments?
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
>>
>> Also there is a problem if the root pool gets destroyed in which case
>> you'll be accessing zombie memory, so I don't think this will help.
>
> Yeah, I was afraid you may say something like this :-(
>
:)
> Never mind, it was just a random idea (I didn't find it particularly
> good either).
>
Well I'm also trying to figure out the safe way of determining
the pool liveness, so any idea leads us at least to a better
problem understanding.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 09:12 +0200, Sander Striker wrote:
> Note that pools are specifically *not* thread safe. Only one thread
> can 'own' a pool at a time, and when there are subpools on another
> thread, you'd better protect those subpools from not being destroyed
> while that thread is running. Hence me saying that cases where the
> root pool is being destroyed is moot.
It seems that with Tomcat's Native APR connector, this cannot be
guaranteed any more. Maybe that code needs to get some locking in place
in order to protect pools that don't belong to the threads that are
killing them.
For now, I think we best tell people that they must do external
synchronisation and signalling or they'll suffer segfaults.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Sander Striker <s....@striker.nl>.
On Tue, Jul 22, 2008 at 8:52 AM, Bojan Smojver <bo...@rexursive.com> wrote:
> On Tue, 2008-07-22 at 08:42 +0200, Mladen Turk wrote:
>
>> In the original pool_destroy the pool->sibling access is guarded
>> by mutex. Since in multithreaded environment child pool might be in
>> the middle of destroy process detaching himself, think you'll have
>> to guard the access to pool->sibling in destroy_safe as well
>>
>> Also there is a problem if the root pool gets destroyed in which case
>> you'll be accessing zombie memory, so I don't think this will help.
>
> Yeah, I was afraid you may say something like this :-(
>
> Never mind, it was just a random idea (I didn't find it particularly
> good either).
>
>> Guarding that externally would make things basically single
>> threaded and that would be performance killer.
>
> If we were to go single threaded for pools (i.e. global pool mutex), the
> solution is easy: have a hash of all pool pointers and look in there
> before whacking any pools. Along the lines of my incorrect "volatile"
> approach, but with zero API change. But I'm sure that isn't going to
> fly, because pools need to be fast and multi-threaded.
Note that pools are specifically *not* thread safe. Only one thread
can 'own' a pool at a time, and when there are subpools on another
thread, you'd better protect those subpools from not being destroyed
while that thread is running. Hence me saying that cases where the
root pool is being destroyed is moot.
Cheers,
Sander
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 08:42 +0200, Mladen Turk wrote:
> In the original pool_destroy the pool->sibling access is guarded
> by mutex. Since in multithreaded environment child pool might be in
> the middle of destroy process detaching himself, think you'll have
> to guard the access to pool->sibling in destroy_safe as well
>
> Also there is a problem if the root pool gets destroyed in which case
> you'll be accessing zombie memory, so I don't think this will help.
Yeah, I was afraid you may say something like this :-(
Never mind, it was just a random idea (I didn't find it particularly
good either).
> Guarding that externally would make things basically single
> threaded and that would be performance killer.
If we were to go single threaded for pools (i.e. global pool mutex), the
solution is easy: have a hash of all pool pointers and look in there
before whacking any pools. Along the lines of my incorrect "volatile"
approach, but with zero API change. But I'm sure that isn't going to
fly, because pools need to be fast and multi-threaded.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> On Mon, 2008-07-21 at 22:05 +1000, Bojan Smojver wrote:
>
>> For example...
>
> After thinking about it a bit more, it appears that we really should not
> need to fiddle with locking here. Essentially, when we call this, we can
> assume that root pool and below will not be modified by another thread
> (because we can always control what root pool is), so we can just go
> ahead and search in peace.
>
> Comments?
>
In the original pool_destroy the pool->sibling access is guarded
by mutex. Since in multithreaded environment child pool might be in
the middle of destroy process detaching himself, think you'll have
to guard the access to pool->sibling in destroy_safe as well
Also there is a problem if the root pool gets destroyed in which case
you'll be accessing zombie memory, so I don't think this will help.
As an example I'll give you Tomcat Native APR connector.
It gets loaded in JVM as an module( .jar + native libraries),
so the module actually uses APR, not the application.
The presumption one controls the application
and apr_initialize/apr_terminate doesn't stand any more, cause module
can be loaded/unloaded many times during the application lifetime.
Since apr_terminate (just an edge case example) can happen at
user choice inside different thread, and there is multiple threads
in the middle of the blocking APR call (accepting socket connections
for example), after the native call breaks one cannot be sure that
both local pool and global (root) pool will be valid.
The problem as I see it, requires some event mechanism, because the
callback is actually a 'message post' to another thread, causing the
callback result to actually execute effectively at some future time,
and due to busyness and thread context switching this can lead to
nasty sporadic cores that we observe nowadays.
The reason is because in one thread apr first destroys child pools in
one quick loop and then go immediately to another loop that calls the
callbacks. If the system is very busy this can cause nasty sync issues
because the function like accept can break (caused by pool destroy)
after you call the registered callback, and since those are executed in
the context of another thread you really have no idea what's going on :)
Perhaps we'll need some sort of event mechanism for callbacks that would
cause waiting before going to another callback in the loop or something
like that. Guarding that externally would make things basically single
threaded and that would be performance killer.
I've got close to solving the issues by having the atomic counter
for each long native function call, causing the apr_pool_destroy to
wait for all native calls to exit, but that's a nightmare to maintain
and write the user code.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 22:05 +1000, Bojan Smojver wrote:
> For example...
After thinking about it a bit more, it appears that we really should not
need to fiddle with locking here. Essentially, when we call this, we can
assume that root pool and below will not be modified by another thread
(because we can always control what root pool is), so we can just go
ahead and search in peace.
Comments?
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 21:16 +1000, Bojan Smojver wrote:
> How about we have apr_pool_destroy_safe(). We give it two arguments:
For example... Don't mind the details, especially locks - they are most
likely all screwed up.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Mladen Turk" <mt...@apache.org>:
> I still don't know how to solve that problem effectively, so
> any ideas are welcome.
How about we have apr_pool_destroy_safe(). We give it two arguments:
- pool to kill
- root pool to start searching from (NULL means global pool)
Then we first check if the pool we are trying to destroy is still
there. If so, we whack it. If not, we just return success.
Brute force, I know...
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 16:22 +1000, Bojan Smojver wrote:
> Nah, that wouldn't work if pool has been destroyed already. I'm trying
> to avoid adding locking on all pool destruction, as you can see.
Oh, and the patch that I sent was an incomplete version anyway :-(
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 16:12 +1000, Bojan Smojver wrote:
> + if (pool->is_volatile) {
Nah, that wouldn't work if pool has been destroyed already. I'm trying
to avoid adding locking on all pool destruction, as you can see.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 12:38 +0200, Mladen Turk wrote:
> I still don't know how to solve that problem effectively, so
> any ideas are welcome.
A "volatile" pool idea...
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 11:11 +0200, Mladen Turk wrote:
> If the user creates a subpool the problem will still be present because
> this sub pool will be destroyed before the destructor is called.
> Also any resource allocated from this pool will be destroyed in advance
> to the destructor (since its registered first, it'll be called last),
> while for apr_reslist_invalidate it won't.
> Don't think that'll work.
Given that reslist's code would then be in control of the pointer to the
sub-pool in question through apr_res_t, it can always make sure that one
of its internal cleanups resets that pointer to zero, so that destructor
doesn't destroy the pool again (if that ever occurs). Essentially, the
approach from that memcache patch and mod_dbd is implemented here
internally by reslist.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
>
> Here's how I would do it:
>
> 0. create a subpool for the reslist as a whole, P
> 1. create a subpool Qn of P for each resource Rn in the reslist code,
> and pass that Qn to the constructor and destructor; store a reference
> to Qn in the apr_res_t for Rn
> 2. register the resource destructor as a cleanup on Qn, for the
> corresponding resource Rn
> 3. destroy_resource(Rn) just destroys the Qn subpool
> 4. reslist_cleanup() no longer calls destroy_resource() on each
> resource; it can rely on subpool destruction to do that
> 5. reslist_cleanup() is registered against P rather than the
> passed-in pool
> 6. apr_reslist_destroy() just destroys P
>
> I can't see any cleanup ordering problems with that scheme.
>
If the user creates a subpool the problem will still be present because
this sub pool will be destroyed before the destructor is called.
Also any resource allocated from this pool will be destroyed in advance
to the destructor (since its registered first, it'll be called last),
while for apr_reslist_invalidate it won't.
Don't think that'll work.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 17:00 +1000, Bojan Smojver wrote:
> When you create your socket with apr_socket_create(), there is a pool
> argument there. If you hang a cleanup off it, it will run before close()
> in socket_cleanup().
Example constructor:
--------------------------
struct my_socket_t{
apr_socket_t *s;
void *other_stuff;
};
typedef struct my_socket_t my_socket_t;
static apr_status_t my_close(void *data){
my_socket_t *s=data;
return my_fancy_thing_with_the_socket(s);
}
static apr_status_t db_create(void **rsc,void *parms,apr_pool_t *pool){
my_socket_t *s;
s=apr_pcalloc(pool,sizeof(*s));
apr_socket_create(&s->s,<can't remember what>,pool);
s->other_stuff=parms;
apr_pool_cleanup_register(pool,s,my_close,apr_pool_cleanup_null);
*rsc=s;
return APR_SUCCESS;
}
--------------------------
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting "Mladen Turk" <mt...@apache.org>:
> With Joe's proposal the additional pool for reslist and the
> single cleanup registered there is actually the pre_cleanup,
> so why not using that? We'll preserve 8K of memory to the user
> simply by using that.
I'll try to summarise like this:
- pre_cleanup was invented to address resources that allocate their
own pools, but without ugly hacks (current reslist worked fine for the
ones that don't)
- this changed original behaviour somewhat, so the API is no longer
exactly the same
With both of the above in mind, I'd say making current reslist
behaviour different may have undesirable consequences, so it probably
cannot be done on the stable branch (you noted that yourself), which
is for the life of 1.x. Not good.
Joe's approach can be a whole new thing for people that need per
resource pools, but with a typical APR design pattern.
But, I could be completely wrong about all this. So, the more input we
can get from the list, the better. Any which way we choose to go, I'll
help with testing.
PS. As I said before, I'd even be happy with that 3-way
apr_reslist_create_ex() approach, so that people can choose what they
really like.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
>
> PPS. The implementation of Joe's proposal can be put into something we
> may call respool (as opposed to reslist), so that people can choose that
> if they so wish.
>
With Joe's proposal the additional pool for reslist and the
single cleanup registered there is actually the pre_cleanup,
so why not using that? We'll preserve 8K of memory to the user
simply by using that.
Also seems that during discussion a simple fact that even with
pre_cleanup the standard pool cleanup will happen anyway is
not taken into account. If you don't destroy them in destructor
they'll be destroyed by the standard pool cleanup, so I don't
understand why is that such a big deal. All that will happen
is that destructor will be called before pools gets destroyed,
and Joe was doing that with bunch of additional pools.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 11:23 +0200, Mladen Turk wrote:
> Well but what if I need that pool in destructor?
> It'll be long gone before destructor is called.
That's the whole "design pattern" argument. If you follow the "design
pattern", you won't need it there.
> IMHO reslist should do what is says:
> For each resource there is constructor/destructor and it's
> the API user responsibility to cleanup any resources in
> destructor that have been allocated in the constructor.
> ... very nice, simple and clean thought.
Well, maybe so. Some others would argue that giving people API where you
simply register cleanups as you normally would in APR is a better thing
to do.
Matter of taste and preference, I guess.
PS. In all honesty, even telling people to malloc()/free() their
resource container and reset any sub-pool pointers in the cleanup
registered in the constructor, would be good enough solution for me.
PPS. The implementation of Joe's proposal can be put into something we
may call respool (as opposed to reslist), so that people can choose that
if they so wish.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
>
>> Again you cannot call pool_create in constructor
>> and then pool_destroy in destructor without segfaulting, and
>> that bring us to the original problem.
>
> If you create a sub-pool in your constructor (or during resource use),
> you do not need to destroy it in the destructor, because when the pool
> specific to the resource is destroyed (and it always is), it will go
> away automatically. If you destroy any sub-pools during the course of
> the resource use, that's safe too. So, it less to worry about, really.
>
> This can all be properly documented, of course.
>
Well but what if I need that pool in destructor?
It'll be long gone before destructor is called.
>
> Maybe we can have a new API for reslists (call it
> apr_reslist_create_ex()) where we have an argument to specify if we want
> that per-resource pool, pre_cleanups or not. Then people get the
> behaviour they want:
>
> 1. Default: today's broken reslist, as per 1.3.x
> 2. Don't create per-resource pool: rely on pre_cleanups
> 3. Create per-resource pool: rely on the "design pattern"
>
IMHO reslist should do what is says:
For each resource there is constructor/destructor and it's
the API user responsibility to cleanup any resources in
destructor that have been allocated in the constructor.
... very nice, simple and clean thought.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 10:07 +0200, Mladen Turk wrote:
> That's my point.
> The API is very unclear in that case and you *must* follow
> certain rules.
Well, you definitely must follow certain (head scratching :-) rules
right now as well.
> Again you cannot call pool_create in constructor
> and then pool_destroy in destructor without segfaulting, and
> that bring us to the original problem.
If you create a sub-pool in your constructor (or during resource use),
you do not need to destroy it in the destructor, because when the pool
specific to the resource is destroyed (and it always is), it will go
away automatically. If you destroy any sub-pools during the course of
the resource use, that's safe too. So, it less to worry about, really.
This can all be properly documented, of course.
> Also additional pool is always created for each resource which
> might not be what user needs
Agreed.
> (not to mention the API change
> in that case, since original says that constructor is provided
> with the pool used for apr_reslist).
Also agreed.
Maybe we can have a new API for reslists (call it
apr_reslist_create_ex()) where we have an argument to specify if we want
that per-resource pool, pre_cleanups or not. Then people get the
behaviour they want:
1. Default: today's broken reslist, as per 1.3.x
2. Don't create per-resource pool: rely on pre_cleanups
3. Create per-resource pool: rely on the "design pattern"
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
>>
>> destructor(res) {
>>
>> socket_shutdown(res->sock);
>> socket_close(res->sock);
>> }
>
> If we follow the design pattern Joe explained, we should not do the
> above. Most definitely will segfault.
>
That's my point.
The API is very unclear in that case and you *must* follow
certain rules. Again you cannot call pool_create in constructor
and then pool_destroy in destructor without segfaulting, and
that bring us to the original problem.
Also additional pool is always created for each resource which
might not be what user needs (not to mention the API change
in that case, since original says that constructor is provided
with the pool used for apr_reslist).
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Sander Striker <s....@striker.nl>.
On Wed, Jul 23, 2008 at 9:34 AM, Bojan Smojver <bo...@rexursive.com> wrote:
[...]
> No so much the opposition, just another way to do it. Probably more in
> line with the "design pattern" Joe described.
Exactly.
Cheers,
Sander.
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 09:23 +0200, Mladen Turk wrote:
> What do you mean by "hang a cleanup off it"?
See my example constructor.
> Socket cleanup is private function and cannot be modified.
Of course.
> constructor(res, pool) {
> res->sock = socket_create(pool)
> }
>
> destructor(res) {
>
> socket_shutdown(res->sock);
> socket_close(res->sock);
> }
If we follow the design pattern Joe explained, we should not do the
above. Most definitely will segfault.
> What I can do is:
>
> my_cleanup(res)
> {
> socket_shutdown(res->sock);
> socket_close(res->sock);
I guess the above are some kind of customized things (your own socket
shutdown and close routines). If not, pool cleanup will take care of
that (resource pool dies, socket is shut down), so no need for that.
> res->sock = NULL
No need.
> }
>
> constructor(res, pool) {
> res->sock = socket_create(pool)
> res->subpool = pool_create(pool)
You don't need that (pool creation) - you already have private pool for
this resource.
> cleanup_register(res, res->subpool, my_cleanup);
Just register cleanup against the pool - it's just for this resource.
> }
>
> destructor(res) {
> if (res->sock) {
> pool_cleanup_run(res->subpool, my_cleanup)
No need. This will run when resource is destroyed, because its pool is
destroyed.
>
> So the whole bunch of code, two additional pools for each object
> (16K of wasted memory; multiply that by 1000 sockets), for what?
Actually, that's one additional pool per resource (automatically created
for you) and one extra for the list. About 8 MB of wasted space in your
example - quite a bit of a waste for sure. I cannot argue with that.
> A simple cleanup. I really don't understand the opposition
> for doing a things in a clean and safe way :(.
No so much the opposition, just another way to do it. Probably more in
line with the "design pattern" Joe described.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
>
>> Socket cleanup is private, so
>> cannot modify that.
>
> When you create your socket with apr_socket_create(), there is a pool
> argument there. If you hang a cleanup off it, it will run before close()
> in socket_cleanup().
What do you mean by "hang a cleanup off it"?
Socket cleanup is private function and cannot be modified.
constructor(res, pool) {
res->sock = socket_create(pool)
}
destructor(res) {
socket_shutdown(res->sock);
socket_close(res->sock);
}
Now this might core because socket_create always registers
a cleanup that will call socket_close before destructor, so
my calls in destructor will be touching closed socket.
What I can do is:
my_cleanup(res)
{
socket_shutdown(res->sock);
socket_close(res->sock);
res->sock = NULL
}
constructor(res, pool) {
res->sock = socket_create(pool)
res->subpool = pool_create(pool)
cleanup_register(res, res->subpool, my_cleanup);
}
destructor(res) {
if (res->sock) {
pool_cleanup_run(res->subpool, my_cleanup)
}
}
So the whole bunch of code, two additional pools for each object
(16K of wasted memory; multiply that by 1000 sockets), for what?
A simple cleanup. I really don't understand the opposition
for doing a things in a clean and safe way :(.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 08:56 +0200, Mladen Turk wrote:
> Cannot do that. Cleanups are LIFO. First cleanup registered is res_clean
> so it'll always be executed *last*.
I wasn't talking about res_clean.
> Socket cleanup is private, so
> cannot modify that.
When you create your socket with apr_socket_create(), there is a pool
argument there. If you hang a cleanup off it, it will run before close()
in socket_cleanup().
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> On Wed, 2008-07-23 at 08:34 +0200, Mladen Turk wrote:
>
>> If you create a socket for example in the constructor,
>> socket registers it's own cleanup. This cleanup will be run
>> *before* destructor meaning you are convicted to apr_socket_close,
>> and you cannot cleanly close the socket via shutdown, or send
>> some data upon destruction.
>
> Hang the custom shutdown as cleanup against your socket's pool and this
> will then run before the socket is shut down.
>
Cannot do that. Cleanups are LIFO. First cleanup registered is res_clean
so it'll always be executed *last*. Socket cleanup is private, so
cannot modify that. The solution would be to make additional pool,
register additional cleanup that would do the pre_cleanup trick.
Other solution is that after constructor runs I call
cleanup_kill(res_clean); cleanup_register(res_clean) so it gets
re-registered as last so it will be run as first. But that might
sill cause problems in pool callback loop (detaching cleanups in
callback loop)
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 08:34 +0200, Mladen Turk wrote:
> If you create a socket for example in the constructor,
> socket registers it's own cleanup. This cleanup will be run
> *before* destructor meaning you are convicted to apr_socket_close,
> and you cannot cleanly close the socket via shutdown, or send
> some data upon destruction.
Hang the custom shutdown as cleanup against your socket's pool and this
will then run before the socket is shut down.
> Also any sub pool created is destroyed before destructor is
> called, so, the same story again, but with one level deeper.
Ditto.
No good?
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Sander Striker <s....@striker.nl>.
On Wed, Jul 23, 2008 at 4:22 PM, Mladen Turk <mt...@apache.org> wrote:
> Joe Orton wrote:
>>
>> To be clear, I am not trying to argue that this problem can be easily
>> fixed given the constraints of the current reslist API. But the APR pools
>> design has lasted essentially intact for over 10 years; I will continue to
>> be concerned about any line of argument which goes:
>>
>> 1. we designed this API, shipped it, woo
>> 2. oh dear, seems the API doesn't really work well with APR pools
>> 3. hmmm, but we can't change the API now since it's shipped
>> 4. so we need to change APR pools!
>>
>> Obviously, however much you change APR pools, people will still be able to
>> follow this line of argument. So we need to differentiate between merely
>> being at step (2) of that road to nowhere and finding a case for a generally
>> useful enhancement to the pools API.
>>
>
> That would be mine concern as well if pool API was changed.
> It wasn't changed a bit except adding the possibility to have
> pre_cleanup that behaves like:
Addition is a change. A notable one, as you're stuck with it for the
lifetime of a major release. As you've noticed with
apr_pool_core_create_ex, it's not to be taken too lightly. This
thread is partly about whether the addition is really warranted, or
whether there are existing ways of accomplishing the same.
I think in this thread some important details around pools have come
up that have caused additional thought, the primary one being the
intentional non-thread safety of pools.
Cheers,
Sander
> apr_pool_create(&s, pool)
> apr_pool_cleanup_register(s, foo, cleanup)
> ...
> apr_pool_create(&bar, pool)
> /* Always make sure it's executed first */
> apr_pool_cleanup_kill(s, foo, cleanup)
> apr_pool_cleanup_register(s, foo, cleanup)
> ...
>
> is actually the:
> apr_pool_pre_cleanup_register(pool, foo, cleanup)
>
> Without a need to create an extra pool and making sure
> it's a last one. Nothing more.
>
> Regards
> --
> ^(TM)
>
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Thu, 2008-07-24 at 11:04 +1000, Bojan Smojver wrote:
> Just a quick an dirty.
OOPS! Forgotten testrespool.c.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 13:00 +0100, Joe Orton wrote:
> But that does make the destructor a bit redundant
Just a quick an dirty.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 13:00 +0100, Joe Orton wrote:
> But that does make the destructor a bit redundant - maybe the destructor
> callback is simply a bad fit for APR pools, full stop. Given a reslist
> implementation which enforces a per-resource subpool what else would you
> need to do in a destructor that you couldn't do in cleanups? The two
> examples in httpd using this API only destroy the subpool in the
> destructor anyway.
That is true. You could always hang the cleanup off anything from
non-pool space to the pool you were given, therefore eliminating the
destructor completely.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 16:22 +0200, Mladen Turk wrote:
> Without a need to create an extra pool and making sure
> it's a last one. Nothing more.
Except that in the case of reslist you _already_ have that per resource
pool created by the user (that's where the problem comes from in the
first place).
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
>
> To be clear, I am not trying to argue that this problem can be easily
> fixed given the constraints of the current reslist API. But the APR
> pools design has lasted essentially intact for over 10 years; I will
> continue to be concerned about any line of argument which goes:
>
> 1. we designed this API, shipped it, woo
> 2. oh dear, seems the API doesn't really work well with APR pools
> 3. hmmm, but we can't change the API now since it's shipped
> 4. so we need to change APR pools!
>
> Obviously, however much you change APR pools, people will still be able
> to follow this line of argument. So we need to differentiate between
> merely being at step (2) of that road to nowhere and finding a case for
> a generally useful enhancement to the pools API.
>
That would be mine concern as well if pool API was changed.
It wasn't changed a bit except adding the possibility to have
pre_cleanup that behaves like:
apr_pool_create(&s, pool)
apr_pool_cleanup_register(s, foo, cleanup)
...
apr_pool_create(&bar, pool)
/* Always make sure it's executed first */
apr_pool_cleanup_kill(s, foo, cleanup)
apr_pool_cleanup_register(s, foo, cleanup)
...
is actually the:
apr_pool_pre_cleanup_register(pool, foo, cleanup)
Without a need to create an extra pool and making sure
it's a last one. Nothing more.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jul 23, 2008 at 08:34:17AM +0200, Mladen Turk wrote:
> If you create a socket for example in the constructor,
> socket registers it's own cleanup. This cleanup will be run
> *before* destructor meaning you are convicted to apr_socket_close,
> and you cannot cleanly close the socket via shutdown, or send
> some data upon destruction.
You could register a cleanup on the pool after creating the socket, and
do whatever connection shutdown stuff in that cleanup, which would be
guaranteed to happen before the socket is closed due to LIFO ordering.
But that does make the destructor a bit redundant - maybe the destructor
callback is simply a bad fit for APR pools, full stop. Given a reslist
implementation which enforces a per-resource subpool what else would you
need to do in a destructor that you couldn't do in cleanups? The two
examples in httpd using this API only destroy the subpool in the
destructor anyway.
To be clear, I am not trying to argue that this problem can be easily
fixed given the constraints of the current reslist API. But the APR
pools design has lasted essentially intact for over 10 years; I will
continue to be concerned about any line of argument which goes:
1. we designed this API, shipped it, woo
2. oh dear, seems the API doesn't really work well with APR pools
3. hmmm, but we can't change the API now since it's shipped
4. so we need to change APR pools!
Obviously, however much you change APR pools, people will still be able
to follow this line of argument. So we need to differentiate between
merely being at step (2) of that road to nowhere and finding a case for
a generally useful enhancement to the pools API.
Regards, Joe
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> On Wed, 2008-07-23 at 16:12 +1000, Bojan Smojver wrote:
>
>> Here is a rough sketch.
>
> As Joe predicted, it even works when pool pointer is not reset.
>
It doesn't ;)
> +
> + res->pool = p;
> + res->list = reslist;
> + apr_pool_cleanup_register(p, res, res_clean, apr_pool_cleanup_null);
> +
You should really use apr_pool_pre_cleanup_register here.
If you create a socket for example in the constructor,
socket registers it's own cleanup. This cleanup will be run
*before* destructor meaning you are convicted to apr_socket_close,
and you cannot cleanly close the socket via shutdown, or send
some data upon destruction.
Also any sub pool created is destroyed before destructor is
called, so, the same story again, but with one level deeper.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 21:23 +0200, Ruediger Pluem wrote:
> Wouldn't it be better to keep the resource specific pools and just
> do an apr_pool_clear instead of the apr_pool_create / apr_pool_destroy
> cycle?
Seriously for a second, with resource lists the caller is in charge of
deciding how many resources should be kept around (min, smax, hmax,
ttl). If we keep pools around past resource destruction, we are
internally overriding that limit (at least partially). Dunno - probably
something that should be further discussed.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 21:23 +0200, Ruediger Pluem wrote:
> Wouldn't it be better to keep the resource specific pools and just
> do an apr_pool_clear instead of the apr_pool_create / apr_pool_destroy
> cycle?
Only if you wanted code to run fast :-)
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/23/2008 08:21 AM, Bojan Smojver wrote:
> On Wed, 2008-07-23 at 16:12 +1000, Bojan Smojver wrote:
>
>> Here is a rough sketch.
Just one quick thought from the peanut gallery:
Wouldn't it be better to keep the resource specific pools and just
do an apr_pool_clear instead of the apr_pool_create / apr_pool_destroy
cycle?
Regards
Rüdiger
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 17:57 +1000, Bojan Smojver wrote:
> Just some minor cleanups.
For the record, that would be leaking memory in _invalidate. The
attached should address that.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 16:21 +1000, Bojan Smojver wrote:
> As Joe predicted, it even works when pool pointer is not reset.
Just some minor cleanups.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-07-23 at 16:12 +1000, Bojan Smojver wrote:
> Here is a rough sketch.
As Joe predicted, it even works when pool pointer is not reset.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 09:15 +0100, Joe Orton wrote:
> Here's how I would do it:
Here is a rough sketch.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 09:15 +0100, Joe Orton wrote:
> There is a design pattern (if you can call it that) in APR which is
> important to maintain: object destructors must be implemented as, or
> wrapped in, a pool cleanup, and must only ever be invoked by running
> that cleanup. You see that in most of the APR code.
I read this as:
Resource destructor is for cleaning non-pool resources (e.g. malloc()-ed
memory etc.). Everything else should simply register a cleanup against
resource pool, sub-pool etc., at the appropriate level.
If there are no non-pool resources, destructor is a no-op. My example
code (the one I used to test new reslist code) has:
static apr_status_t db_destroy(void *rsc,void *parms,apr_pool_t *pool){
return APR_SUCCESS;
}
That's because I'm using APU DBD here to test, which is fully pooled.
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Jul 21, 2008 at 10:11:44AM -0700, Chris Darroch wrote:
> The particular issue I'd like to see addressed with any kind of
> "pre-cleanup" of APR pools is the one we work around today in mod_dbd.
> That's a concrete example of a difficult hack around the behaviour
> of APR pools and reslists.
Thanks a lot for writing that up, that's very useful to see.
After my initial analysis of that problem I was ready to agree that the
pre-cleanups seem like an attractive and appropriate solution. But
since working out how I would solve the problem *without* using
pre-cleanups, I am no longer convinced.
Here's how I would do it:
0. create a subpool for the reslist as a whole, P
1. create a subpool Qn of P for each resource Rn in the reslist code,
and pass that Qn to the constructor and destructor; store a reference
to Qn in the apr_res_t for Rn
2. register the resource destructor as a cleanup on Qn, for the
corresponding resource Rn
3. destroy_resource(Rn) just destroys the Qn subpool
4. reslist_cleanup() no longer calls destroy_resource() on each
resource; it can rely on subpool destruction to do that
5. reslist_cleanup() is registered against P rather than the
passed-in pool
6. apr_reslist_destroy() just destroys P
I can't see any cleanup ordering problems with that scheme.
This does mandate use of a subpool for each resource, but presumably
that is mandatory anyway; any resource constructor implementation which
used the main pool would be leaky.
I think that would simplify and improve the code overall both in
APR-util and in the reslist API consumers:
- consumers no longer have to manage subpools; simpler consumers
- not possible (easy) to write leaky consumers which forget to
create a subpool; correctness by design
- slightly simpler apr_reslist.c, maybe
whereas adding pre-cleanups has just *increased* complexity of APR as a
whole, so far as I can see.
There is a design pattern (if you can call it that) in APR which is
important to maintain: object destructors must be implemented as, or
wrapped in, a pool cleanup, and must only ever be invoked by running
that cleanup. You see that in most of the APR code.
Avoiding that "design pattern" will lead to this conflict between
implicit (pool-destroy driven) and explicit (apr_reslist_* API-driven)
destruction, which is what you've got here with the reslist code and
possibly what Mladen is seeing also.
Regards, Joe
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2008-07-22 at 07:29 +1000, Bojan Smojver wrote:
> Or you can malloc() the resource container in the constructor (i.e. rec
> in dbd_construct()) and then free() it in destructor.
Something like this. Completely untested, of course :-)
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 17:00 -0700, Chris Darroch wrote:
> I'm going to have to bow out here but I guess my main concern
> is that it's not unreasonable for developers to assume APR reslists
> and pools should play well together, and they really don't, at
> present -- you either have to resort to bare malloc()/free(),
> leak memory, or work around the cleanup order problem.
I absolutely agree with you that it is less than straightforward - it
certainly made me scratch my head for a while. In terms of mod_dbd,
malloc()/free() solution probably yields slightly lower memory use and
simpler code, but it's just another ugly hack.
Do you think apr_pool_destroy_safe() would help, as presented in the
last patch I sent to the list?
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Bojan Smojver wrote:
> Or you can malloc() the resource container in the constructor (i.e. rec
> in dbd_construct()) and then free() it in destructor. That way you know
> it will outlive anything that may have been whacked.
Yes, certainly, but now you're no longer using whatever APR
allocator you may have set up for your app, and you've lost any other
nice abstractions APR introduced between you and the OS. For me
a large part of the value of APR pools is that they take a lot of
the hassle out of memory management -- except, of course, in cases
like this, where they don't.
I'm going to have to bow out here but I guess my main concern
is that it's not unreasonable for developers to assume APR reslists
and pools should play well together, and they really don't, at
present -- you either have to resort to bare malloc()/free(),
leak memory, or work around the cleanup order problem.
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 10:11 -0700, Chris Darroch wrote:
> See also the notes in mod_dbd.c in dbd_setup() and the workaround
> involving the destructor dbd_destruct(), a flag variable, and
> a second cleanup function dbd_destroy() registered on the parent pool
> (i.e., P) after the reslist is created.
Or you can malloc() the resource container in the constructor (i.e. rec
in dbd_construct()) and then free() it in destructor. That way you know
it will outlive anything that may have been whacked. And regular cleanup
(dbd_close() in this case) can then simply zero the sub-pool pointer,
signalling to destructor that this pool has already been whacked. So,
you save the group->destroyed flag. You may even be able to avoid
per-group pools this way, given that reslists get created once only in
child_init() (i.e. are not dynamic, so no need to create/destroy these
pools for the duration of the process).
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Joe Orton wrote:
> In the model where you have S allocated out of P, let's presume we also
> have a subpool Q.
>
> If for some reason specific to the design of your application, you need
> S to be closed before the destruction of Q, you can register a cleanup
> against Q which does calls apr_socket_close(S). That will give defined
> behaviour.
>
> The pre_cleanup stuff seems to be entirely redundant in this regard. I
> don't understand the motivation for it in the first place, to be honest.
I'm coming in late to this discussion, and I'm afraid I'm also
going to bail out early, so I apologize if my comments are not
relevant or applicable.
The particular issue I'd like to see addressed with any kind of
"pre-cleanup" of APR pools is the one we work around today in mod_dbd.
That's a concrete example of a difficult hack around the behaviour
of APR pools and reslists.
Specifically, suppose you pass pool P to the reslist creation function.
In our constructor for each resource in the list, suppose we need to
allocate a private structure; this is pretty typical. If you allocate
out of P, you leak memory until the reslist itself is destroyed -- not
good for a reslist which lasts for a long time, e.g., a reslist of DB
connections in httpd.
OK, so let's create a sub-pool Q in the constructor and allocate
a per-resource structure out of that. In the destructor we'll
close our DB connection, then destroy Q.
Here's the rub, though. Note that the reslist's own creation function
registered a cleanup for the reslist on P. That reslist cleanup function
begins by calling our destructor function for every resource, which
makes sense.
But now we can't let P be destroyed, because what it will do first
is destroy all its sub-pools (all the Qs we've created for each resource).
Then it will call the cleanups for P, including the reslist's cleanup,
which will invoke our destructor on each resource, which will now
attempt to destroy each Q a second time. This was the cause of PR 39985
which took a lot of effort to solve:
https://issues.apache.org/bugzilla/show_bug.cgi?id=39985
See also the notes in mod_dbd.c in dbd_setup() and the workaround
involving the destructor dbd_destruct(), a flag variable, and
a second cleanup function dbd_destroy() registered on the parent pool
(i.e., P) after the reslist is created.
After resolving that bug Mladen and I had an exchange regarding
the possibility of a pre-cleanup which would allow reslist to register
a pre-cleanup function that dealt with destroying all the resources,
and a post-cleanup function that did any remaining work on before P
was destroyed.
Then destroying P would result in first the reslist's pre-cleanup
running (which would call our destructor for each resource, which would
close the DB connection and destroy each resource's Q), then destroying
any remaining sub-pools (but not our Qs since our destructor's already
taken care of them), and finally calling the reslist's usual cleanup,
which might destroy its internal mutexes or whatever before P is destroyed.
Mladen first described this to me back in the aftermath of dealing
with PR 39985, referencing a previous description of his from mid-2006:
http://mail-archives.apache.org/mod_mbox/apr-dev/200612.mbox/%3c458CDED3.8000108@apache.org%3e
http://mail-archives.apache.org/mod_mbox/apr-dev/200609.mbox/%3c4517A4D4.3070106@apache.org%3e
If this is what's under discussion, please do consider the pre-cleanup
idea carefully, as it would make life a lot easier when handling
reslists of resources with private data structures. If not, I apologize
again for wasting everyone's time. Thanks,
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: Changing the order of cleanup for some core objects
Posted by Sander Striker <s....@striker.nl>.
On Mon, Jul 21, 2008 at 1:12 PM, Mladen Turk <mt...@apache.org> wrote:
> Joe Orton wrote:
>> In the model where you have S allocated out of P, let's presume we also
>> have a subpool Q.
>>
>> If for some reason specific to the design of your application, you need S
>> to be closed before the destruction of Q, you can register a cleanup against
>> Q which does calls apr_socket_close(S). That will give defined behaviour.
>
> That's fine, but then if I explicitly close the S I have to deregister
> the cleanup for Q, close the S, and then register the cleanup for Q
> again. That's how its done now in multiple places where APR is used.
If these places happen to be something specific you can point at,
could you please?
What I'm reading in your response is that you're trying to do both
implicit *and* explicit tear down/clean up? Why?
> I need additional pool and additional cleanup for the exact behavior that
> pre_cleanup does.
The behaviour being: a cleanup being run prior to all subpools of a
certain pool being
destroyed.
The number of use cases for that I perceive as quite limited. Correct
me if I'm wrong.
> Also this pool has to be the 'last'
> one registered, so adding any dependent child pools to P requires
> deregister/register of Q so its assured it's callback is run first.
>
>> The pre_cleanup stuff seems to be entirely redundant in this regard. I
>> don't understand the motivation for it in the first place, to be honest.
>
> See above.
I wish I could say it's now all clear to me, but it isn't :(.
Cheers,
Sander
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
> On Mon, Jul 21, 2008 at 01:12:13PM +0200, Mladen Turk wrote:
>> Joe Orton wrote:
>>> In the model where you have S allocated out of P, let's presume we also
>>> have a subpool Q.
>>>
>>> If for some reason specific to the design of your application, you need
>>> S to be closed before the destruction of Q, you can register a cleanup
>>> against Q which does calls apr_socket_close(S). That will give defined
>>> behaviour.
>>>
>> That's fine, but then if I explicitly close the S I have to deregister
>> the cleanup for Q, close the S, and then register the cleanup for Q
>> again. That's how its done now in multiple places where APR is used.
>
> So you're saying it is possible to achieve the desired behaviour with
> APR today, but it's a bit of effort? Why can't you encapsulate that
> effort in a couple of (AFAICT trivial two-line) wrapper functions?
>
Cannot do that. The user of the API needs to do that in it's code
and take an extra care about pool ordering, so instead of well defined
API, without pre_cleanup its pretty much 'howto'.
> You have to make a case why it is necessary to change the default
> behaviour of APR objects.
I'm not saying to change default behavior, but to have an option
to change the behavior at runtime as applicable by the API user.
The pool logic is pretty much the core of the APR so it should
be more flexible thought.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Jul 21, 2008 at 01:12:13PM +0200, Mladen Turk wrote:
> Joe Orton wrote:
>>
>> In the model where you have S allocated out of P, let's presume we also
>> have a subpool Q.
>>
>> If for some reason specific to the design of your application, you need
>> S to be closed before the destruction of Q, you can register a cleanup
>> against Q which does calls apr_socket_close(S). That will give defined
>> behaviour.
>>
>
> That's fine, but then if I explicitly close the S I have to deregister
> the cleanup for Q, close the S, and then register the cleanup for Q
> again. That's how its done now in multiple places where APR is used.
So you're saying it is possible to achieve the desired behaviour with
APR today, but it's a bit of effort? Why can't you encapsulate that
effort in a couple of (AFAICT trivial two-line) wrapper functions?
You have to make a case why it is necessary to change the default
behaviour of APR objects. I'm not seeing that; I'm not seeing why the
next guy who comes along couldn't write their app in such a way that
it'd be more convenient for APR sockets to be destroyed before the
pre-cleanup phase, and require a pre-pre-cleanup phase to make things a
bit easier for them.
(this argument applies to the existence of the pre-cleanup phase itself,
too)
joe
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
>
> In the model where you have S allocated out of P, let's presume we also
> have a subpool Q.
>
> If for some reason specific to the design of your application, you need
> S to be closed before the destruction of Q, you can register a cleanup
> against Q which does calls apr_socket_close(S). That will give defined
> behaviour.
>
That's fine, but then if I explicitly close the S I have to deregister
the cleanup for Q, close the S, and then register the cleanup for Q
again. That's how its done now in multiple places where APR is used.
I need additional pool and additional cleanup for the exact behavior
that pre_cleanup does. Also this pool has to be the 'last'
one registered, so adding any dependent child pools to P requires
deregister/register of Q so its assured it's callback is run first.
> The pre_cleanup stuff seems to be entirely redundant in this regard. I
> don't understand the motivation for it in the first place, to be honest.
>
See above.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 12:01 +0100, Joe Orton wrote:
> I still don't understand the problem at all. Can you give an example of
> something which cannot be done today because of the current design of
> APR pools?
I think it's not so much that it cannot be done, just that it becomes a
bit of a head scratching exercise (at least for me :-).
For example, this wasn't obvious to me until I actually ran some tests
against Mladen's pre_cleanup stuff (this is an example where sub-pools
were left allocated, because it wasn't obvious how to safely destroy
them):
http://marc.info/?l=apr-dev&m=121642397621125&w=2
So, I think Mladen is trying to have an API where stuff like this
doesn't have to happen. I can sympathize with this (as I like things
simple), but on the other hand, this is a C API and things are not
always straightforward...
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Jul 21, 2008 at 12:38:18PM +0200, Mladen Turk wrote:
> Joe Orton wrote:
>> 2) No APR function is defined to be async-signal-safe, calling
>> apr_pool_destroy(P) from a signal handler is liable to crash and burn
>> regardless of how you change the cleanup ordering.
>>
>
> Well, hitting CTRL+C shouldn't cause the core :)
> I probably wrongly explained the issue. Pool detachment from the parent
> is guarded by mutex (I'm here referring to the threaded usage), so
> pool can be destroyed asynchronously.
>
> Of course the client code must provide the needed synchronization,
> but even with that there is a problem with reverse execution order
> of the callbacks; childs are destroyed before or after the cleanup
> depending on the initiator. For sockets as an example with pool_destroy
> or cleanup case the socket is always hard closed (by invoking its
> pool cleanup callback)
I still don't understand the problem at all. Can you give an example of
something which cannot be done today because of the current design of
APR pools?
In the model where you have S allocated out of P, let's presume we also
have a subpool Q.
If for some reason specific to the design of your application, you need
S to be closed before the destruction of Q, you can register a cleanup
against Q which does calls apr_socket_close(S). That will give defined
behaviour.
The pre_cleanup stuff seems to be entirely redundant in this regard. I
don't understand the motivation for it in the first place, to be honest.
joe
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
>>
>> ...
>> S = apr_socket_create(P)
>> ... do something
>> ... with socket
>> apr_socket_close(S) -> calls socket_cleanup(S)
>> ...
>>
>> However if the apr_pool_destroy(P) gets called before
>> apr_socket_close call (somebody rise the signal, etc..)
>> the apr_pool_destroy call will cause the socket_cleanup(S)
>> call and the apr_socket_close(S) will be no-op , and everything
>> will behave as expected.
>
> 1) Any operation on S after P is destroyed has undefined behaviour,
> including calling apr_socket_close(S). If that doesn't crash and burn
> with the current implementation it is pure chance.
>
Sure, and that's the problem. You need to design your
code accordingly so it skips the operations on S if the P was
destroyed by its parent, and that requires maintaining
some sort of operational state info if you bail
out from a blocking call on socket S. You simply
don't have the info about that. This info has to be
maintained in parent pool so that code doesn't touch the
zombie memory still pointed by S (allocated by P), and
that dramatically complicates the user code.
> 2) No APR function is defined to be async-signal-safe, calling
> apr_pool_destroy(P) from a signal handler is liable to crash and burn
> regardless of how you change the cleanup ordering.
>
Well, hitting CTRL+C shouldn't cause the core :)
I probably wrongly explained the issue. Pool detachment from the parent
is guarded by mutex (I'm here referring to the threaded usage), so
pool can be destroyed asynchronously.
Of course the client code must provide the needed synchronization,
but even with that there is a problem with reverse execution order
of the callbacks; childs are destroyed before or after the cleanup
depending on the initiator. For sockets as an example with pool_destroy
or cleanup case the socket is always hard closed (by invoking its
pool cleanup callback)
I still don't know how to solve that problem effectively, so
any ideas are welcome.
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Jul 21, 2008 at 09:24:09AM +0200, Mladen Turk wrote:
...
> Socket created with S = apr_socket_create(P) registers its
> cleanup for pool P. The call for apr_socket_close(S) merely
> calls that cleanup causing the underlaying OS socket to get closed.
>
> ...
> S = apr_socket_create(P)
> ... do something
> ... with socket
> apr_socket_close(S) -> calls socket_cleanup(S)
> ...
>
> However if the apr_pool_destroy(P) gets called before
> apr_socket_close call (somebody rise the signal, etc..)
> the apr_pool_destroy call will cause the socket_cleanup(S)
> call and the apr_socket_close(S) will be no-op , and everything
> will behave as expected.
1) Any operation on S after P is destroyed has undefined behaviour,
including calling apr_socket_close(S). If that doesn't crash and burn
with the current implementation it is pure chance.
2) No APR function is defined to be async-signal-safe, calling
apr_pool_destroy(P) from a signal handler is liable to crash and burn
regardless of how you change the cleanup ordering.
joe
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 09:24 +0200, Mladen Turk wrote:
> Since we use pools, this can be achieved only by creating a separate
> pool for each accepted socked that will be destroyed once the
> socket is not needed any more.
Somewhat related... I kind of remember a discussion about every new pool
allocating 8k of memory. Do we have a way to control this?
--
Bojan
Re: Changing the order of cleanup for some core objects
Posted by Mladen Turk <mt...@apache.org>.
Sander Striker wrote:
>>
>> Using pre_cleanup for some objects that presumably have
>> child pools can resolve some of those issues, but it
>> definitely needs a thorough evaluation.
>>
>> Comments?
>
> More a question: what is the actual proposal?
>
Well I thought to add some sort of API that would
allow to register a callback list to parent object callback.
Using socket API for example that would allow
listening socket to have some sort of info about
all accepted sockets. Right now socket_cleanup simply
closes the socket, but in this case it would also sweep
trough all registered child sockets and execute
provided cleanup for each of them. The callback code
can be as simple as 'hey I'm dead' flag, or a full blown
notify/wait mechanism.
This is basically extending the pool_cleanup_register
to any object by creating the cleanup chain for each
cleanup callback (in pre_cleanup).
However the idea might not work at the first place :)
Regards
--
^(TM)
Re: Changing the order of cleanup for some core objects
Posted by Sander Striker <s....@striker.nl>.
On Mon, Jul 21, 2008 at 9:24 AM, Mladen Turk <mt...@apache.org> wrote:
> Hi,
>
> For you that have track the discussion Bojan, Sander and myself took
> about apr_reslist and pre_cleanup_register this will be familiar.
> For others I'll give a quick overview of the problem and
> API usage diversity. As an example, I'll use the socket API.
>
>
> If you are creating the socket server you'll need at least the
> listening socket and then accept the connections creating more
> sockets. Since the accepted sockets gets created and destroyed
> during the server lifetime, user must ensure the memory doesn't
> leek, by destroying at least the accepted socket memory used.
> Since we use pools, this can be achieved only by creating a separate
> pool for each accepted socked that will be destroyed once the
> socket is not needed any more. All that is pretty straighforward
> and used in many implementations.
>
> Now the problem arises with the order of cleanups.
>
> Socket created with S = apr_socket_create(P) registers its
> cleanup for pool P. The call for apr_socket_close(S) merely
> calls that cleanup causing the underlaying OS socket to get closed.
>
> ...
> S = apr_socket_create(P)
> ... do something
> ... with socket
> apr_socket_close(S) -> calls socket_cleanup(S)
> ...
>
> However if the apr_pool_destroy(P) gets called before
> apr_socket_close call (somebody rise the signal, etc..)
> the apr_pool_destroy call will cause the socket_cleanup(S)
> call and the apr_socket_close(S) will be no-op , and everything
> will behave as expected.
>
> Now, lets assume that socket S accepts connections. To keep
> the memory usage constant the accept loop must clear all the memory
> used so far:
>
> ...
> S = apr_socket_create(P)
> while (alive) {
> C = apr_pool_create(P)
> A = apr_socket_accept(S, C)
> ... do something
> ... with accepted socket A
> apr_socket_close(A)
> apr_pool_destroy(C) // No memory leaks
> }
> apr_socket_close(S)
> ...
>
> So here we create sub-pool (C) of the pool (P)
> and after we've done we destroy that pool to keep
> the memory usage constant.
>
> However if the signal arises (somebody hit CTRL+C)
> the whole sort of problems comes in due to the fact
> that upper algorithm doesn't work any more, and the
> order of objects destruction occurs.
Not seeing that case TBH. Either the app has a signal handler and
proper teardown logic, or it's a signal you can't catch and the kernel
reclaims all resources anyway.
> For example if we call apr_socket_close(S) while
> the code had A sockets alive, any operation on the
> socket A would fail with some OS specific return value.
>
> ... A = apr_socket_accept(S, C)
> apr_socket_close(S) -> rv = apr_socket_read(A)
> ... if (rv == ERROR)
> ... apr_socket_close(A)
> ... apr_pool_destroy(C)
> ... ...
>
> However if instead apr_socket_close the interruption
> was done by apr_pool_destroy(P) (socket S will be
> closed by the cleanup callback) we cannot use the upper
> code any more because:
>
> ... A = apr_socket_accept(S, C)
> apr_pool_destroy(S) -> rv = apr_socket_read(A)
> ... if (rv == ERROR)
> ... apr_socket_close(A)
> ... apr_pool_destroy(C) => !CORE
>
> The reason why the legitimate code cores is because the
> pool from which the socket A was allocated was destroyed
> before the actual cleanup for socket S was called.
>
> The user in this case must figure out the reason of
> the error code from the apr_socket_read and in the
> case the call failed because of the apr_pool_destroy made
> on the parent pool it *must not* call its pool destroy
> because it was destroyed already so you have double-free
> problem.
The fact that the parent pool is being destroyed means that that code
shouldn't be executing anymore in the first place.
> This is usually resolved either by some global variable,
> by additional cleanup for each accepted socket (complete
> waste of resources)
I'll bite, even though it doesn't matter for this discussion:
additional cleanup is not in all cases a complete waste of resources.
Remember, a lot of cleanups fit inside the initial 8k chunk a pool
starts with.
> or something even more innovative ;)
>
> Using pre_cleanup for some objects that presumably have
> child pools can resolve some of those issues, but it
> definitely needs a thorough evaluation.
>
> Comments?
More a question: what is the actual proposal?
Cheers,
Sander
Re: Changing the order of cleanup for some core objects
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 09:24 +0200, Mladen Turk wrote:
> C = apr_pool_create(P)
So, before this we have (because it's P that gets whacked out or order
in our example):
R = apr_pool_parent_get(P)
> ... apr_pool_destroy(C) => !CORE
This then becomes:
apr_pool_destroy_safe(C, R)
--
Bojan