You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Justin Erenkrantz <je...@ebuilt.com> on 2001/07/18 02:49:08 UTC
[PATCH] Allow unrelated SMSes to reset
Comments? Would this make anyone happy? =)
This would allow a non-related SMS to be reset when that particular
SMS is reset.
I wonder what would happen if the thread SMS is destroyed and the
cleanup is not unregistered. Hmm, that could be a problem.
The idea is important though. I can work on the details if we
agree this is the way to go. -- justin
Index: include/apr_sms.h
===================================================================
RCS file: /home/cvs/apr/include/apr_sms.h,v
retrieving revision 1.38
diff -u -r1.38 apr_sms.h
--- include/apr_sms.h 2001/07/11 17:00:00 1.38
+++ include/apr_sms.h 2001/07/18 00:41:27
@@ -332,6 +332,14 @@
apr_status_t (*cleanup_fn)(void *));
/**
+ * Register a SMS to be cleaned up when we are reset or destroyed
+ * @param pms The parent SMS to register the cleanup function with
+ * @param cms The child SMS to destroy
+ */
+APR_DECLARE(apr_status_t) apr_sms_cleanup_register_reset(apr_sms_t *pms,
+ apr_sms_t *cms);
+
+/**
* Unregister a previously registered cleanup function
* @param sms The memory system the cleanup function is registered
* with
Index: memory/unix/apr_sms.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_sms.c,v
retrieving revision 1.48
diff -u -r1.48 apr_sms.c
--- memory/unix/apr_sms.c 2001/07/11 14:20:02 1.48
+++ memory/unix/apr_sms.c 2001/07/18 00:41:35
@@ -722,6 +722,13 @@
return APR_SUCCESS;
}
+APR_DECLARE(apr_status_t) apr_sms_cleanup_register_reset(apr_sms_t *pms,
+ apr_sms_t *cms)
+{
+ return apr_sms_cleanup_register(pms, APR_ALL_CLEANUPS, cms,
+ apr_sms_reset);
+}
+
APR_DECLARE(apr_status_t) apr_sms_cleanup_unregister(apr_sms_t *sms,
apr_int32_t type,
const void *data,
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Luke Kenneth Casson Leighton <lk...@samba-tng.org>.
On Wed, Jul 18, 2001 at 05:30:17PM +0200, Sander Striker wrote:
> > Yup. That's the catch. It'd probably need to be a bit more
> > sophisticated than what I've posted OR make the apr_sms_reset a bit more
> > robust (i.e. handle SMSes that have already been cleaned up). I'm
> > leaning towards making the apr_sms_reset more robust. -- justin
>
> I wonder how you plan on doing that.
>
> Furthermore, the method of registering the cleanup with an
> unrelated sms(B) will not work if the registering sms(A)
> is destroyed before the cleanup is run in B. In other
> words, if A is destroyed the cleanup is still registered
> in B. If B is reset/destroyed: boom.
> Or maybe you were planning to unregister the function when
> the thread exits (in which case it would probably work).
manual unregistering is quite common - it's used a lot in
mod_virgule.
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Wed, Jul 18, 2001 at 05:30:17PM +0200, Sander Striker wrote:
> > > can you guarantee that the registration and destruction of the
> > > sms, via cleanup, will not stuff the parent by effectively
> > > destroying the child sms twice?
> >
> > Yup. That's the catch. It'd probably need to be a bit more
> > sophisticated than what I've posted OR make the apr_sms_reset a bit more
> > robust (i.e. handle SMSes that have already been cleaned up). I'm
> > leaning towards making the apr_sms_reset more robust. -- justin
>
> I wonder how you plan on doing that.
>
> Furthermore, the method of registering the cleanup with an
> unrelated sms(B) will not work if the registering sms(A)
> is destroyed before the cleanup is run in B. In other
> words, if A is destroyed the cleanup is still registered
> in B. If B is reset/destroyed: boom.
> Or maybe you were planning to unregister the function when
> the thread exits (in which case it would probably work).
Ideally, it could do that. Add another cleanup in B that calls
unregister on A. That's work, right? Even if we are being called from
A to cleanup? And that fixes the mess of what happens when B dies
before A does. Otherwise, something similar to what Dean Gaudet posted
yesterday with the refcount of 2 might work as well.
> I'd opt for making the registration a bit more sophisticated.
> A simple check for ancestry should be enough.
At least in our use case, ancestry should never occur. But, in the
general case, it very well might. -- justin
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Luke Kenneth Casson Leighton <lk...@samba-tng.org>.
On Wed, Jul 18, 2001 at 05:37:33PM +0200, Sander Striker wrote:
> > yeah, well it seems to me that this would be a really smart
> > thing to do _anyway_.
> >
> > throw an assert if someone tries to register the same
> > 'object' twice, not just a child-sms.
> >
> > is that possible?
>
> Possible, yes :)
mmm.... i don't like the sound of that. possible, yes.
[terry pratchett readers will be familiar with this]
hm, sounds like you're a dwarf who's just been asked to
Botch an engineering job and Get It Done By Wednesday...
...what's the catch? :)
> > if you have a file object, how can you check it's not the same?
> >
> > .... by using the cleanup function's address _and_ the void*
> > as the 'key'?
> >
> > does that work?
>
> Yes (but you need to add the type too). That is how the matching
> works in sms cleanups already. We just don't do a check for
> uniqueness in the registration function yet.
okay, okay: forgot about that. yep!
> > .... about the only exception is the null cleanup [is that used
> > in sms?]
>
> We don't have a null cleanup in sms. It is not needed here.
> Although I have come across a situation which could screw up
> the entire idea behind the sms cleanups. It was in http_log.c
> I believe... :( [I'll expand on that later]
separate subject?
lukes
RE: [PATCH] Allow unrelated SMSes to reset
Posted by Sander Striker <st...@apache.org>.
> yeah, well it seems to me that this would be a really smart
> thing to do _anyway_.
>
> throw an assert if someone tries to register the same
> 'object' twice, not just a child-sms.
>
> is that possible?
Possible, yes :)
> if you have a file object, how can you check it's not the same?
>
> .... by using the cleanup function's address _and_ the void*
> as the 'key'?
>
> does that work?
Yes (but you need to add the type too). That is how the matching
works in sms cleanups already. We just don't do a check for
uniqueness in the registration function yet.
> that way, if it's a file object, then the file-cleanup-function
> pointer is the same and oh, whoops, the file object is the
> same too ASSERT. same for handles, sockets...
>
> .... about the only exception is the null cleanup [is that used
> in sms?]
We don't have a null cleanup in sms. It is not needed here.
Although I have come across a situation which could screw up
the entire idea behind the sms cleanups. It was in http_log.c
I believe... :( [I'll expand on that later]
Sander
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Luke Kenneth Casson Leighton <lk...@samba-tng.org>.
> Yup. That's the catch. It'd probably need to be a bit more
> sophisticated than what I've posted OR make the apr_sms_reset a bit more
> robust (i.e. handle SMSes that have already been cleaned up). I'm
> leaning towards making the apr_sms_reset more robust. -- justin
yeah, well it seems to me that this would be a really smart
thing to do _anyway_.
throw an assert if someone tries to register the same
'object' twice, not just a child-sms.
is that possible?
if you have a file object, how can you check it's not the same?
... by using the cleanup function's address _and_ the void*
as the 'key'?
does that work?
that way, if it's a file object, then the file-cleanup-function
pointer is the same and oh, whoops, the file object is the
same too ASSERT. same for handles, sockets...
... about the only exception is the null cleanup [is that used
in sms?]
luke
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Luke Kenneth Casson Leighton <lk...@samba-tng.org>.
> It seems to me that there are so many ways of doing this that we're already
> talking about adding more and more complexity when we may not need to.
when it comes to the sms api, i'm quite happy to ward off any
recommended changes that make it more complex, don't worry.
think of this as a bit like craftsmanship. yes, there are
loads of ways of doing things: only very few of them are
worthwhile doing that you can be proud of, because [collectively]
we have the experience and knowledge to pick the best.
luke
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Thu, Jul 19, 2001 at 09:57:24AM +0100, David Reid wrote:
> - sms does seem to work, so your comment seems a little strange though very
> Justin :)
Try hammering your SMS-enabled/thread-enabled httpd with requests. I
get segfaults all over the place (I believe I posted the backtrace of
one). Sander and I think the cleanup routines/mechanisms need to be
reexamined. I've backed off using the SMS code for now until I have
time to examine the cleanup code in detail. Sander has suggested a
few places to look.
> What problem are we trying to fix here? That's the one that never seems to
> be answered... ISTR that we are basically saying that memory allocated
> within a thread doesn't have to be locked as it's only available within a
> single thread. Well, we have a lot of ways of doing that already...
For a thread, I want the allocation parent to be the std SMS (i.e.
malloc/free) - this makes the allocation of a thread to be independent
of all other SMS (the locking moves into libc where it belongs).
Others have asked that there be a separate concept for a "cleanup"
parent. When a particular SMS is destroyed (even though it is
unrelated hierarchically), the per-thread SMS needs to go away too
even if its thread is still active (can't even try to kill the thread).
And, you don't want to have a global allocation SMS that all SMS
would use. That brings us back to the current pool code. -- justin
Re: [PATCH] Allow unrelated SMSes to reset
Posted by David Reid <dr...@jetnet.co.uk>.
> On Wed, Jul 18, 2001 at 11:02:27AM -0500, William A. Rowe, Jr. wrote:
> > > And enforcing that the 'allocation' pool is either top-level itself,
or a
> > > descendant of the 'scope' pool, assures that the cleanups will unwind
> > > properly for both (since this thread-child pool is torn down,
everything below
> > > in the 'scope' heirarchy will be cleaned up, as well).
> >
> > Err... "enforce that the 'allocation' pool is either top-level itself,
or the
> > same as or one of the parents of the 'scope' pool".
>
> Yes.
>
> The trick, as I see it, is that we let the apr_thread_create function
> determine what the apr_thread_t's SMS is. If the thread_create
> function says the allocation SMS is now a "top-level" one or whether
> it uses the pool passed in.
>
> BTW, I'm not sure that the apr_pool_cleanup_for_exec will do the right
> thing in the SMS architecture because there is no common parent anymore.
> We may need to rethink how that function works. Thoughts?
>
> Of course, everyone realizes that in order to do all of this, we have
> to get SMS working. -- justin
Well, a few quick comments...
- sms does seem to work, so your comment seems a little strange though very
Justin :)
- how in hells name have we taken a nice simple set of concepts and made
such a pigs ear of them? I mean we started with 1+1=2 and we seem to be at
quantum physics level at the moment. Maybe we should all agree to stop and
take an evening off before we get too far u our own... Well, you get the
idea.
What problem are we trying to fix here? That's the one that never seems to
be answered... ISTR that we are basically saying that memory allocated
within a thread doesn't have to be locked as it's only available within a
single thread. Well, we have a lot of ways of doing that already...
All memory is heirachical or we have real problems, especially in httpd.
1. If we have a "memory thingymebobby" go away then it needs to remove
itself and all those who are based in it - can we all garee this?
2. the pools code isn't truly heirachial as all memory comes from the same
source, so while you have relationships they exist at a management level not
at a resource level. sms the relationships exist at both levels.
3. we don't like globals (with good reasons) but pools currently use a
global that leads to many locking issues.
It seems to me that there are so many ways of doing this that we're already
talking about adding more and more complexity when we may not need to.
For instance, have the apr_initialize function create an sms that will
handle all top level allocations, and get every sms we create to call it for
getting memory. This mimics what we do at present with pools, and allows us
to do cleanups and so on exactly as per pools, but it does add another evil
global!
david
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Wed, Jul 18, 2001 at 11:02:27AM -0500, William A. Rowe, Jr. wrote:
> > And enforcing that the 'allocation' pool is either top-level itself, or a
> > descendant of the 'scope' pool, assures that the cleanups will unwind
> > properly for both (since this thread-child pool is torn down, everything below
> > in the 'scope' heirarchy will be cleaned up, as well).
>
> Err... "enforce that the 'allocation' pool is either top-level itself, or the
> same as or one of the parents of the 'scope' pool".
Yes.
The trick, as I see it, is that we let the apr_thread_create function
determine what the apr_thread_t's SMS is. If the thread_create
function says the allocation SMS is now a "top-level" one or whether
it uses the pool passed in.
BTW, I'm not sure that the apr_pool_cleanup_for_exec will do the right
thing in the SMS architecture because there is no common parent anymore.
We may need to rethink how that function works. Thoughts?
Of course, everyone realizes that in order to do all of this, we have
to get SMS working. -- justin
Re: [PATCH] Allow unrelated SMSes to reset
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "William A. Rowe, Jr." <wr...@rowe-clan.net>
Sent: Wednesday, July 18, 2001 10:27 AM
> And enforcing that the 'allocation' pool is either top-level itself, or a
> descendant of the 'scope' pool, assures that the cleanups will unwind
> properly for both (since this thread-child pool is torn down, everything below
> in the 'scope' heirarchy will be cleaned up, as well).
Err... "enforce that the 'allocation' pool is either top-level itself, or the
same as or one of the parents of the 'scope' pool".
Re: [PATCH] Allow unrelated SMSes to reset
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Justin Erenkrantz" <je...@ebuilt.com>
Sent: Wednesday, July 18, 2001 10:01 AM
> On Wed, Jul 18, 2001 at 01:52:54PM +0200, Luke Kenneth Casson Leighton wrote:
>
> > also, what happens if the child sms is *already part of the parent*?
> >
> > can you guarantee that the registration and destruction of the
> > sms, via cleanup, will not stuff the parent by effectively
> > destroying the child sms twice?
>
> Yup. That's the catch. It'd probably need to be a bit more
> sophisticated than what I've posted OR make the apr_sms_reset a bit more
> robust (i.e. handle SMSes that have already been cleaned up). I'm
> leaning towards making the apr_sms_reset more robust. -- justin
If we simply go to the 'scope' pool to execute cleanups, and the 'allocation'
pool for nothing but allocation, then this whole problem _should_ go away.
And enforcing that the 'allocation' pool is either top-level itself, or a
descendant of the 'scope' pool, assures that the cleanups will unwind
properly for both (since this thread-child pool is torn down, everything below
in the 'scope' heirarchy will be cleaned up, as well).
The entire cleanups_for_exec run correctly too, since all children's 'scope'
pools must descend from the single parent pool.
Bill
RE: [PATCH] Allow unrelated SMSes to reset
Posted by Sander Striker <st...@apache.org>.
> > can you guarantee that the registration and destruction of the
> > sms, via cleanup, will not stuff the parent by effectively
> > destroying the child sms twice?
>
> Yup. That's the catch. It'd probably need to be a bit more
> sophisticated than what I've posted OR make the apr_sms_reset a bit more
> robust (i.e. handle SMSes that have already been cleaned up). I'm
> leaning towards making the apr_sms_reset more robust. -- justin
I wonder how you plan on doing that.
Furthermore, the method of registering the cleanup with an
unrelated sms(B) will not work if the registering sms(A)
is destroyed before the cleanup is run in B. In other
words, if A is destroyed the cleanup is still registered
in B. If B is reset/destroyed: boom.
Or maybe you were planning to unregister the function when
the thread exits (in which case it would probably work).
I'd opt for making the registration a bit more sophisticated.
A simple check for ancestry should be enough.
Sander
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Wed, Jul 18, 2001 at 01:52:54PM +0200, Luke Kenneth Casson Leighton wrote:
> On Tue, Jul 17, 2001 at 05:49:08PM -0700, Justin Erenkrantz wrote:
> > Comments? Would this make anyone happy? =)
>
> yes, funnily enough.
>
> you intend this to be part of the sms 'utility' API, yes?
> a bit like apr_pool_cleanup_register_file() and socket() etc, yes?
I imagine that inside of the apr_thread_create function, this would be
called explicitly. It would make the "real" parent the std SMS (aka
malloc/free) and then it would call this with the parent that was passed
into apr_thread_create to make sure that the thread SMS is reset (or
destroyed) when the parent is.
(Assuming something similiar to Aaron Bannert's patch to the thread API
is accepted. The current code in APR does not allow access to
apr_thread_t's pool. That needs to be fixed before anyone can use the
pool/SMS inside of apr_thread_t.)
> also, what happens if the child sms is *already part of the parent*?
>
> can you guarantee that the registration and destruction of the
> sms, via cleanup, will not stuff the parent by effectively
> destroying the child sms twice?
Yup. That's the catch. It'd probably need to be a bit more
sophisticated than what I've posted OR make the apr_sms_reset a bit more
robust (i.e. handle SMSes that have already been cleaned up). I'm
leaning towards making the apr_sms_reset more robust. -- justin
Re: [PATCH] Allow unrelated SMSes to reset
Posted by Luke Kenneth Casson Leighton <lk...@samba-tng.org>.
On Tue, Jul 17, 2001 at 05:49:08PM -0700, Justin Erenkrantz wrote:
> Comments? Would this make anyone happy? =)
yes, funnily enough.
you intend this to be part of the sms 'utility' API, yes?
a bit like apr_pool_cleanup_register_file() and socket() etc, yes?
basically it would be a 'convenience' wrapper around the
cleanup-register, doing the typecasting etc. for you
around the void*s, yes?
[oh, yes, i see from the patch: yes it is :) :)]
> This would allow a non-related SMS to be reset when that particular
> SMS is reset.
non-related??
... mmm....
mmm... *wary*.
> I wonder what would happen if the thread SMS is destroyed and the
> cleanup is not unregistered. Hmm, that could be a problem.
also, what happens if the child sms is *already part of the parent*?
can you guarantee that the registration and destruction of the
sms, via cleanup, will not stuff the parent by effectively
destroying the child sms twice?
:)
> The idea is important though. I can work on the details if we
> agree this is the way to go. -- justin
in principle, i like it, it 'feels' right, but i can see that
there might be problems with people using it accidentally to
destroy sms childs twice, amongst other things...
> Index: include/apr_sms.h
> ===================================================================
> RCS file: /home/cvs/apr/include/apr_sms.h,v
> retrieving revision 1.38
> diff -u -r1.38 apr_sms.h
> --- include/apr_sms.h 2001/07/11 17:00:00 1.38
> +++ include/apr_sms.h 2001/07/18 00:41:27
> @@ -332,6 +332,14 @@
> apr_status_t (*cleanup_fn)(void *));
>
> /**
> + * Register a SMS to be cleaned up when we are reset or destroyed
> + * @param pms The parent SMS to register the cleanup function with
> + * @param cms The child SMS to destroy
> + */
> +APR_DECLARE(apr_status_t) apr_sms_cleanup_register_reset(apr_sms_t *pms,
> + apr_sms_t *cms);
> +
> +/**
> * Unregister a previously registered cleanup function
> * @param sms The memory system the cleanup function is registered
> * with
> Index: memory/unix/apr_sms.c
> ===================================================================
> RCS file: /home/cvs/apr/memory/unix/apr_sms.c,v
> retrieving revision 1.48
> diff -u -r1.48 apr_sms.c
> --- memory/unix/apr_sms.c 2001/07/11 14:20:02 1.48
> +++ memory/unix/apr_sms.c 2001/07/18 00:41:35
> @@ -722,6 +722,13 @@
> return APR_SUCCESS;
> }
>
> +APR_DECLARE(apr_status_t) apr_sms_cleanup_register_reset(apr_sms_t *pms,
> + apr_sms_t *cms)
> +{
> + return apr_sms_cleanup_register(pms, APR_ALL_CLEANUPS, cms,
> + apr_sms_reset);
> +}
> +
> APR_DECLARE(apr_status_t) apr_sms_cleanup_unregister(apr_sms_t *sms,
> apr_int32_t type,
> const void *data,