You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jon Travis <jt...@covalent.net> on 2001/09/18 00:52:21 UTC
New post-log-transaction hook?
I've got a bit of code that needs to run after a connection to a client
has been closed. Right now I can (kind of) spoof this by setting the
keepalive for the client to 0, and registering a cleanup on the
request_req pool. Unfortunately the code in there is somewhat bulky,
so any subsequent cleanups that it registers will never get called
(apparently registering a cleanup within a cleanup no workie).
I'd like to propose a new hook which gets run after connection to
the client has been closed. (in my case, I run some cleanup code
which can take a while, and would like the client to be on its way).
Any thoughts?
-- Jon
Re: New post-log-transaction hook?
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
> Exactly, the client is still connected, but by this time, the client should have
> received all of the data. The only thing we have is a connection that will be
> left open until both sides get around to closing it. If a non-keepalive request
> is still receiving data after the log-transaction phase is run, then it is a bug in
> the server, and it should be fixed.
The client may still be sending data if it is pipelining requests and
the server has decided (due to error or choice) to close the connection
in the middle of the pipeline. The server needs to keep the connnection
open long enough for the client to ack the receipt of the TCP segments
that contained the last server response.
As a protocol, this whole thing is lame -- it is caused because the TCP
socket API is broken in regards to close semantics.
....Roy
Re: New post-log-transaction hook?
Posted by Ryan Bloom <rb...@covalent.net>.
On Monday 17 September 2001 09:33 pm, William A. Rowe, Jr. wrote:
> From: "Jon Travis" <jt...@covalent.net>
> Sent: Monday, September 17, 2001 6:32 PM
>
> > I tried setting keepalive == 0 in the handler, and doing my ju-ju in
> > the log_transaction phase. The client was still hanging around.
>
> That sounds right ... the lazy disconnect logic in httpd can leave a
> connection hanging around a bit. The client will be flushed out soon
> enough - and httpd shouldn't be wasting any more resources/cpu slices
> on it. Did adding your hook at the very back end this change the lazy
> close time profile?
Exactly, the client is still connected, but by this time, the client should have
received all of the data. The only thing we have is a connection that will be
left open until both sides get around to closing it. If a non-keepalive request
is still receiving data after the log-transaction phase is run, then it is a bug in
the server, and it should be fixed.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: New post-log-transaction hook?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Jon Travis" <jt...@covalent.net>
Sent: Monday, September 17, 2001 6:32 PM
> I tried setting keepalive == 0 in the handler, and doing my ju-ju in
> the log_transaction phase. The client was still hanging around.
That sounds right ... the lazy disconnect logic in httpd can leave a
connection hanging around a bit. The client will be flushed out soon
enough - and httpd shouldn't be wasting any more resources/cpu slices
on it. Did adding your hook at the very back end this change the lazy
close time profile?
Bill
Re: New post-log-transaction hook?
Posted by Jon Travis <jt...@covalent.net>.
I tried setting keepalive == 0 in the handler, and doing my ju-ju in
the log_transaction phase. The client was still hanging around.
-- Jon
On Mon, Sep 17, 2001 at 04:11:58PM -0700, Ryan Bloom wrote:
> On Monday 17 September 2001 03:52 pm, Jon Travis wrote:
>
> Why can't you do it in the log_transaction phase. Assuming this is
> not a keepalive connection, the client will be gone by the time that
> phase is run. If this is a keep-alive transaction, then you won't
> save anything by adding another phase.
>
> Ryan
>
> > I've got a bit of code that needs to run after a connection to a client
> > has been closed. Right now I can (kind of) spoof this by setting the
> > keepalive for the client to 0, and registering a cleanup on the
> > request_req pool. Unfortunately the code in there is somewhat bulky,
> > so any subsequent cleanups that it registers will never get called
> > (apparently registering a cleanup within a cleanup no workie).
> >
> > I'd like to propose a new hook which gets run after connection to
> > the client has been closed. (in my case, I run some cleanup code
> > which can take a while, and would like the client to be on its way).
> >
> > Any thoughts?
> >
> > -- Jon
>
> --
>
> ______________________________________________________________
> Ryan Bloom rbb@apache.org
> Covalent Technologies rbb@covalent.net
> --------------------------------------------------------------
Re: New post-log-transaction hook?
Posted by Ryan Bloom <rb...@covalent.net>.
On Monday 17 September 2001 03:52 pm, Jon Travis wrote:
Why can't you do it in the log_transaction phase. Assuming this is
not a keepalive connection, the client will be gone by the time that
phase is run. If this is a keep-alive transaction, then you won't
save anything by adding another phase.
Ryan
> I've got a bit of code that needs to run after a connection to a client
> has been closed. Right now I can (kind of) spoof this by setting the
> keepalive for the client to 0, and registering a cleanup on the
> request_req pool. Unfortunately the code in there is somewhat bulky,
> so any subsequent cleanups that it registers will never get called
> (apparently registering a cleanup within a cleanup no workie).
>
> I'd like to propose a new hook which gets run after connection to
> the client has been closed. (in my case, I run some cleanup code
> which can take a while, and would like the client to be on its way).
>
> Any thoughts?
>
> -- Jon
--
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: child_exit/pchild cleanup (was Re: New post-log-transaction
hook?)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 18 Sep 2001, Ryan Bloom wrote:
> > Actually, I was just about to ask about child_exit for unrelated reasons.
> > Somebody asked me what happened to child_exit... I was just about to tell
> > them that all they had to do was register a cleanup on pchild when I
> > realized that the pchild pointer is static to each individual MPM and I
> > can't figure out how to access it from a module. Is there some other pool
> > with a similar lifetime that should be used besides pchild? Or is there
> > some way to get at pchild that I'm missing?
>
> pchild is passed in to each module as a part of the child_init phase.
Ahhhhh... now I see. I knew it had to be something simple. Thanks.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: child_exit/pchild cleanup (was Re: New post-log-transaction hook?)
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 18 September 2001 08:19 am, Cliff Woolley wrote:
> On Tue, 18 Sep 2001, Ryan Bloom wrote:
> > > You've confused the issue with your subject line (everybody is bugging
> > > out because they're relating it to logging). It should not have
> > > anything to do with "log". We have a pre-connection hook, so call yours
> > > post-connection. That is when you want to run the hook, so we're all
> > > set.
> >
> > Actually, that isn't why I am "bugging out". I don't think we need the
> > hook. In the past, we had a child_exit hook, because we also had a
> > child_init hook. However, the entire group thought that was a hack,
> > because the cleanest way to do child_exit, is to register a cleanup.
>
> Actually, I was just about to ask about child_exit for unrelated reasons.
> Somebody asked me what happened to child_exit... I was just about to tell
> them that all they had to do was register a cleanup on pchild when I
> realized that the pchild pointer is static to each individual MPM and I
> can't figure out how to access it from a module. Is there some other pool
> with a similar lifetime that should be used besides pchild? Or is there
> some way to get at pchild that I'm missing?
pchild is passed in to each module as a part of the child_init phase.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
child_exit/pchild cleanup (was Re: New post-log-transaction hook?)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 18 Sep 2001, Ryan Bloom wrote:
> > You've confused the issue with your subject line (everybody is bugging out
> > because they're relating it to logging). It should not have anything to do
> > with "log". We have a pre-connection hook, so call yours post-connection.
> > That is when you want to run the hook, so we're all set.
>
> Actually, that isn't why I am "bugging out". I don't think we need the hook.
> In the past, we had a child_exit hook, because we also had a child_init hook.
> However, the entire group thought that was a hack, because the cleanest way
> to do child_exit, is to register a cleanup.
Actually, I was just about to ask about child_exit for unrelated reasons.
Somebody asked me what happened to child_exit... I was just about to tell
them that all they had to do was register a cleanup on pchild when I
realized that the pchild pointer is static to each individual MPM and I
can't figure out how to access it from a module. Is there some other pool
with a similar lifetime that should be used besides pchild? Or is there
some way to get at pchild that I'm missing?
Thanks,
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: New post-log-transaction hook?
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 18 September 2001 02:10 am, Greg Stein wrote:
> On Mon, Sep 17, 2001 at 03:52:21PM -0700, Jon Travis wrote:
> > I've got a bit of code that needs to run after a connection to a client
> > has been closed. Right now I can (kind of) spoof this by setting the
> > keepalive for the client to 0, and registering a cleanup on the
> > request_req pool. Unfortunately the code in there is somewhat bulky,
> > so any subsequent cleanups that it registers will never get called
> > (apparently registering a cleanup within a cleanup no workie).
> >
> > I'd like to propose a new hook which gets run after connection to
> > the client has been closed. (in my case, I run some cleanup code
> > which can take a while, and would like the client to be on its way).
> >
> > Any thoughts?
>
> Yes. :-)
>
> You've confused the issue with your subject line (everybody is bugging out
> because they're relating it to logging). It should not have anything to do
> with "log". We have a pre-connection hook, so call yours post-connection.
> That is when you want to run the hook, so we're all set.
Actually, that isn't why I am "bugging out". I don't think we need the hook.
In the past, we had a child_exit hook, because we also had a child_init hook.
However, the entire group thought that was a hack, because the cleanest way
to do child_exit, is to register a cleanup.
Same thing here. Whenever we have wanted to do something after a
particular event, then we register a cleanup for it, and let the pool logic handle
running it at the correct time. The bug here, is that we can't register a cleanup
from within a cleanup. Perhaps we should fix that bug, and leave this as a
pool cleanup.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: New post-log-transaction hook?
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Sep 18, 2001 at 06:52:35AM -0400, Jeff Trawick wrote:
> Greg Stein <gs...@lyra.org> writes:
>
> > 2) move the ap_lingering_close inside ap_process_connection, then call it
> > from with ap_process_connection. This *almost* works. All MPMs have a
> > call to ap_process_connection followed by a call to ap_lingering_close.
> > The only MPM that does other funny stuff in there is the winnt MPM.
> > Conceivably, you could pass a flag that says "I'll manage the shutdown,
> > thankyouvermuch", and winnt would do its close followed by the
> > post-connection hook.
>
> It is nice for ap_lingering_close() to be handled in the MPM. It
> shouldn't be too hard for the MPM to implement its own lingering close
> in a manner that doesn't consume one thread per closing connection
> (e.g., dedicate one thread per MPM to do all the lingering close
> logic). Of course there could always be another hook :)
Great idea, and yes: switching to a hook would still provide for a complex
lingering close capability.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
Posted by Ryan Bloom <rb...@covalent.net>.
On Wednesday 19 September 2001 12:27 pm, Jon Travis wrote:
> On Wed, Sep 19, 2001 at 12:16:24PM -0700, Ryan Bloom wrote:
> > On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote:
> > > From: "Greg Stein" <gs...@lyra.org>
> > > Sent: Wednesday, September 19, 2001 1:26 PM
> > >
> > > > On Wed, Sep 19, 2001 at 01:52:12PM -0400, Rodent of Unusual Size wrote:
> > > > > Greg Stein wrote:
> > > > > > It isn't a bug. Cleanups are for just wrapping things up,
> > > > > > not doing work.
> > > > >
> > > > > If that's the authoritative answer, then we need to provide
> > > > > a supported way for 'doing work' at cleanup time.
> > > > >
> > > > > > You might not even be able to open and use that file if
> > > > > > your pool is n the process of being destroyed.
> > > > >
> > > > > That sounds like utter tripe. If you can't depend on the
> > > > > pool lasting at least until your cleanup routine ends,
> > > > > then the whole cleanup mechanism is seriously borked. AFAIK
> > > > > it isn't, so I think the above assertion *is*.
> > > >
> > > > The problem is cross-dependency between the cleanup actions. One can
> > > > destroy something another cleanup needs. If you restrict their
> > > > actions to "simple" things, then the cross-dependencies are
> > > > (hopefully!) removed.
> > >
> > > Really? No. Cleanups are run as a LIFO stack. Anything that existed
> > > when something was added to the pool must exist when that something is
> > > removed from the pool.
> > >
> > > IMHO, we need to make subpool scrubbing an actual LIFO cleanup as well,
> > > so that will also be true of subpools.
> > >
> > > Considering how we use pools for dynamic libraries and the rest, it's
> > > absolutely vital that they are unspun from the pool in LIFO order of
> > > their creation.
> >
> > I agree with Bill. Having reviewed the code quite deeply yesterday, pool
> > cleanups follow a very clean rule, and registering a cleanup from within
> > a cleanup will always work if done correctly. If you have data in a pool
>
> BZzzzt. The attached code registers a cleanup from within a cleanup, and
> does so 'correctly'. See the program attached at the bottom, which behaves
> incorrectly. It is simple code, but not knowing that a given
> function registers a cleanup can cause major problems (leaking
> file descriptors, etc. eventually). The file should contain 'Cleanup',
> because the cleanup of the file should flush the buffer -- that
> cleanup is never run, though.
>
> > when the cleanup is registered, it is gauranteed to be there when the
> > cleanup is run.
> >
> > Anything else is completely broken.
Sorry Jon, I wasn't clear. I didn't mean if the cleanup was registered correctly,
I meant if the pool_cleanup code worked correctly. I was trying to say that if
we fixed the bug that you pointed out the "right" way, then we are safe.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: pool cleanup
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Sep 20, 2001 at 07:02:58AM -0700, Ryan Bloom wrote:
> On Wednesday 19 September 2001 02:21 pm, Greg Stein wrote:
>...
> > They are not strictly LIFO. You can remove a cleanup and insert a new one
> > at any time. Let's say that the cleanup list looked like:
> >
> > cleanups: A
> >
> > and you add a new one to the "front":
> >
> > cleanups: B A
> >
> > and now case 1, where A needs to rejigger its cleanup param a bit:
> >
> > cleanups: A' B
> >
> > or case 2, where A simply removes its cleanup:
> >
> > cleanups: B
> >
> >
> > Case 2 actually happens quite often.
>
> This is all true, but it is also orthogonal to this conversation.
Partly. The conversation moved into "what can you do in a cleanup". If you
want to look at the simple issue of registering cleanups... okay. But when
people were expecting to be able to do "anything" in a cleanup... that is
intrinsically incorrect.
> The question we are
> trying to answer here, is can you register a cleanup within a cleanup.
Aaron posted a patch, but it introduces too many function calls in the
processing. I posted one that is much more optimal, processing the cleanups
in batches. That would fix your issue.
> If we are in
> the middle of running the cleanups, and somebody actually calls cleanup_run
> or cleanup_kill from within a cleanup, they are broken and it may not work.
My above case wasn't talking about doing those from within a cleanup (which
is definitely and always wrong). I was showing how the cleanups could be
reordered; therefore, how you cannot depend upon particular cross-cleanup
ordering dependencies. Thus, you are actually somewhat limited in what kinds
of things you can truly do in a cleanup.
> It also doesn't make any sense, because the reason to run a cleanup, is to perform
> some action sooner than you would have otherwise, but in this case, we are going
> to perform that action in a few seconds anyway.
I don't get this part. A cleanup is to do just that: clean up after yourself
when the pool goes away. It provides a point in time for specific types of
actions. I'm not sure how that gives you "sooner"; if anything, a cleanup is
for running things later.
> Since the two cases above require a programer to either remove or run a cleanup,
> they don't really make sense in the context of registering a cleanup within a cleanup.
> This means that is safe to register a cleanup within a cleanup, assuming the code
> is patched correctly.
Agreed. My point was addressing the "arbitrary work in a cleanup" meme that
was brought up.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: pool cleanup (was: Re: New post-log-transaction hook?)
Posted by Ryan Bloom <rb...@covalent.net>.
On Wednesday 19 September 2001 02:21 pm, Greg Stein wrote:
> On Wed, Sep 19, 2001 at 12:16:24PM -0700, Ryan Bloom wrote:
> > On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote:
> > > From: "Greg Stein" <gs...@lyra.org>
> > > Sent: Wednesday, September 19, 2001 1:26 PM
> > > Really? No. Cleanups are run as a LIFO stack. Anything that existed
> > > when something was added to the pool must exist when that something is
> > > removed from the pool.
>
> They are not strictly LIFO. You can remove a cleanup and insert a new one
> at any time. Let's say that the cleanup list looked like:
>
> cleanups: A
>
> and you add a new one to the "front":
>
> cleanups: B A
>
> and now case 1, where A needs to rejigger its cleanup param a bit:
>
> cleanups: A' B
>
> or case 2, where A simply removes its cleanup:
>
> cleanups: B
>
>
> Case 2 actually happens quite often.
This is all true, but it is also orthogonal to this conversation. The question we are
trying to answer here, is can you register a cleanup within a cleanup. If we are in
the middle of running the cleanups, and somebody actually calls cleanup_run
or cleanup_kill from within a cleanup, they are broken and it may not work.
It also doesn't make any sense, because the reason to run a cleanup, is to perform
some action sooner than you would have otherwise, but in this case, we are going
to perform that action in a few seconds anyway.
Since the two cases above require a programer to either remove or run a cleanup,
they don't really make sense in the context of registering a cleanup within a cleanup.
This means that is safe to register a cleanup within a cleanup, assuming the code
is patched correctly.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Sander Striker" <st...@apache.org>
Sent: Friday, September 21, 2001 2:51 AM
> > From: Greg Stein [mailto:gstein@lyra.org]
> > Sent: 21 September 2001 09:35
> > On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> > ....
> > > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > > following patch:
> > >
> > > Why is it a bit much? I just took a quick look at it, it is an
> > if, and three assignments.
> >
> > Heh. It is a bit much when you consider you consider a thread in October
> > 1999 discussing using NULL rather than ap_null_cleanup:
> >
> > http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> > Oct/0189.html
> >
> > In that case, an "if" was considered too much :-)
>
> I always wondered about that. It seems so silly to have to pass in
> apr_pool_cleanup_null allmost all the time as second argument. It isn't
> very clear for first time users and certainly not intuitive. Oh well,
> Manoj pointed this out aswell in that thread and still we have
> apr_pool_cleanup_null...
Not only is it non-intuitive (to a first time apr_pools coder) but it
is plain wrong. The cycles to set up the stack and call directly to
a return stub is _far_ slower than testing if (int_val). And compiler
and cpu instruction discussions are always appropriate in this forum.
There are several options here that satisfy everyone, except the few who
seem to insist on forcing broken cleanup behavior to keep coder folks
'on their toes'.
First, there is no reason to push a cleanup at all today of a null value.
We have a single stack of two lists (a prepare fork and a general cleanup.)
There is no reason I can see for a single stack. Two stacks would be more
effective to implement what we want.
Greg's pointed out that the registering a cleanup for child fork (which
we do _often_ due to our set/unset inheritance toggle) breaks the ordering
of sub-object creation. And cleaning up for a child fork is even worse;
even if we 'fix' the broken reordering of pool cleanups, the child cleanups
are still misordered.
That is a _bug_. If we _cannot_ preserve LIFO, our model is wrong,
and it will return to bite us. As one who's spent hours tracking down
dynamic lib load/unload bugs of all sorts, from MS COM+ to Perl XS setup
and teardown, it's much more important to get this right.
Aaron's patch doesn't solve all these problems, but it's a start. Unless
someone has a valid technical veto, let's please commit and begin digging
deeper.
Bill
Re: [PATCH] fix cleanups in cleanups
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Sander Striker" <st...@apache.org>
Sent: Friday, September 21, 2001 2:51 AM
> > From: Greg Stein [mailto:gstein@lyra.org]
> > Sent: 21 September 2001 09:35
> > On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> > ....
> > > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > > following patch:
> > >
> > > Why is it a bit much? I just took a quick look at it, it is an
> > if, and three assignments.
> >
> > Heh. It is a bit much when you consider you consider a thread in October
> > 1999 discussing using NULL rather than ap_null_cleanup:
> >
> > http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> > Oct/0189.html
> >
> > In that case, an "if" was considered too much :-)
>
> I always wondered about that. It seems so silly to have to pass in
> apr_pool_cleanup_null allmost all the time as second argument. It isn't
> very clear for first time users and certainly not intuitive. Oh well,
> Manoj pointed this out aswell in that thread and still we have
> apr_pool_cleanup_null...
Not only is it non-intuitive (to a first time apr_pools coder) but it
is plain wrong. The cycles to set up the stack and call directly to
a return stub is _far_ slower than testing if (int_val). And compiler
and cpu instruction discussions are always appropriate in this forum.
There are several options here that satisfy everyone, except the few who
seem to insist on forcing broken cleanup behavior to keep coder folks
'on their toes'.
First, there is no reason to push a cleanup at all today of a null value.
We have a single stack of two lists (a prepare fork and a general cleanup.)
There is no reason I can see for a single stack. Two stacks would be more
effective to implement what we want.
Greg's pointed out that the registering a cleanup for child fork (which
we do _often_ due to our set/unset inheritance toggle) breaks the ordering
of sub-object creation. And cleaning up for a child fork is even worse;
even if we 'fix' the broken reordering of pool cleanups, the child cleanups
are still misordered.
That is a _bug_. If we _cannot_ preserve LIFO, our model is wrong,
and it will return to bite us. As one who's spent hours tracking down
dynamic lib load/unload bugs of all sorts, from MS COM+ to Perl XS setup
and teardown, it's much more important to get this right.
Aaron's patch doesn't solve all these problems, but it's a start. Unless
someone has a valid technical veto, let's please commit and begin digging
deeper.
Bill
RE: [PATCH] fix cleanups in cleanups
Posted by Sander Striker <st...@apache.org>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: 21 September 2001 09:35
> On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> ....
> > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > following patch:
> >
> > Why is it a bit much? I just took a quick look at it, it is an
> if, and three assignments.
>
> Heh. It is a bit much when you consider you consider a thread in October
> 1999 discussing using NULL rather than ap_null_cleanup:
>
> http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> Oct/0189.html
>
> In that case, an "if" was considered too much :-)
I always wondered about that. It seems so silly to have to pass in
apr_pool_cleanup_null allmost all the time as second argument. It isn't
very clear for first time users and certainly not intuitive. Oh well,
Manoj pointed this out aswell in that thread and still we have
apr_pool_cleanup_null...
Sander
RE: [PATCH] fix cleanups in cleanups
Posted by Sander Striker <st...@apache.org>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: 21 September 2001 09:35
> On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> ....
> > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > following patch:
> >
> > Why is it a bit much? I just took a quick look at it, it is an
> if, and three assignments.
>
> Heh. It is a bit much when you consider you consider a thread in October
> 1999 discussing using NULL rather than ap_null_cleanup:
>
> http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> Oct/0189.html
>
> In that case, an "if" was considered too much :-)
I always wondered about that. It seems so silly to have to pass in
apr_pool_cleanup_null allmost all the time as second argument. It isn't
very clear for first time users and certainly not intuitive. Oh well,
Manoj pointed this out aswell in that thread and still we have
apr_pool_cleanup_null...
Sander
Re: [PATCH] fix cleanups in cleanups
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
...
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
Heh. It is a bit much when you consider you consider a thread in October
1999 discussing using NULL rather than ap_null_cleanup:
http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/Oct/0189.html
In that case, an "if" was considered too much :-)
> I would assume that any compiler worth it's salt would inline this function as well.
Very bad assumption. But this isn't the place to discuss compilers.
> This patch also keeps the LIFO behavior, which is important, because it means
> that it is much less likely that an item allocated out of the pool when the cleanup
> was registered will no longer be there when the cleanup is run.
I am telling you that dependency upon items in the pool is bogus. You cannot
do that. Thus, LIFO means diddly squat.
Consider a case where A inserts its cleanup, expecting to have B present. At
some point between then and the pool cleanup, B goes and removes its
cleanup, then inserts some other cleanup. This new one, B', will now run
*before* A's cleanup.
Gee. Guess what? Your oh-so-needed LIFO behavior has been screwed. B is
*not* present for A to use.
The *only* guarantees you have are between parent and child pools. Within a
pool, you've got nothing.
> > while ((c = p->cleanups) != NULL) {
> > p->cleanups = NULL;
> > run_cleanups(c);
> > }
>
> Which function is this in? I have looked, but the only place that I can find to
> put this code would be in apr_pool_clear, around the run_cleanups code.
Yes, it would go in apr_pool_clear. And a similar one for the child cleanup
case.
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> This makes it more more likely that a variable in the same pool that was available
> when the cleanup was registered would not be available when your cleanup
> ran. I would really want to see a performance analysis before we broke that
> behavior.
Broke what behavior? Registering a cleanup in a cleanup doesn't work now.
The new behavior is, "if you register it, I'll run it after I'm done running
these other cleanups."
Stop. Take a breath. Think about it. LIFO doesn't matter.
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
...
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
Heh. It is a bit much when you consider you consider a thread in October
1999 discussing using NULL rather than ap_null_cleanup:
http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/Oct/0189.html
In that case, an "if" was considered too much :-)
> I would assume that any compiler worth it's salt would inline this function as well.
Very bad assumption. But this isn't the place to discuss compilers.
> This patch also keeps the LIFO behavior, which is important, because it means
> that it is much less likely that an item allocated out of the pool when the cleanup
> was registered will no longer be there when the cleanup is run.
I am telling you that dependency upon items in the pool is bogus. You cannot
do that. Thus, LIFO means diddly squat.
Consider a case where A inserts its cleanup, expecting to have B present. At
some point between then and the pool cleanup, B goes and removes its
cleanup, then inserts some other cleanup. This new one, B', will now run
*before* A's cleanup.
Gee. Guess what? Your oh-so-needed LIFO behavior has been screwed. B is
*not* present for A to use.
The *only* guarantees you have are between parent and child pools. Within a
pool, you've got nothing.
> > while ((c = p->cleanups) != NULL) {
> > p->cleanups = NULL;
> > run_cleanups(c);
> > }
>
> Which function is this in? I have looked, but the only place that I can find to
> put this code would be in apr_pool_clear, around the run_cleanups code.
Yes, it would go in apr_pool_clear. And a similar one for the child cleanup
case.
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> This makes it more more likely that a variable in the same pool that was available
> when the cleanup was registered would not be available when your cleanup
> ran. I would really want to see a performance analysis before we broke that
> behavior.
Broke what behavior? Registering a cleanup in a cleanup doesn't work now.
The new behavior is, "if you register it, I'll run it after I'm done running
these other cleanups."
Stop. Take a breath. Think about it. LIFO doesn't matter.
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
I would assume that any compiler worth it's salt would inline this function as well.
This patch also keeps the LIFO behavior, which is important, because it means
that it is much less likely that an item allocated out of the pool when the cleanup
was registered will no longer be there when the cleanup is run.
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
Which function is this in? I have looked, but the only place that I can find to
put this code would be in apr_pool_clear, around the run_cleanups code.
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
This makes it more more likely that a variable in the same pool that was available
when the cleanup was registered would not be available when your cleanup
ran. I would really want to see a performance analysis before we broke that
behavior.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
>
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
>
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that,
> then it could just calll B. But the most important part: all cleanups *do*
> get run.
You've got to keep the LIFO behavior, or the kind of problems you posted
about yesterday are more likely.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
>
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
>
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that,
> then it could just calll B. But the most important part: all cleanups *do*
> get run.
You've got to keep the LIFO behavior, or the kind of problems you posted
about yesterday are more likely.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
I would assume that any compiler worth it's salt would inline this function as well.
This patch also keeps the LIFO behavior, which is important, because it means
that it is much less likely that an item allocated out of the pool when the cleanup
was registered will no longer be there when the cleanup is run.
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
Which function is this in? I have looked, but the only place that I can find to
put this code would be in apr_pool_clear, around the run_cleanups code.
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
This makes it more more likely that a variable in the same pool that was available
when the cleanup was registered would not be available when your cleanup
ran. I would really want to see a performance analysis before we broke that
behavior.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 08:12 pm, Aaron Bannert wrote:
> On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> > On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> That does encourage deeper recursion, would this be a potential problem
> for OSs like Netware that have a rather small and limited stack size?
> I don't really know how much the pool_clear() routines consume stack space.
>
> > It does not maintain the LIFO behavior where cleanup A registers cleanup
> > B and expects B to run *just after* cleanup A finishes. If A wanted that,
> > then it could just calll B. But the most important part: all cleanups
> > *do* get run.
>
> Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
>
> LIFO == cleanup registers a cleanup, it gets run after the cleanups
> FIFO == cleanup registers a cleanup, it gets run as soon as this one
> returns
>
> Am I missing something?
Well, I wouldn't say what I want, rather the way cleanups have always worked.
As for the FIFO vs LIFO, I think you have it backwards.
LIFO == cleanup registers a cleanup, it gets run as soon as this one returns
FIFO == cleanup registers a cleanup, it gets run after the the current batch
of cleanups.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 08:12 pm, Aaron Bannert wrote:
> On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> > On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> That does encourage deeper recursion, would this be a potential problem
> for OSs like Netware that have a rather small and limited stack size?
> I don't really know how much the pool_clear() routines consume stack space.
>
> > It does not maintain the LIFO behavior where cleanup A registers cleanup
> > B and expects B to run *just after* cleanup A finishes. If A wanted that,
> > then it could just calll B. But the most important part: all cleanups
> > *do* get run.
>
> Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
>
> LIFO == cleanup registers a cleanup, it gets run after the cleanups
> FIFO == cleanup registers a cleanup, it gets run as soon as this one
> returns
>
> Am I missing something?
Well, I wouldn't say what I want, rather the way cleanups have always worked.
As for the FIFO vs LIFO, I think you have it backwards.
LIFO == cleanup registers a cleanup, it gets run as soon as this one returns
FIFO == cleanup registers a cleanup, it gets run after the the current batch
of cleanups.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Aaron Bannert" <aa...@clove.org>
Sent: Thursday, September 20, 2001 10:12 PM
> On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> > On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> > >...
> > > Does this fix it for you? All testmem tests passed for me and your code
> > > above properly flushes "Cleanup" to the file.
> > >
> > > (Someone needs to check my work on run_child_cleanups() and make sure
> > > that the popping is necessary. It looked to be in the same situation.)
> >
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> The reason I went that route, instead of just inlining the pop_cleanup()
> code is that it gets called in two places. If it is a performance issue,
> I'd just put it all inside the while loop in run_cleanups(). Would
> that be preferable?
Aaron, you were right on here. If I'm in a cleanup, which has to create an
apr_foo entity on the same pool, when I'm done with my cleanup, that new
cleanup added by apr_foo should be called _next_. So it's not overkill to
treat this as a pure stack.
> > Basically, the above code processes the cleanups in batches. Everything that
> > was initially registered is processed, then everything registerd during the
> > first cleanup round, etc.
>
> That does encourage deeper recursion, would this be a potential problem
> for OSs like Netware that have a rather small and limited stack size?
> I don't really know how much the pool_clear() routines consume stack space.
This actually sounds cyclically hazerdous. (not that the other cleanup couldn't
suffer the same, if three different cleanups kept registering one another, round
robin style, by creating objects of each other.)
I'd rather stay very clean, and know that everything in existance when a cleanup
is registered may still exist when that cleanup is called. This 'batching'
appears too hazerdous, from that perspective.
> > It does not maintain the LIFO behavior where cleanup A registers cleanup B
> > and expects B to run *just after* cleanup A finishes. If A wanted that, then
> > it could just calll B. But the most important part: all cleanups *do* get
> > run.
>
> Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
>
> LIFO == cleanup registers a cleanup, it gets run after the cleanups
> FIFO == cleanup registers a cleanup, it gets run as soon as this one returns
>
> Am I missing something?
Yes, FIFO = cleanup registers a cleaup, it was the _last_ one registered, so it
would be cleaned up last. That's exactly what we don't want.
LIFO = cleanup registers a cleanup, it is the _last_ one registered, it is the
first one cleaned off (even if we are in the processing of running the list of
cleanups as it was registered.)
Bill
Re: [PATCH] fix cleanups in cleanups
Posted by Aaron Bannert <aa...@clove.org>.
On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
The reason I went that route, instead of just inlining the pop_cleanup()
code is that it gets called in two places. If it is a performance issue,
I'd just put it all inside the while loop in run_cleanups(). Would
that be preferable?
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
Wouldn't that go in run_cleanups(), or does this go in apr_pool_clear()?
> Basically, the above code processes the cleanups in batches. Everything that
> was initially registered is processed, then everything registerd during the
> first cleanup round, etc.
That does encourage deeper recursion, would this be a potential problem
for OSs like Netware that have a rather small and limited stack size?
I don't really know how much the pool_clear() routines consume stack space.
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that, then
> it could just calll B. But the most important part: all cleanups *do* get
> run.
Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
LIFO == cleanup registers a cleanup, it gets run after the cleanups
FIFO == cleanup registers a cleanup, it gets run as soon as this one returns
Am I missing something?
-aaron
RE: Fix broken cleanups model
Posted by Sander Striker <st...@apache.org>.
> From: William A. Rowe, Jr. [mailto:wrowe@rowe-clan.net]
> Sent: 21 September 2001 16:42
> ----- Original Message -----
> From: "Ben Hyde" <bh...@pobox.com>
> To: <de...@httpd.apache.org>
> Sent: Friday, September 21, 2001 9:12 AM
> > I also think it's a long standing mistake that the subpools aren't
> > unwound via the same cleanup stack as everything else but have their
> > one one off to the side; that's a royal pain when you want to allocate
> > a node into it's own subpool while doing things like the above.
>
> Agreed. Here are my thoughts.
>
> We introduce an apr_cleanup_t. This is a _single_ cleanup holder for
> mutiple, extendable types of cleanups (at this moment, pool cleanup and
> cleanup for exec.)
We tried to change that in SMS. You might want to take a look
at that (concept wise).
Sander
Fix broken cleanups model
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
----- Original Message -----
From: "Ben Hyde" <bh...@pobox.com>
To: <de...@httpd.apache.org>
Sent: Friday, September 21, 2001 9:12 AM
Subject: Re: [PATCH] fix cleanups in cleanups
> I also think it's a long standing mistake that the subpools aren't
> unwound via the same cleanup stack as everything else but have their
> one one off to the side; that's a royal pain when you want to allocate
> a node into it's own subpool while doing things like the above.
Agreed. Here are my thoughts.
We introduce an apr_cleanup_t. This is a _single_ cleanup holder for
mutiple, extendable types of cleanups (at this moment, pool cleanup and
cleanup for exec.)
All apr types (and http types) that need cleanups get one of these
records. Because it exists _within_ the allocated data, we can
set/unset them at will. This would no longer change the registrated
ordering.
The API must _allow_ us to move the ordering when it's absolutely
appropriate. If a type is created, but a second call does the 'dirty
work' of obtaining the resource (such as make/create or create/open
pairs) then the order must be fixed to obtaining the resource. If
someone used the apr structure before a resource is obtained, they
really have a placeholder, not the live resource.
We allow users to register a cleanup as we always have. Once it's
registered, they have an apr_cleanup_t. They can unset that cleanup
and register another (changing the order), or they can simply change
their given cleanup. Whichever is appropriate for that call.
As with any library code, a cleanup must be tolerant that _anyone_
could have closed an apr type, either in the mainline, by reacting to
some event, or in it's own reordered cleanup.
Finally, an apr_pool_t is just another apr type. Its cleanup gets
invoked by the mainline cleanup code. I still need to ponder just
what the interaction between two pool's cleanups are, but I'll save
that for another mail after this headcold clears a bit. Hope this
part made sense.
Bill
Re: [PATCH] fix cleanups in cleanups
Posted by Ben Hyde <bh...@pobox.com>.
Greg Stein wrote:
> code processes the cleanups in batches ...
> ... does not maintain the LIFO behavior ...
I think that's a mistake.
I've certainly have writen lots of code that depends on knowing that the
tree I build will be torn down in the right order with the child
cleanups unlinking them from the parents. Sometimes the parents are in
the same pool and sometime not. Sometimes the nodes in these trees
aren't even in pools.
I also think it's a long standing mistake that the subpools aren't
unwound via the same cleanup stack as everything else but have their
one one off to the side; that's a royal pain when you want to allocate
a node into it's own subpool while doing things like the above.
- ben
http://www.cozy.org:83/bhyde.html
Re: [PATCH] fix cleanups in cleanups
Posted by Aaron Bannert <aa...@clove.org>.
On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
The reason I went that route, instead of just inlining the pop_cleanup()
code is that it gets called in two places. If it is a performance issue,
I'd just put it all inside the while loop in run_cleanups(). Would
that be preferable?
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
Wouldn't that go in run_cleanups(), or does this go in apr_pool_clear()?
> Basically, the above code processes the cleanups in batches. Everything that
> was initially registered is processed, then everything registerd during the
> first cleanup round, etc.
That does encourage deeper recursion, would this be a potential problem
for OSs like Netware that have a rather small and limited stack size?
I don't really know how much the pool_clear() routines consume stack space.
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that, then
> it could just calll B. But the most important part: all cleanups *do* get
> run.
Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
LIFO == cleanup registers a cleanup, it gets run after the cleanups
FIFO == cleanup registers a cleanup, it gets run as soon as this one returns
Am I missing something?
-aaron
Re: [PATCH] fix cleanups in cleanups
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
>...
> Does this fix it for you? All testmem tests passed for me and your code
> above properly flushes "Cleanup" to the file.
>
> (Someone needs to check my work on run_child_cleanups() and make sure
> that the popping is necessary. It looked to be in the same situation.)
Calling pop_cleanup() on every iteration is a bit much. Consider the
following patch:
while ((c = p->cleanups) != NULL) {
p->cleanups = NULL;
run_cleanups(c);
}
You don't even have to change run_cleanups or run_child_cleanups.
Basically, the above code processes the cleanups in batches. Everything that
was initially registered is processed, then everything registerd during the
first cleanup round, etc.
It does not maintain the LIFO behavior where cleanup A registers cleanup B
and expects B to run *just after* cleanup A finishes. If A wanted that, then
it could just calll B. But the most important part: all cleanups *do* get
run.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
>...
> Does this fix it for you? All testmem tests passed for me and your code
> above properly flushes "Cleanup" to the file.
>
> (Someone needs to check my work on run_child_cleanups() and make sure
> that the popping is necessary. It looked to be in the same situation.)
Calling pop_cleanup() on every iteration is a bit much. Consider the
following patch:
while ((c = p->cleanups) != NULL) {
p->cleanups = NULL;
run_cleanups(c);
}
You don't even have to change run_cleanups or run_child_cleanups.
Basically, the above code processes the cleanups in batches. Everything that
was initially registered is processed, then everything registerd during the
first cleanup round, etc.
It does not maintain the LIFO behavior where cleanup A registers cleanup B
and expects B to run *just after* cleanup A finishes. If A wanted that, then
it could just calll B. But the most important part: all cleanups *do* get
run.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
[PATCH] fix cleanups in cleanups (Was Re: New post-log-transaction hook?)
Posted by Aaron Bannert <aa...@clove.org>.
On Wed, Sep 19, 2001 at 12:27:35PM -0700, Jon Travis wrote:
> BZzzzt. The attached code registers a cleanup from within a cleanup, and
> does so 'correctly'. See the program attached at the bottom, which behaves
> incorrectly. It is simple code, but not knowing that a given
> function registers a cleanup can cause major problems (leaking
> file descriptors, etc. eventually). The file should contain 'Cleanup',
> because the cleanup of the file should flush the buffer -- that
> cleanup is never run, though.
>
> > when the cleanup is registered, it is gauranteed to be there when the cleanup
> > is run.
> >
> > Anything else is completely broken.
>
>
> #include "apr.h"
> #include "apr_file_io.h"
>
> static apr_status_t my_cleanup(void *cbdata){
> apr_pool_t *p = cbdata;
> apr_file_t *file;
>
> apr_file_open(&file, "/tmp/bonk",
> APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BUFFERED,
> APR_OS_DEFAULT, p);
> apr_file_printf(file, "Cleanup");
> return APR_SUCCESS;
> }
>
> int main(int argc, char *argv[]){
> apr_pool_t *pool;
>
> apr_initialize();
> apr_pool_create(&pool, NULL);
> apr_pool_cleanup_register(pool, pool, my_cleanup, NULL);
> apr_pool_destroy(pool);
> apr_terminate();
> return 0;
> }
Does this fix it for you? All testmem tests passed for me and your code
above properly flushes "Cleanup" to the file.
(Someone needs to check my work on run_child_cleanups() and make sure
that the popping is necessary. It looked to be in the same situation.)
-aaron
Index: memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvspublic/apr/memory/unix/apr_pools.c,v
retrieving revision 1.111
diff -u -r1.111 apr_pools.c
--- memory/unix/apr_pools.c 2001/09/17 20:12:23 1.111
+++ memory/unix/apr_pools.c 2001/09/20 18:06:46
@@ -564,7 +564,8 @@
struct process_chain;
struct cleanup;
-static void run_cleanups(struct cleanup *c);
+static struct cleanup *pop_cleanup(apr_pool_t *p);
+static void run_cleanups(apr_pool_t *p);
static void free_proc_chain(struct process_chain *p);
static apr_pool_t *permanent_pool;
@@ -764,26 +765,35 @@
return (*cleanup) (data);
}
-static void run_cleanups(struct cleanup *c)
+static struct cleanup *pop_cleanup(apr_pool_t *p)
{
- while (c) {
- (*c->plain_cleanup) ((void *)c->data);
- c = c->next;
+ struct cleanup *c;
+ if ((c = p->cleanups)) {
+ p->cleanups = c->next;
+ c->next = NULL;
}
+ return c;
}
-static void run_child_cleanups(struct cleanup *c)
+static void run_cleanups(apr_pool_t *p)
{
- while (c) {
+ struct cleanup *c;
+ while ((c = pop_cleanup(p))) {
+ (*c->plain_cleanup) ((void *)c->data);
+ }
+}
+
+static void run_child_cleanups(apr_pool_t *p)
+{
+ struct cleanup *c;
+ while ((c = pop_cleanup(p))) {
(*c->child_cleanup) ((void *)c->data);
- c = c->next;
}
}
static void cleanup_pool_for_exec(apr_pool_t *p)
{
- run_child_cleanups(p->cleanups);
- p->cleanups = NULL;
+ run_child_cleanups(p);
for (p = p->sub_pools; p; p = p->sub_next) {
cleanup_pool_for_exec(p);
@@ -863,8 +873,7 @@
}
/* run cleanups and free any subprocesses. */
- run_cleanups(a->cleanups);
- a->cleanups = NULL;
+ run_cleanups(a);
free_proc_chain(a->subprocesses);
a->subprocesses = NULL;
Re: New post-log-transaction hook?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Jon Travis" <jt...@covalent.net>
Sent: Wednesday, September 19, 2001 2:27 PM
> On Wed, Sep 19, 2001 at 12:16:24PM -0700, Ryan Bloom wrote:
> > On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote:
> > >
> > > Considering how we use pools for dynamic libraries and the rest, it's
> > > absolutely vital that they are unspun from the pool in LIFO order of their
> > > creation.
> >
> > I agree with Bill. Having reviewed the code quite deeply yesterday, pool
> > cleanups follow a very clean rule, and registering a cleanup from within
> > a cleanup will always work if done correctly. If you have data in a pool
>
> BZzzzt. The attached code registers a cleanup from within a cleanup, and
> does so 'correctly'. See the program attached at the bottom, which behaves
> incorrectly. It is simple code, but not knowing that a given
> function registers a cleanup can cause major problems (leaking
> file descriptors, etc. eventually). The file should contain 'Cleanup',
> because the cleanup of the file should flush the buffer -- that
> cleanup is never run, though.
Jon, there is no _doubt_ that it's broken right now. I was speaking of the
'ideal', not today's borked behavior.
Extending the LIFO cleanups idea as a stack, we should be popping as we call
each cleanup, and allowing new cleanups to be pushed onto the end of the
cleanup stack..
[PATCH] fix cleanups in cleanups (Was Re: New post-log-transaction hook?)
Posted by Aaron Bannert <aa...@clove.org>.
On Wed, Sep 19, 2001 at 12:27:35PM -0700, Jon Travis wrote:
> BZzzzt. The attached code registers a cleanup from within a cleanup, and
> does so 'correctly'. See the program attached at the bottom, which behaves
> incorrectly. It is simple code, but not knowing that a given
> function registers a cleanup can cause major problems (leaking
> file descriptors, etc. eventually). The file should contain 'Cleanup',
> because the cleanup of the file should flush the buffer -- that
> cleanup is never run, though.
>
> > when the cleanup is registered, it is gauranteed to be there when the cleanup
> > is run.
> >
> > Anything else is completely broken.
>
>
> #include "apr.h"
> #include "apr_file_io.h"
>
> static apr_status_t my_cleanup(void *cbdata){
> apr_pool_t *p = cbdata;
> apr_file_t *file;
>
> apr_file_open(&file, "/tmp/bonk",
> APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BUFFERED,
> APR_OS_DEFAULT, p);
> apr_file_printf(file, "Cleanup");
> return APR_SUCCESS;
> }
>
> int main(int argc, char *argv[]){
> apr_pool_t *pool;
>
> apr_initialize();
> apr_pool_create(&pool, NULL);
> apr_pool_cleanup_register(pool, pool, my_cleanup, NULL);
> apr_pool_destroy(pool);
> apr_terminate();
> return 0;
> }
Does this fix it for you? All testmem tests passed for me and your code
above properly flushes "Cleanup" to the file.
(Someone needs to check my work on run_child_cleanups() and make sure
that the popping is necessary. It looked to be in the same situation.)
-aaron
Index: memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvspublic/apr/memory/unix/apr_pools.c,v
retrieving revision 1.111
diff -u -r1.111 apr_pools.c
--- memory/unix/apr_pools.c 2001/09/17 20:12:23 1.111
+++ memory/unix/apr_pools.c 2001/09/20 18:06:46
@@ -564,7 +564,8 @@
struct process_chain;
struct cleanup;
-static void run_cleanups(struct cleanup *c);
+static struct cleanup *pop_cleanup(apr_pool_t *p);
+static void run_cleanups(apr_pool_t *p);
static void free_proc_chain(struct process_chain *p);
static apr_pool_t *permanent_pool;
@@ -764,26 +765,35 @@
return (*cleanup) (data);
}
-static void run_cleanups(struct cleanup *c)
+static struct cleanup *pop_cleanup(apr_pool_t *p)
{
- while (c) {
- (*c->plain_cleanup) ((void *)c->data);
- c = c->next;
+ struct cleanup *c;
+ if ((c = p->cleanups)) {
+ p->cleanups = c->next;
+ c->next = NULL;
}
+ return c;
}
-static void run_child_cleanups(struct cleanup *c)
+static void run_cleanups(apr_pool_t *p)
{
- while (c) {
+ struct cleanup *c;
+ while ((c = pop_cleanup(p))) {
+ (*c->plain_cleanup) ((void *)c->data);
+ }
+}
+
+static void run_child_cleanups(apr_pool_t *p)
+{
+ struct cleanup *c;
+ while ((c = pop_cleanup(p))) {
(*c->child_cleanup) ((void *)c->data);
- c = c->next;
}
}
static void cleanup_pool_for_exec(apr_pool_t *p)
{
- run_child_cleanups(p->cleanups);
- p->cleanups = NULL;
+ run_child_cleanups(p);
for (p = p->sub_pools; p; p = p->sub_next) {
cleanup_pool_for_exec(p);
@@ -863,8 +873,7 @@
}
/* run cleanups and free any subprocesses. */
- run_cleanups(a->cleanups);
- a->cleanups = NULL;
+ run_cleanups(a);
free_proc_chain(a->subprocesses);
a->subprocesses = NULL;
Re: New post-log-transaction hook?
Posted by Jon Travis <jt...@covalent.net>.
On Wed, Sep 19, 2001 at 12:16:24PM -0700, Ryan Bloom wrote:
> On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote:
> > From: "Greg Stein" <gs...@lyra.org>
> > Sent: Wednesday, September 19, 2001 1:26 PM
> >
> > > On Wed, Sep 19, 2001 at 01:52:12PM -0400, Rodent of Unusual Size wrote:
> > > > Greg Stein wrote:
> > > > > It isn't a bug. Cleanups are for just wrapping things up,
> > > > > not doing work.
> > > >
> > > > If that's the authoritative answer, then we need to provide
> > > > a supported way for 'doing work' at cleanup time.
> > > >
> > > > > You might not even be able to open and use that file if
> > > > > your pool is n the process of being destroyed.
> > > >
> > > > That sounds like utter tripe. If you can't depend on the
> > > > pool lasting at least until your cleanup routine ends,
> > > > then the whole cleanup mechanism is seriously borked. AFAIK
> > > > it isn't, so I think the above assertion *is*.
> > >
> > > The problem is cross-dependency between the cleanup actions. One can
> > > destroy something another cleanup needs. If you restrict their actions to
> > > "simple" things, then the cross-dependencies are (hopefully!) removed.
> >
> > Really? No. Cleanups are run as a LIFO stack. Anything that existed when
> > something was added to the pool must exist when that something is removed
> > from the pool.
> >
> > IMHO, we need to make subpool scrubbing an actual LIFO cleanup as well, so
> > that will also be true of subpools.
> >
> > Considering how we use pools for dynamic libraries and the rest, it's
> > absolutely vital that they are unspun from the pool in LIFO order of their
> > creation.
>
> I agree with Bill. Having reviewed the code quite deeply yesterday, pool
> cleanups follow a very clean rule, and registering a cleanup from within
> a cleanup will always work if done correctly. If you have data in a pool
BZzzzt. The attached code registers a cleanup from within a cleanup, and
does so 'correctly'. See the program attached at the bottom, which behaves
incorrectly. It is simple code, but not knowing that a given
function registers a cleanup can cause major problems (leaking
file descriptors, etc. eventually). The file should contain 'Cleanup',
because the cleanup of the file should flush the buffer -- that
cleanup is never run, though.
> when the cleanup is registered, it is gauranteed to be there when the cleanup
> is run.
>
> Anything else is completely broken.
#include "apr.h"
#include "apr_file_io.h"
static apr_status_t my_cleanup(void *cbdata){
apr_pool_t *p = cbdata;
apr_file_t *file;
apr_file_open(&file, "/tmp/bonk",
APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BUFFERED,
APR_OS_DEFAULT, p);
apr_file_printf(file, "Cleanup");
return APR_SUCCESS;
}
int main(int argc, char *argv[]){
apr_pool_t *pool;
apr_initialize();
apr_pool_create(&pool, NULL);
apr_pool_cleanup_register(pool, pool, my_cleanup, NULL);
apr_pool_destroy(pool);
apr_terminate();
return 0;
}
pool cleanup (was: Re: New post-log-transaction hook?)
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Sep 19, 2001 at 12:16:24PM -0700, Ryan Bloom wrote:
> On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote:
> > From: "Greg Stein" <gs...@lyra.org>
> > Sent: Wednesday, September 19, 2001 1:26 PM
>...
> > > The problem is cross-dependency between the cleanup actions. One can
> > > destroy something another cleanup needs. If you restrict their actions to
> > > "simple" things, then the cross-dependencies are (hopefully!) removed.
> >
> > Really? No. Cleanups are run as a LIFO stack. Anything that existed when
> > something was added to the pool must exist when that something is removed
> > from the pool.
They are not strictly LIFO. You can remove a cleanup and insert a new one at
any time. Let's say that the cleanup list looked like:
cleanups: A
and you add a new one to the "front":
cleanups: B A
and now case 1, where A needs to rejigger its cleanup param a bit:
cleanups: A' B
or case 2, where A simply removes its cleanup:
cleanups: B
Case 2 actually happens quite often.
> > IMHO, we need to make subpool scrubbing an actual LIFO cleanup as well, so
> > that will also be true of subpools.
Subpools are entire entities. There shouldn't be any cross-dependencies
between those. Care in ordering isn't necessary.
> > Considering how we use pools for dynamic libraries and the rest, it's
> > absolutely vital that they are unspun from the pool in LIFO order of their
> > creation.
Ah. That is a good example... had forgotten about that one (I even ran into
this once). Using the symbology from above, what happens if B is using code
from a dso that installed A? What happens if A is unloaded and reloaded,
thus rearrange the queue as in case 1.
The point is: I've observed problems with cleanup ordering. And the DSO
thing is particular nasty. At one point, I was considering adjusting
Apache's pool structure to better order when the DSOs were unloaded. For
example, if we loaded DSOs into Pool X and then used subpool Y for all
module-related allocations (e.g. pconf == Y and X is internal).
> I agree with Bill. Having reviewed the code quite deeply yesterday, pool
> cleanups follow a very clean rule, and registering a cleanup from within
> a cleanup will always work if done correctly.
Huh? If you register a cleanup within a cleanup, it *won't* be run. We grab
the list, then run it. Most types of changes to that list won't be seen;
particularly insertions at the front of it.
> If you have data in a pool
> when the cleanup is registered, it is gauranteed to be there when the cleanup
> is run.
The pool data might, but associated resources may not be. For example, the
apr_file_t structure might still be in the pool (simply cuz it can't be
freed), but the underlying file descriptor could be closed.
> Anything else is completely broken.
Depends on your interpretation :-) I think the cleanup behavior is very
well defined, and has certain restrictions on the kinds of things that you
can do in there. And one of those restrictions is watching out for
dependending on something else in your pool. As a result, arbitrary work can
definitely be affected.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
Posted by Ryan Bloom <rb...@covalent.net>.
On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <gs...@lyra.org>
> Sent: Wednesday, September 19, 2001 1:26 PM
>
> > On Wed, Sep 19, 2001 at 01:52:12PM -0400, Rodent of Unusual Size wrote:
> > > Greg Stein wrote:
> > > > It isn't a bug. Cleanups are for just wrapping things up,
> > > > not doing work.
> > >
> > > If that's the authoritative answer, then we need to provide
> > > a supported way for 'doing work' at cleanup time.
> > >
> > > > You might not even be able to open and use that file if
> > > > your pool is n the process of being destroyed.
> > >
> > > That sounds like utter tripe. If you can't depend on the
> > > pool lasting at least until your cleanup routine ends,
> > > then the whole cleanup mechanism is seriously borked. AFAIK
> > > it isn't, so I think the above assertion *is*.
> >
> > The problem is cross-dependency between the cleanup actions. One can
> > destroy something another cleanup needs. If you restrict their actions to
> > "simple" things, then the cross-dependencies are (hopefully!) removed.
>
> Really? No. Cleanups are run as a LIFO stack. Anything that existed when
> something was added to the pool must exist when that something is removed
> from the pool.
>
> IMHO, we need to make subpool scrubbing an actual LIFO cleanup as well, so
> that will also be true of subpools.
>
> Considering how we use pools for dynamic libraries and the rest, it's
> absolutely vital that they are unspun from the pool in LIFO order of their
> creation.
I agree with Bill. Having reviewed the code quite deeply yesterday, pool
cleanups follow a very clean rule, and registering a cleanup from within
a cleanup will always work if done correctly. If you have data in a pool
when the cleanup is registered, it is gauranteed to be there when the cleanup
is run.
Anything else is completely broken.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: New post-log-transaction hook?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Greg Stein" <gs...@lyra.org>
Sent: Wednesday, September 19, 2001 1:26 PM
> On Wed, Sep 19, 2001 at 01:52:12PM -0400, Rodent of Unusual Size wrote:
> > Greg Stein wrote:
> > > It isn't a bug. Cleanups are for just wrapping things up,
> > > not doing work.
> >
> > If that's the authoritative answer, then we need to provide
> > a supported way for 'doing work' at cleanup time.
> >
> > > You might not even be able to open and use that file if
> > > your pool is n the process of being destroyed.
> >
> > That sounds like utter tripe. If you can't depend on the
> > pool lasting at least until your cleanup routine ends,
> > then the whole cleanup mechanism is seriously borked. AFAIK
> > it isn't, so I think the above assertion *is*.
>
> The problem is cross-dependency between the cleanup actions. One can destroy
> something another cleanup needs. If you restrict their actions to "simple"
> things, then the cross-dependencies are (hopefully!) removed.
Really? No. Cleanups are run as a LIFO stack. Anything that existed when
something was added to the pool must exist when that something is removed
from the pool.
IMHO, we need to make subpool scrubbing an actual LIFO cleanup as well, so that
will also be true of subpools.
Considering how we use pools for dynamic libraries and the rest, it's absolutely
vital that they are unspun from the pool in LIFO order of their creation.
Bill
Re: New post-log-transaction hook?
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
"Roy T. Fielding" wrote:
>
> I think that anyone using "tripe" in an analogy should be
> forced to eat it.
Don't go there, Roy, or someone is sure to apply it to
'bullsh*t' as well.. :-D
--
#ken P-)}
Ken Coar, Sanagendamgagwedweinini http://Golux.Com/coar/
Author, developer, opinionist http://Apache-Server.Com/
"All right everyone! Step away from the glowing hamburger!"
Re: New post-log-transaction hook?
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
On Wed, Sep 19, 2001 at 03:42:42PM -0400, Rodent of Unusual Size wrote:
> Greg Stein wrote:
> >
> > p.s. "utter tripe" indeed... that was rather inflammatory...
>
> Sorry, but the whole thrust of your message seemed to be
> 'cleanups can't depend on diddly-squat'. I didn't say it
> *was* tripe, just that it sounded like it. :-)
I think that anyone using "tripe" in an analogy should be forced to eat it.
Ken, the next time you are in the neighborhood, Aaron and I will take you
to his favorite Vietnamese restaurant and order it for you. And, I assure
you, it doesn't sound like much at all.
....Roy
Re: New post-log-transaction hook?
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
Greg Stein wrote:
>
> p.s. "utter tripe" indeed... that was rather inflammatory...
Sorry, but the whole thrust of your message seemed to be
'cleanups can't depend on diddly-squat'. I didn't say it
*was* tripe, just that it sounded like it. :-)
--
#ken P-)}
Ken Coar, Sanagendamgagwedweinini http://Golux.Com/coar/
Author, developer, opinionist http://Apache-Server.Com/
"All right everyone! Step away from the glowing hamburger!"
Re: New post-log-transaction hook?
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Sep 19, 2001 at 01:52:12PM -0400, Rodent of Unusual Size wrote:
> Greg Stein wrote:
> > It isn't a bug. Cleanups are for just wrapping things up,
> > not doing work.
>
> If that's the authoritative answer, then we need to provide
> a supported way for 'doing work' at cleanup time.
>
> > You might not even be able to open and use that file if
> > your pool is n the process of being destroyed.
>
> That sounds like utter tripe. If you can't depend on the
> pool lasting at least until your cleanup routine ends,
> then the whole cleanup mechanism is seriously borked. AFAIK
> it isn't, so I think the above assertion *is*.
The problem is cross-dependency between the cleanup actions. One can destroy
something another cleanup needs. If you restrict their actions to "simple"
things, then the cross-dependencies are (hopefully!) removed.
Consider a platform that creates a subpool to manage a set of file
descriptors for the apr_file_t objects. That subpool is linked from the
userdata of specified pool. At cleanup time, that subpool has been tossed,
so you cannot create files on the given pool.
Or maybe it is as simple as APR registers a cleanup on the pool to do some
cleanup, which clears out some basic structure needed for creating FOO
objects. And that APR's cleanup runs before yours.
The basic issue is that cleanups are generally non-deterministic. A cleanup
could run before another and blast some basic service, preventing the second
cleanup from doing "work". Think about database connection pools, shared
memory stuff, temporary files, etc. The list goes on and on.
That said: if you want to create a second list of "do some work when the
pool is cleared" type thing, then fine. As long as its understood that
*that* list is for non-destructive work, and the cleanups are for
destructive work. The work-list can run before subpools are tossed, which
means the pools are "unchanged".
Cheers,
-g
p.s. "utter tripe" indeed... that was rather inflammatory...
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
Greg Stein wrote:
>
> It isn't a bug. Cleanups are for just wrapping things up,
> not doing work.
If that's the authoritative answer, then we need to provide
a supported way for 'doing work' at cleanup time.
> You might not even be able to open and use that file if
> your pool is n the process of being destroyed.
That sounds like utter tripe. If you can't depend on the
pool lasting at least until your cleanup routine ends,
then the whole cleanup mechanism is seriously borked. AFAIK
it isn't, so I think the above assertion *is*.
--
#ken P-)}
Ken Coar, Sanagendamgagwedweinini http://Golux.Com/coar/
Author, developer, opinionist http://Apache-Server.Com/
"All right everyone! Step away from the glowing hamburger!"
Re: New post-log-transaction hook?
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Sep 19, 2001 at 11:17:36AM -0500, William A. Rowe, Jr. wrote:
> From: "Ryan Bloom" <rb...@covalent.net>
> Sent: Wednesday, September 19, 2001 9:52 AM
>
>
> > On Tuesday 18 September 2001 06:09 pm, Greg Stein wrote:
> > > I agree with OtherBill.
> > >
> > > Cleanups are not always the answer. When they are run, many things
> > > associated with that pool could be torn down already because their cleanups
> > > have already run.
> >
> > Jon is already using a pool cleanup to solve this problem. In fact, his
> > initial thought was to use a pool cleanup, but he couldn't register cleanups
> > from within a cleanup. Adding a new hook was a second solution from Jon,
> > because he just needed to solve his problem.
>
> Note (for those not paying that close attention) just trying to open a file within
> a cleanup, do something, and close it will attempt to create a cleanup :(
>
> This bug still must be fixed.
It isn't a bug. Cleanups are for just wrapping things up, not doing work.
You might not even be able to open and use that file if your pool is in the
process of being destroyed.
Doing significant work at cleanup time, *especially* things which involve
that same pool, is flat out prone to bugs. The cleanups are not ordered
(they shouldn't be), and the fact they are running means that the pool's
usability is marginable. Note that the pool's subpools have already been
destroyed.
It isn't a bug.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Wednesday, September 19, 2001 9:52 AM
> On Tuesday 18 September 2001 06:09 pm, Greg Stein wrote:
> > I agree with OtherBill.
> >
> > Cleanups are not always the answer. When they are run, many things
> > associated with that pool could be torn down already because their cleanups
> > have already run.
>
> Jon is already using a pool cleanup to solve this problem. In fact, his
> initial thought was to use a pool cleanup, but he couldn't register cleanups
> from within a cleanup. Adding a new hook was a second solution from Jon,
> because he just needed to solve his problem.
Note (for those not paying that close attention) just trying to open a file within
a cleanup, do something, and close it will attempt to create a cleanup :(
This bug still must be fixed.
> We are adding things just to add them. I have yet to see a need for this new
> hook.
Agreed - that's why I asked that we fix first, find a use-case afterwards.
Re: New post-log-transaction hook?
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 18 September 2001 06:09 pm, Greg Stein wrote:
> I agree with OtherBill.
>
> Cleanups are not always the answer. When they are run, many things
> associated with that pool could be torn down already because their cleanups
> have already run.
>
> If you need a known state to perform *operations* (as it sounds like Jon is
> doing), then you can't use a cleanup.
>
> Hooks are orderable, but more importantly: they run at a specified time. In
> this case, before the pool is cleaned up.
>
> Lingering close should not be part of the connection pool cleanup. It is a
> specific action that needs to occur *before* we are done with the
> connection pool. That is why Bill's suggestion of running it as a hook is
> great. It also gives people a chance to perform actions relative to that
> close and before pool cleanup occurs.
Jon is already using a pool cleanup to solve this problem. In fact, his
initial thought was to use a pool cleanup, but he couldn't register cleanups
from within a cleanup. Adding a new hook was a second solution from Jon,
because he just needed to solve his problem.
We are adding things just to add them. I have yet to see a need for this new
hook.
Ryan
> On Tue, Sep 18, 2001 at 02:20:35PM -0500, William A. Rowe, Jr. wrote:
> > From: "Ryan Bloom" <rb...@covalent.net>
> > Sent: Tuesday, September 18, 2001 11:44 AM
> >
> > > On Tuesday 18 September 2001 08:17 am, William A. Rowe, Jr. wrote:
> > > > Why not let the MPM register the lingerclose with APR_HOOK_MIDDLE in
> > > > the post_connection hook? That way, if Jon's (or any other author's)
> > > > intent is to work before the lingering close, then it can be
> > > > APR_HOOK_FIRST. Otherwise register it APR_HOOK_LAST.
> > >
> > > It shouldn't be a hook. This should just be done with a pool cleanup.
> > > Hooks aren't the answer to every problem in the server. Doing
> > > something after a specific action, like the close of the connection
> > > should be done by registering a pool cleanup. Fix the bug that you
> > > can't register a cleanup within a cleanup, and Jon's problem goes away
> > > completely, because he can use the cleanup that he is already using.
> >
> > The pool cleanup has one disadvantage (assuming the register cleanup
> > within cleanup bug is fixed), the order of cleanups is a strict LIFO.
> >
> > There _may_ be an advantage to an orderable hook. At this point I agree,
> > fix the register cleanup in cleanup bug, let Jon experiment with that
> > solution, and then argue the merits for a new hook.
> >
> > Bill
--
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: New post-log-transaction hook?
Posted by Greg Stein <gs...@lyra.org>.
I agree with OtherBill.
Cleanups are not always the answer. When they are run, many things
associated with that pool could be torn down already because their cleanups
have already run.
If you need a known state to perform *operations* (as it sounds like Jon is
doing), then you can't use a cleanup.
Hooks are orderable, but more importantly: they run at a specified time. In
this case, before the pool is cleaned up.
Lingering close should not be part of the connection pool cleanup. It is a
specific action that needs to occur *before* we are done with the connection
pool. That is why Bill's suggestion of running it as a hook is great. It
also gives people a chance to perform actions relative to that close and
before pool cleanup occurs.
Cheers,
-g
On Tue, Sep 18, 2001 at 02:20:35PM -0500, William A. Rowe, Jr. wrote:
> From: "Ryan Bloom" <rb...@covalent.net>
> Sent: Tuesday, September 18, 2001 11:44 AM
>
>
> > On Tuesday 18 September 2001 08:17 am, William A. Rowe, Jr. wrote:
> > > Why not let the MPM register the lingerclose with APR_HOOK_MIDDLE in the
> > > post_connection hook? That way, if Jon's (or any other author's) intent is
> > > to work before the lingering close, then it can be APR_HOOK_FIRST.
> > > Otherwise register it APR_HOOK_LAST.
> >
> > It shouldn't be a hook. This should just be done with a pool cleanup. Hooks
> > aren't the answer to every problem in the server. Doing something after a
> > specific action, like the close of the connection should be done by registering
> > a pool cleanup. Fix the bug that you can't register a cleanup within a cleanup,
> > and Jon's problem goes away completely, because he can use the cleanup
> > that he is already using.
>
> The pool cleanup has one disadvantage (assuming the register cleanup within cleanup
> bug is fixed), the order of cleanups is a strict LIFO.
>
> There _may_ be an advantage to an orderable hook. At this point I agree, fix the
> register cleanup in cleanup bug, let Jon experiment with that solution, and then
> argue the merits for a new hook.
>
> Bill
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
Posted by Jon Travis <jt...@covalent.net>.
On Tue, Sep 18, 2001 at 02:20:35PM -0500, William A. Rowe, Jr. wrote:
> From: "Ryan Bloom" <rb...@covalent.net>
> Sent: Tuesday, September 18, 2001 11:44 AM
>
>
> > On Tuesday 18 September 2001 08:17 am, William A. Rowe, Jr. wrote:
> > > Why not let the MPM register the lingerclose with APR_HOOK_MIDDLE in the
> > > post_connection hook? That way, if Jon's (or any other author's) intent is
> > > to work before the lingering close, then it can be APR_HOOK_FIRST.
> > > Otherwise register it APR_HOOK_LAST.
> >
> > It shouldn't be a hook. This should just be done with a pool cleanup. Hooks
> > aren't the answer to every problem in the server. Doing something after a
> > specific action, like the close of the connection should be done by registering
> > a pool cleanup. Fix the bug that you can't register a cleanup within a cleanup,
> > and Jon's problem goes away completely, because he can use the cleanup
> > that he is already using.
>
> The pool cleanup has one disadvantage (assuming the register cleanup within cleanup
> bug is fixed), the order of cleanups is a strict LIFO.
>
> There _may_ be an advantage to an orderable hook. At this point I agree, fix the
> register cleanup in cleanup bug, let Jon experiment with that solution, and then
> argue the merits for a new hook.
I've got a workaround for that right now (which seems to work fine). I
create a new pool within my cleanup, and destroy it before I exit.
-- Jon
Re: New post-log-transaction hook?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Tuesday, September 18, 2001 11:44 AM
> On Tuesday 18 September 2001 08:17 am, William A. Rowe, Jr. wrote:
> > Why not let the MPM register the lingerclose with APR_HOOK_MIDDLE in the
> > post_connection hook? That way, if Jon's (or any other author's) intent is
> > to work before the lingering close, then it can be APR_HOOK_FIRST.
> > Otherwise register it APR_HOOK_LAST.
>
> It shouldn't be a hook. This should just be done with a pool cleanup. Hooks
> aren't the answer to every problem in the server. Doing something after a
> specific action, like the close of the connection should be done by registering
> a pool cleanup. Fix the bug that you can't register a cleanup within a cleanup,
> and Jon's problem goes away completely, because he can use the cleanup
> that he is already using.
The pool cleanup has one disadvantage (assuming the register cleanup within cleanup
bug is fixed), the order of cleanups is a strict LIFO.
There _may_ be an advantage to an orderable hook. At this point I agree, fix the
register cleanup in cleanup bug, let Jon experiment with that solution, and then
argue the merits for a new hook.
Bill
Re: New post-log-transaction hook?
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 18 September 2001 08:17 am, William A. Rowe, Jr. wrote:
> Why not let the MPM register the lingerclose with APR_HOOK_MIDDLE in the
> post_connection hook? That way, if Jon's (or any other author's) intent is
> to work before the lingering close, then it can be APR_HOOK_FIRST.
> Otherwise register it APR_HOOK_LAST.
It shouldn't be a hook. This should just be done with a pool cleanup. Hooks
aren't the answer to every problem in the server. Doing something after a
specific action, like the close of the connection should be done by registering
a pool cleanup. Fix the bug that you can't register a cleanup within a cleanup,
and Jon's problem goes away completely, because he can use the cleanup
that he is already using.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: New post-log-transaction hook?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Why not let the MPM register the lingerclose with APR_HOOK_MIDDLE in the
post_connection hook? That way, if Jon's (or any other author's) intent is
to work before the lingering close, then it can be APR_HOOK_FIRST. Otherwise
register it APR_HOOK_LAST.
Problem solved?
----- Original Message -----
From: "Jeff Trawick" <tr...@attglobal.net>
To: <de...@httpd.apache.org>
Sent: Tuesday, September 18, 2001 5:52 AM
Subject: Re: New post-log-transaction hook?
> Greg Stein <gs...@lyra.org> writes:
>
> > 2) move the ap_lingering_close inside ap_process_connection, then call it
> > from with ap_process_connection. This *almost* works. All MPMs have a
> > call to ap_process_connection followed by a call to ap_lingering_close.
> > The only MPM that does other funny stuff in there is the winnt MPM.
> > Conceivably, you could pass a flag that says "I'll manage the shutdown,
> > thankyouvermuch", and winnt would do its close followed by the
> > post-connection hook.
>
> It is nice for ap_lingering_close() to be handled in the MPM. It
> shouldn't be too hard for the MPM to implement its own lingering close
> in a manner that doesn't consume one thread per closing connection
> (e.g., dedicate one thread per MPM to do all the lingering close
> logic). Of course there could always be another hook :)
>
> --
> Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
> http://www.geocities.com/SiliconValley/Park/9289/
> Born in Roswell... married an alien...
>
Re: New post-log-transaction hook?
Posted by Jeff Trawick <tr...@attglobal.net>.
Greg Stein <gs...@lyra.org> writes:
> 2) move the ap_lingering_close inside ap_process_connection, then call it
> from with ap_process_connection. This *almost* works. All MPMs have a
> call to ap_process_connection followed by a call to ap_lingering_close.
> The only MPM that does other funny stuff in there is the winnt MPM.
> Conceivably, you could pass a flag that says "I'll manage the shutdown,
> thankyouvermuch", and winnt would do its close followed by the
> post-connection hook.
It is nice for ap_lingering_close() to be handled in the MPM. It
shouldn't be too hard for the MPM to implement its own lingering close
in a manner that doesn't consume one thread per closing connection
(e.g., dedicate one thread per MPM to do all the lingering close
logic). Of course there could always be another hook :)
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: New post-log-transaction hook?
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Sep 17, 2001 at 03:52:21PM -0700, Jon Travis wrote:
> I've got a bit of code that needs to run after a connection to a client
> has been closed. Right now I can (kind of) spoof this by setting the
> keepalive for the client to 0, and registering a cleanup on the
> request_req pool. Unfortunately the code in there is somewhat bulky,
> so any subsequent cleanups that it registers will never get called
> (apparently registering a cleanup within a cleanup no workie).
>
> I'd like to propose a new hook which gets run after connection to
> the client has been closed. (in my case, I run some cleanup code
> which can take a while, and would like the client to be on its way).
>
> Any thoughts?
Yes. :-)
You've confused the issue with your subject line (everybody is bugging out
because they're relating it to logging). It should not have anything to do
with "log". We have a pre-connection hook, so call yours post-connection.
That is when you want to run the hook, so we're all set.
Now the question is whether to run it before or after the ap_lingering_close
call. If it is before, then connection.c::ap_process_connection can do it.
If it is after, then there are two options:
1) make all MPMs run the hook
2) move the ap_lingering_close inside ap_process_connection, then call it
from with ap_process_connection. This *almost* works. All MPMs have a
call to ap_process_connection followed by a call to ap_lingering_close.
The only MPM that does other funny stuff in there is the winnt MPM.
Conceivably, you could pass a flag that says "I'll manage the shutdown,
thankyouvermuch", and winnt would do its close followed by the
post-connection hook.
Note that I *would* think that it should run afterwards, to ensure the
client has got all the data and has fully detached.
And all your spoofy nonsense can stay in your module, but it can't go in the
core. That would just be too hard to understand and maintain over the long
haul.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
Posted by Jon Travis <jt...@covalent.net>.
On Mon, Sep 17, 2001 at 07:01:21PM -0400, Cliff Woolley wrote:
> On Mon, 17 Sep 2001, Jon Travis wrote:
>
> > I've got a bit of code that needs to run after a connection to a client
> > has been closed. Right now I can (kind of) spoof this by setting the
> > keepalive for the client to 0, and registering a cleanup on the
> > request_req pool. Unfortunately the code in there is somewhat bulky,
> > so any subsequent cleanups that it registers will never get called
> > (apparently registering a cleanup within a cleanup no workie).
> >
> > I'd like to propose a new hook which gets run after connection to
> > the client has been closed. (in my case, I run some cleanup code
> > which can take a while, and would like the client to be on its way).
>
> Why can't you just register a cleanup on c->pool instead of r->pool?
>
Well, I can register a cleanup on either pool, create a new subpool and
destroy it myself before I return from the cleanup. That would solve
all the problems, but it does seem really hackish (and an abuse of the
cleanups).
-- Jon
Re: New post-log-transaction hook?
Posted by Cliff Woolley <cl...@yahoo.com>.
On Mon, 17 Sep 2001, Jon Travis wrote:
> I've got a bit of code that needs to run after a connection to a client
> has been closed. Right now I can (kind of) spoof this by setting the
> keepalive for the client to 0, and registering a cleanup on the
> request_req pool. Unfortunately the code in there is somewhat bulky,
> so any subsequent cleanups that it registers will never get called
> (apparently registering a cleanup within a cleanup no workie).
>
> I'd like to propose a new hook which gets run after connection to
> the client has been closed. (in my case, I run some cleanup code
> which can take a while, and would like the client to be on its way).
Why can't you just register a cleanup on c->pool instead of r->pool?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA