You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Brian Pane <br...@cnet.com> on 2002/12/20 19:24:18 UTC
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
On Fri, 2002-12-20 at 08:57, rbb@apache.org wrote:
> I am actually pretty sure that allocating brigades out of the
> bucket_allocator is a VERY big mistake. In fact, I am close to asking
> that change to be backed out because it is too dangerous.
>
> When we first designed buckets and bucket brigades, we made one VERY clear
> distinction. Bucket_brigades are allocated out of a pool, because that
> stops us from leaking memory.
Allocating brigades from a pool is often a design mistake.
Brigades tend to outlive the transactions that produce
them (especially in apps like http servers), and having a
brigade disappear as a side-effect of a transaction pool's
cleanup will cause problems elsewhere in the code.
> By allocating bucket_brigades out of the bucket_allocator, we have removed
> the protection that the pool cleanups used to give us.
It's important to keep in mind just what sort of protection
the pool-based brigade allocation offered. It allowed code
that did this:
bb = apr_brigade_create(...);
apr_brigade_destroy(bb);
APR_BRIGADE_INSERT_HEAD(bb, bucket);
to work accidentally. I'm hesitant to call that "protection."
Brian
Re: httpd memory ownership and response pools was Re: [PATCH]
Update to Brian's patch...
Posted by Brian Pane <br...@cnet.com>.
On Sat, 2002-12-21 at 15:50, Justin Erenkrantz wrote:
> --On Saturday, December 21, 2002 10:00 AM -0800 Brian Pane
> <br...@cnet.com> wrote:
>
> > On Sat, 2002-12-21 at 02:05, Justin Erenkrantz wrote:
> >
> >> Saying a response pool 'doubles' the memory is an absolute
> >> non-starter. Back that up, please. If you're meaning the
> >> additional 8kb minimum for the separate pool, I seriously can't
> >> believe that's going to be a major concern.
> >
> > I do consider it a cause for concern as we try to scale
> > the httpd to handle thousands of connections.
>
> And, an additional 8k is going to grossly harm that? Sorry, but I
> don't believe that. If that is your main reason against it, then go
> change the pool code to have a minimum threshold of 4k. Again, I'd
> like to see proof that this will irreparably harm httpd's
> architecture and scalability. (Oh, 4k might kill any alignment
> benefits, but hey, you want to keep our memory footprint constant
> rather than our speed.)
Actually, I don't want to keep the memory footprint constant;
I want to decrease it. Currently, we dedicate at least
24KB (8KB each for request pool, connection pool, and bucket
allocator) to each connection. At 1,000 connnections, that's
24MB of allocated by mostly unused that I'd much rather be
devoting to something useful like the filesystem cache.
> > I have mixed opinions on response pools. They have all the usual
> > benefits of pools, but with a couple of big complications. They're
> > great if they enable you to free up the request pool when you're
> > finished generating the response. But there's data in the request
> > pool that really needs to survive until after the response has been
> > sent--specifically, anything needed in support of the logger. That
>
> Doesn't that mean that that information should be allocated from the
> response pool instead?
Probably, although that's a slippery slope; when you move everything
needed to either deliver or log the response into a response pool,
there may be very little left in the request pool for typical
requests. In that case, it would be easier to just alloc everything
from the request pool, and keep the request pool in existence until
the full response is set.
That's for typical requests, though. For any app that uses a lot of
temporary space when generating a request (I think you had an example
of a module that did this, but I can't remember which one), it would
be beneficial to be able to free up all that working memory as soon
as the generation is finished.
That begs the question, though: how common are such modules? If
it's common to have a large ratio of "request data" to "response
data," then it makes sense to have separate pools. If it's uncommon,
then it's better to have just one pool, to eliminate the risk of
the core or any module every using the wrong pool for a given
allocation--and let the exceptional modules that do need a memory
cleanup pre-response create their own temp pools.
I suspect that modules that need separate request and response pools
are very uncommon, but let me know what you think... Either way,
it would be a big win to be able to keep the pool containing the
response data in existence until we sent all the data allocated
from it; that could eliminate the need for some very brittle bucket
setaside logic.
> > leaves us with the choice of either keeping the request pool around
> > (which somewhat defeats the point of using a response pool) or
> > copying the needed request and connection info to the response pool
> > (which works but is slightly wasteful). The other problem with
>
> I'm not saying this is an easy change - it's a radical departure from
> our current architecture (only suited for 2.1 and beyond). I don't
> think you'd copy anything from the response pool to the request pool.
> We'd change the lifetime of whatever needs to live as long as the
> response instead of the request. The request lifetime ends as soon
> as the EOS is generated. The response lifetime ends as soon as the
> EOS is consumed. Only when we're positive that all code is
> allocating from the correct pools can we switch to destroying the
> request pool when the EOS is seen rather than consumed. And, since
> we're relying on pools, we get error cleanups for free.
>
> > response pools is that they're pools. If you have an app that's
> > generating a large response in small units, you'll have a lot of
> > small brigades being allocated from the response pool, and you
> > won't be able to free up any of that space until the entire
> > response is finished. This is bearable for most HTTP connections,
> > but it won't work for connections that live forever.
>
> Connections that live forever? Don't you mean a response that never
> ends? (If you had a connection that lived forever with multiple
> responses, the memory would be cleaned up as the response is sent.)
No, the problem is that if the brigade is ever copied to a
connection-level pool, it won't get deallocated until the
connection closes. For HTTP, it's bearable. But if we use
brigades for connections that live forever (think app server
or database connections, for example), then we'll have a memory
leak if ever a brigade is allocated from a connection-level pool.
> So, how is your model going to make things appreciably better? The
> only thing you'd gain is that you might be able to reuse a brigade
> sooner.
>
> As seen in our code, we're *trying* to reuse the brigade memory right
> now. So, where is the big benefit of a bucket allocator for brigades
> right now? I don't think we're creating an infinite number of
> brigades. Most of our filters seem to be trying to reuse the
> brigades it creates. It isn't creating a brigade every filter call -
> they are usually doing their best to create only one brigade (perhaps
> two or three - yet a constant number). While the number of buckets
> may be unbounded (since we don't know how large the response is), the
> number of brigades should almost certainly be (near) constant
> (probably as a factor of the number of filters involved).
>
> This is why everything falls down with your patch. The
> core_output_filter shouldn't be destroying a brigade it doesn't own.
> It should only be clearing them. It didn't create them, so it
> shouldn't destroy it.
That's actually independent of my patch. The core_output_filter
has been deleting brigades all along. That's most likely a bug,
given Bill's observation that the code is then reusing brigades
after destroying them; it only works as a side-effect of the
deleted brigade still having space devoted to it in a pool.
> (The only time it should destroy is when it
> setaside the brigade - seems easy enough to fix.) We're already
> reusing the bucket memory, so no big win there.
>
> I have a feeling you want to switch ownership of all memory into an
> implicit ownership by core_output_filter.
More generally, I want to transfer ownership to the entity that
consumes the data, so that we don't require the generator of the
data to be responsible for cleaning it up.
> (I think you were
> expecting this to be occurring right now, but it isn't.)
No, I'm only expecting that for post-2.0. In 2.0, the brigade is
owned by the pool.
> Only
> core_output_filter would be responsible for freeing all memory (note
> that it isn't responsible for allocating memory). However, this
> would require a rewrite of all the code to be built around the
> assumption that if it passes a brigade down, it loses ownership of
> that brigade.
I think we'll have to lose that assumption no matter what in
2.1 and later, because passing a brigade may mean handing it
to another thread.
> Then, you also introduce the problem with brigades
> becoming numerous (parallel to the number of buckets). Hence, we
> must allocate brigades from the bucket allocator rather than a pool.
>
> But, isn't this creating more work for httpd if it were using a
> synchronous MPM rather than an async MPM? Do we really think that
> your ownership model works well for *all* cases? I'm beginning to
> think it doesn't. Is it really cheaper to call the bucket allocator
> every time we pass something down the filter chain than reusing the
> same brigade for the entire duration of the request in a filter? I
> would be suspicious of that claim under a sync MPM.
Reusing the brigade is easy, but I don't think it's likely to be
an option for a filter that needs to work with both sync and async
MPMs.
> Is there a compromise? I think so. A hybrid completion MPM isn't a
> true aysnc MPM. However, the memory ownership model in this hybrid
> could still be identical to the sync MPM. Once we see EOS, we can
> clear the request pool and transfer the request to a 'completion'
> thread/process that multiplexes all finished-not-sent-responses.
> This is why it is crucial to have a response pool in addition to the
> request pool - all information pertaining to the response live in the
> response pool not the request pool. (All intermediate data that
> helps to produce the response is in the request pool, of course.)
I've been a proponent of the hybrid completion model in the past
(see some of my notes in the ROADMAP file), but I think of it as
a stepping-stone to a full async MPM, rather than as something
actually usable.
The problem with the hybrid completion model is that it's too
easy to conduct a DoS against it. If we release an MPM that
reads requests and generates responses in a thread-per-request
pool and then hands off the brigades to an event-loop thread to
do the write, all an attacker needs to do is open enough connections
to fill up the request thread pool (which typically will be a lot
smaller than the thread pool in, say, worker, because the whole
point of the hybrid design is to let the server administrator
run with fewer threads) and never send a request.
> This allows the current sync thread to go back and process a request
> as soon as it can. When this completion thread is done, it can
> simply clear the pool as it is guaranteed ownership. Once the EOS is
> seen, we *know* that all filters are done with the response. So, if
> we implement this hybrid MPM, what benefits are we gaining from
> further switching to a fully async MPM? -- justin
This is also susceptible to a DoS on the write side. If a malicious
client requests a URI that's handled by a streaming generator (like
a CGI) and then never reads the response, the writes will block as
soon as the outgoing TCP buffer fills up. If the writes are being
handled by a multiplexed I/O completion thread at that point, no
problem. But in the case of a streamed response, if the request
handler thread is allowed to do the socket writes until it produces
an EOS, then you'll block the request handler thread.
In order to have a small number of request handler threads without
making the server too vulnerable to such attacks, we'll need to
ensure that the client can't have a huge influence on how much
time a request handler thread (our scarce resource in an aysnc
MPM) spends processing a request. This basically means doing all
the network reads and writes in a non-scarce resouce, such as a
dedicated I/O thread that can multiplex hundreds of sockets.
We'll still face some major design challenges due to the "client
sends a request for a streamed resource and then never reads" case.
We don't want to let the request processing thread generate an
unbounded amount of response data that we have to buffer until
the connection becomes writable or is aborted. That leaves two
choices that I can think of: either make the request processing
thread block if there's too much unsent data on the connection,
or design such modules as state machines that can be set aside
in the output overflow condition and then resumed when the connection
is writable. Either way, it's a nasty problem. For the post-2.0
releases, we may also want to explore ways to partition the
request handling thread pool to keep simple static requests
separate from dynamic requests, so that a server can still
serve static requests even if all the dynamic request threads
are busy.
Brian
Re: httpd memory ownership and response pools was Re: [PATCH]
Update to Brian's patch...
Posted by Justin Erenkrantz <je...@apache.org>.
--On Saturday, December 21, 2002 10:00 AM -0800 Brian Pane
<br...@cnet.com> wrote:
> On Sat, 2002-12-21 at 02:05, Justin Erenkrantz wrote:
>
>> Saying a response pool 'doubles' the memory is an absolute
>> non-starter. Back that up, please. If you're meaning the
>> additional 8kb minimum for the separate pool, I seriously can't
>> believe that's going to be a major concern.
>
> I do consider it a cause for concern as we try to scale
> the httpd to handle thousands of connections.
And, an additional 8k is going to grossly harm that? Sorry, but I
don't believe that. If that is your main reason against it, then go
change the pool code to have a minimum threshold of 4k. Again, I'd
like to see proof that this will irreparably harm httpd's
architecture and scalability. (Oh, 4k might kill any alignment
benefits, but hey, you want to keep our memory footprint constant
rather than our speed.)
> I have mixed opinions on response pools. They have all the usual
> benefits of pools, but with a couple of big complications. They're
> great if they enable you to free up the request pool when you're
> finished generating the response. But there's data in the request
> pool that really needs to survive until after the response has been
> sent--specifically, anything needed in support of the logger. That
Doesn't that mean that that information should be allocated from the
response pool instead?
> leaves us with the choice of either keeping the request pool around
> (which somewhat defeats the point of using a response pool) or
> copying the needed request and connection info to the response pool
> (which works but is slightly wasteful). The other problem with
I'm not saying this is an easy change - it's a radical departure from
our current architecture (only suited for 2.1 and beyond). I don't
think you'd copy anything from the response pool to the request pool.
We'd change the lifetime of whatever needs to live as long as the
response instead of the request. The request lifetime ends as soon
as the EOS is generated. The response lifetime ends as soon as the
EOS is consumed. Only when we're positive that all code is
allocating from the correct pools can we switch to destroying the
request pool when the EOS is seen rather than consumed. And, since
we're relying on pools, we get error cleanups for free.
> response pools is that they're pools. If you have an app that's
> generating a large response in small units, you'll have a lot of
> small brigades being allocated from the response pool, and you
> won't be able to free up any of that space until the entire
> response is finished. This is bearable for most HTTP connections,
> but it won't work for connections that live forever.
Connections that live forever? Don't you mean a response that never
ends? (If you had a connection that lived forever with multiple
responses, the memory would be cleaned up as the response is sent.)
So, how is your model going to make things appreciably better? The
only thing you'd gain is that you might be able to reuse a brigade
sooner.
As seen in our code, we're *trying* to reuse the brigade memory right
now. So, where is the big benefit of a bucket allocator for brigades
right now? I don't think we're creating an infinite number of
brigades. Most of our filters seem to be trying to reuse the
brigades it creates. It isn't creating a brigade every filter call -
they are usually doing their best to create only one brigade (perhaps
two or three - yet a constant number). While the number of buckets
may be unbounded (since we don't know how large the response is), the
number of brigades should almost certainly be (near) constant
(probably as a factor of the number of filters involved).
This is why everything falls down with your patch. The
core_output_filter shouldn't be destroying a brigade it doesn't own.
It should only be clearing them. It didn't create them, so it
shouldn't destroy it. (The only time it should destroy is when it
setaside the brigade - seems easy enough to fix.) We're already
reusing the bucket memory, so no big win there.
I have a feeling you want to switch ownership of all memory into an
implicit ownership by core_output_filter. (I think you were
expecting this to be occurring right now, but it isn't.) Only
core_output_filter would be responsible for freeing all memory (note
that it isn't responsible for allocating memory). However, this
would require a rewrite of all the code to be built around the
assumption that if it passes a brigade down, it loses ownership of
that brigade. Then, you also introduce the problem with brigades
becoming numerous (parallel to the number of buckets). Hence, we
must allocate brigades from the bucket allocator rather than a pool.
But, isn't this creating more work for httpd if it were using a
synchronous MPM rather than an async MPM? Do we really think that
your ownership model works well for *all* cases? I'm beginning to
think it doesn't. Is it really cheaper to call the bucket allocator
every time we pass something down the filter chain than reusing the
same brigade for the entire duration of the request in a filter? I
would be suspicious of that claim under a sync MPM.
Is there a compromise? I think so. A hybrid completion MPM isn't a
true aysnc MPM. However, the memory ownership model in this hybrid
could still be identical to the sync MPM. Once we see EOS, we can
clear the request pool and transfer the request to a 'completion'
thread/process that multiplexes all finished-not-sent-responses.
This is why it is crucial to have a response pool in addition to the
request pool - all information pertaining to the response live in the
response pool not the request pool. (All intermediate data that
helps to produce the response is in the request pool, of course.)
This allows the current sync thread to go back and process a request
as soon as it can. When this completion thread is done, it can
simply clear the pool as it is guaranteed ownership. Once the EOS is
seen, we *know* that all filters are done with the response. So, if
we implement this hybrid MPM, what benefits are we gaining from
further switching to a fully async MPM? -- justin
Re: [PATCH] Update to Brian's patch to allocate brigades out
of the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Sat, 2002-12-21 at 02:05, Justin Erenkrantz wrote:
> Saying a response pool 'doubles' the memory is an absolute
> non-starter. Back that up, please. If you're meaning the additional
> 8kb minimum for the separate pool, I seriously can't believe that's
> going to be a major concern.
I do consider it a cause for concern as we try to scale
the httpd to handle thousands of connections.
> A response pool solves the same problem as allocating the brigade
> from a non-cleanup protected memory allocator, and I believe has
> better long-term advantages for the overall architecture of httpd.
I have mixed opinions on response pools. They have all the usual
benefits of pools, but with a couple of big complications. They're
great if they enable you to free up the request pool when you're
finished generating the response. But there's data in the request
pool that really needs to survive until after the response has been
sent--specifically, anything needed in support of the logger. That
leaves us with the choice of either keeping the request pool around
(which somewhat defeats the point of using a response pool) or
copying the needed request and connection info to the response pool
(which works but is slightly wasteful). The other problem with
response pools is that they're pools. If you have an app that's
generating a large response in small units, you'll have a lot of
small brigades being allocated from the response pool, and you
won't be able to free up any of that space until the entire
response is finished. This is bearable for most HTTP connections,
but it won't work for connections that live forever.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out
of the bucket allocator
Posted by Justin Erenkrantz <je...@apache.org>.
--On Friday, December 20, 2002 12:30 PM -0800 Brian Pane
<br...@cnet.com> wrote:
> especially in the case of a proxy. Creating a "response pool"
> will work, but that doubles the amount of memory that the server
> must devote to a given request (one pool for request, one for
> response).
IMHO, there are a lot of reasons to bite the bullet and start using a
response pool in httpd. It would certainly allow us to improve the
memory use of mod_dav (pet peeve of mine). Switching where the
brigades are allocated isn't going to help mod_dav at all (due to the
way it needs to walk the tree before generating any output). The
only proper solution would be a response pool.
Saying a response pool 'doubles' the memory is an absolute
non-starter. Back that up, please. If you're meaning the additional
8kb minimum for the separate pool, I seriously can't believe that's
going to be a major concern. How is it going to double the memory
when that minimum threshold is exceeded?
A response pool solves the same problem as allocating the brigade
from a non-cleanup protected memory allocator, and I believe has
better long-term advantages for the overall architecture of httpd.
-- justin
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On Fri, 20 Dec 2002, Aaron Bannert wrote:
> On Friday, December 20, 2002, at 01:26 PM, <rb...@apache.org> wrote:
> > The pools are used so that we don't
> > have to track resources this closely, that is, in fact, their main
> > benefit.
>
> They were also added for performance reasons, but I don't know if
> this was a side effect or the main reason. Delaying freelist management
> until after a response gives a huge performance gain from the
> perspective
> of an http client.
yes, performance was a reason in httpd, but in APR, it really came down to
convenience.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On Fri, 20 Dec 2002, Aaron Bannert wrote:
> On Friday, December 20, 2002, at 01:26 PM, <rb...@apache.org> wrote:
> > The pools are used so that we don't
> > have to track resources this closely, that is, in fact, their main
> > benefit.
>
> They were also added for performance reasons, but I don't know if
> this was a side effect or the main reason. Delaying freelist management
> until after a response gives a huge performance gain from the
> perspective
> of an http client.
yes, performance was a reason in httpd, but in APR, it really came down to
convenience.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the bucket allocator
Posted by Aaron Bannert <aa...@clove.org>.
On Friday, December 20, 2002, at 01:26 PM, <rb...@apache.org> wrote:
> The pools are used so that we don't
> have to track resources this closely, that is, in fact, their main
> benefit.
They were also added for performance reasons, but I don't know if
this was a side effect or the main reason. Delaying freelist management
until after a response gives a huge performance gain from the
perspective
of an http client.
my 2c,
-aaron
Re: [PATCH] Update to Brian's patch to allocate brigades out of the bucket allocator
Posted by Aaron Bannert <aa...@clove.org>.
On Friday, December 20, 2002, at 01:26 PM, <rb...@apache.org> wrote:
> The pools are used so that we don't
> have to track resources this closely, that is, in fact, their main
> benefit.
They were also added for performance reasons, but I don't know if
this was a side effect or the main reason. Delaying freelist management
until after a response gives a huge performance gain from the
perspective
of an http client.
my 2c,
-aaron
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 12:19, rbb@apache.org wrote:
>
> > As for this not working when the filter and handler are in different
> > threads, of course it will. As long as the buckets are copied into a
> > brigade that was allocated out of a pool that will still be alive when the
> > filter is called, this will work just fine.
>
> In practice, it's not feasible to copy the buckets into a brigade
> allocated from a new pool. The connection pool isn't generally
> suitable, as it may be a long time before it is cleaned up,
> especially in the case of a proxy. Creating a "response pool"
> will work, but that doubles the amount of memory that the server
> must devote to a given request (one pool for request, one for
> response).
That is why the core_output_filter destroys the bucket_brigade, cleaning
up 90% of the memory used by the brigade (all the buckets) as soon as the
data is written to the network. It is feasible to do the copy, because
nothing else works. Regardless of how the brigade is allocated, the
brigade that is passed to you doesn't belong to you. It belongs to the
filter/handler that originally allocated it, and you can't gaurantee that
it will still be around the next time your filter is called. It is very
simple. If you are going to set-aside data in a filter, you _must_ copy
the data to a brigade that you allocated. Nothing else is gauranteed to
work. That is why copying/merging brigades was made so cheap. It is also
why buckets aren't allocated out of pools.
> > > The change doesn't remove that ability at all. Both my patch
> > > and Bill's variant allow for a pool cleanup to destroy a brigade.
> >
> > Yes, but before it was mandatory, not it is optional. It can't be
> > optional, it will cause a memory leak.
>
> It's wildly inaccurate to say "it will cause a memory leak." A
> more correct characterization is, "if the brigade is not allocated
> from a pool, its deallocation is the responsibility of the
> application." Same semantics as every other non-pool-based
> allocation API ever developed.
It is not inaccurate at all. The current code has a memory leak if you
don't pass a pool to the brigade_create function. You may not see it, but
it is there. Apache is built around pools to remove these types of memory
leaks. Filters, buckets, and bucket_brigades were designed to use pools
to remove these kinds of memory leaks. Your change allows a brigade to be
created that removes this protection. Because the rest of the server
expects brigades to be allocated out of pools, you have created a memory
leak. This isn't hand-waving. As soon as Bill's patch is committed, if
you _ever_ pass NULL into the brigade_create function for the pool
variable, you will have a memory leak. You can fix the leaks, but you
have removed a the memory protection that the filters were designed
around.
> > The change isn't buying you anything at all, especially with Bill's
> > proposed patch. All filters must assume that the bucket_brigade was
> > allocated out of a pool, because otherwise they will be broken if it
> > wasn't.
>
> That's only true for Apache 2.0. For later releases, it may
> be not only unnecessary but inadvisable to have brigades allocated
> from pools. Once you've allocated a brigade from a pool, for
> example, any subsequent split of that brigade will alloc from
> the same pool. If the split happens in a filter that's running
> in thread 2 while the request's handler is still running in thread
> 1, the colliding apr_pallocs will segfault. We almost certainly
> won't want to add a mutex to apr_palloc, because it would ruin
> the performance of lots of apps. But if the brigade allocations
> are done from a bucket allocator, it's quite feasible to add a
> mutex (or possibly a spinlock) into the bucket allocator without
> compromising performance.
There are a lot of solutions to the problem you have described above. The
easiest is to actually pass a pool to the brigade_split function, so that
you know which pool is used to allocate the new brigade. Yes, using the
bucket_allocator is another solution, but it is currently causing seg
faults, and when that problem is fixed it will cause memory leaks. To fix
the memory leaks, you will need to ensure that all brigades are destroyed,
which means that the code will become more complex (if only to track when
brigades are created and destroyed). The pools are used so that we don't
have to track resources this closely, that is, in fact, their main
benefit.
Again, please explain the problem that you are trying to solve in detail
instead of describing your solution. It is just possible that other
people on this list will have an idea that will solve your problem while
not causing the side-effects that the current change will cause.
In the mean-time, I am -0. leaning towards -1 for this change, because of
the side-effects that it has already demonstrated and those that I have
detailed as lurking around the corner. This change actually makes it
harder to write filters, which are complex enough IMNSHO.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 12:19, rbb@apache.org wrote:
>
> > As for this not working when the filter and handler are in different
> > threads, of course it will. As long as the buckets are copied into a
> > brigade that was allocated out of a pool that will still be alive when the
> > filter is called, this will work just fine.
>
> In practice, it's not feasible to copy the buckets into a brigade
> allocated from a new pool. The connection pool isn't generally
> suitable, as it may be a long time before it is cleaned up,
> especially in the case of a proxy. Creating a "response pool"
> will work, but that doubles the amount of memory that the server
> must devote to a given request (one pool for request, one for
> response).
That is why the core_output_filter destroys the bucket_brigade, cleaning
up 90% of the memory used by the brigade (all the buckets) as soon as the
data is written to the network. It is feasible to do the copy, because
nothing else works. Regardless of how the brigade is allocated, the
brigade that is passed to you doesn't belong to you. It belongs to the
filter/handler that originally allocated it, and you can't gaurantee that
it will still be around the next time your filter is called. It is very
simple. If you are going to set-aside data in a filter, you _must_ copy
the data to a brigade that you allocated. Nothing else is gauranteed to
work. That is why copying/merging brigades was made so cheap. It is also
why buckets aren't allocated out of pools.
> > > The change doesn't remove that ability at all. Both my patch
> > > and Bill's variant allow for a pool cleanup to destroy a brigade.
> >
> > Yes, but before it was mandatory, not it is optional. It can't be
> > optional, it will cause a memory leak.
>
> It's wildly inaccurate to say "it will cause a memory leak." A
> more correct characterization is, "if the brigade is not allocated
> from a pool, its deallocation is the responsibility of the
> application." Same semantics as every other non-pool-based
> allocation API ever developed.
It is not inaccurate at all. The current code has a memory leak if you
don't pass a pool to the brigade_create function. You may not see it, but
it is there. Apache is built around pools to remove these types of memory
leaks. Filters, buckets, and bucket_brigades were designed to use pools
to remove these kinds of memory leaks. Your change allows a brigade to be
created that removes this protection. Because the rest of the server
expects brigades to be allocated out of pools, you have created a memory
leak. This isn't hand-waving. As soon as Bill's patch is committed, if
you _ever_ pass NULL into the brigade_create function for the pool
variable, you will have a memory leak. You can fix the leaks, but you
have removed a the memory protection that the filters were designed
around.
> > The change isn't buying you anything at all, especially with Bill's
> > proposed patch. All filters must assume that the bucket_brigade was
> > allocated out of a pool, because otherwise they will be broken if it
> > wasn't.
>
> That's only true for Apache 2.0. For later releases, it may
> be not only unnecessary but inadvisable to have brigades allocated
> from pools. Once you've allocated a brigade from a pool, for
> example, any subsequent split of that brigade will alloc from
> the same pool. If the split happens in a filter that's running
> in thread 2 while the request's handler is still running in thread
> 1, the colliding apr_pallocs will segfault. We almost certainly
> won't want to add a mutex to apr_palloc, because it would ruin
> the performance of lots of apps. But if the brigade allocations
> are done from a bucket allocator, it's quite feasible to add a
> mutex (or possibly a spinlock) into the bucket allocator without
> compromising performance.
There are a lot of solutions to the problem you have described above. The
easiest is to actually pass a pool to the brigade_split function, so that
you know which pool is used to allocate the new brigade. Yes, using the
bucket_allocator is another solution, but it is currently causing seg
faults, and when that problem is fixed it will cause memory leaks. To fix
the memory leaks, you will need to ensure that all brigades are destroyed,
which means that the code will become more complex (if only to track when
brigades are created and destroyed). The pools are used so that we don't
have to track resources this closely, that is, in fact, their main
benefit.
Again, please explain the problem that you are trying to solve in detail
instead of describing your solution. It is just possible that other
people on this list will have an idea that will solve your problem while
not causing the side-effects that the current change will cause.
In the mean-time, I am -0. leaning towards -1 for this change, because of
the side-effects that it has already demonstrated and those that I have
detailed as lurking around the corner. This change actually makes it
harder to write filters, which are complex enough IMNSHO.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 12:19, rbb@apache.org wrote:
> As for this not working when the filter and handler are in different
> threads, of course it will. As long as the buckets are copied into a
> brigade that was allocated out of a pool that will still be alive when the
> filter is called, this will work just fine.
In practice, it's not feasible to copy the buckets into a brigade
allocated from a new pool. The connection pool isn't generally
suitable, as it may be a long time before it is cleaned up,
especially in the case of a proxy. Creating a "response pool"
will work, but that doubles the amount of memory that the server
must devote to a given request (one pool for request, one for
response).
> > The change doesn't remove that ability at all. Both my patch
> > and Bill's variant allow for a pool cleanup to destroy a brigade.
>
> Yes, but before it was mandatory, not it is optional. It can't be
> optional, it will cause a memory leak.
It's wildly inaccurate to say "it will cause a memory leak." A
more correct characterization is, "if the brigade is not allocated
from a pool, its deallocation is the responsibility of the
application." Same semantics as every other non-pool-based
allocation API ever developed.
> The change isn't buying you anything at all, especially with Bill's
> proposed patch. All filters must assume that the bucket_brigade was
> allocated out of a pool, because otherwise they will be broken if it
> wasn't.
That's only true for Apache 2.0. For later releases, it may
be not only unnecessary but inadvisable to have brigades allocated
from pools. Once you've allocated a brigade from a pool, for
example, any subsequent split of that brigade will alloc from
the same pool. If the split happens in a filter that's running
in thread 2 while the request's handler is still running in thread
1, the colliding apr_pallocs will segfault. We almost certainly
won't want to add a mutex to apr_palloc, because it would ruin
the performance of lots of apps. But if the brigade allocations
are done from a bucket allocator, it's quite feasible to add a
mutex (or possibly a spinlock) into the bucket allocator without
compromising performance.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 12:19, rbb@apache.org wrote:
> As for this not working when the filter and handler are in different
> threads, of course it will. As long as the buckets are copied into a
> brigade that was allocated out of a pool that will still be alive when the
> filter is called, this will work just fine.
In practice, it's not feasible to copy the buckets into a brigade
allocated from a new pool. The connection pool isn't generally
suitable, as it may be a long time before it is cleaned up,
especially in the case of a proxy. Creating a "response pool"
will work, but that doubles the amount of memory that the server
must devote to a given request (one pool for request, one for
response).
> > The change doesn't remove that ability at all. Both my patch
> > and Bill's variant allow for a pool cleanup to destroy a brigade.
>
> Yes, but before it was mandatory, not it is optional. It can't be
> optional, it will cause a memory leak.
It's wildly inaccurate to say "it will cause a memory leak." A
more correct characterization is, "if the brigade is not allocated
from a pool, its deallocation is the responsibility of the
application." Same semantics as every other non-pool-based
allocation API ever developed.
> The change isn't buying you anything at all, especially with Bill's
> proposed patch. All filters must assume that the bucket_brigade was
> allocated out of a pool, because otherwise they will be broken if it
> wasn't.
That's only true for Apache 2.0. For later releases, it may
be not only unnecessary but inadvisable to have brigades allocated
from pools. Once you've allocated a brigade from a pool, for
example, any subsequent split of that brigade will alloc from
the same pool. If the split happens in a filter that's running
in thread 2 while the request's handler is still running in thread
1, the colliding apr_pallocs will segfault. We almost certainly
won't want to add a mutex to apr_palloc, because it would ruin
the performance of lots of apps. But if the brigade allocations
are done from a bucket allocator, it's quite feasible to add a
mutex (or possibly a spinlock) into the bucket allocator without
compromising performance.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 11:24, rbb@apache.org wrote:
>
> > Ummmm, I don't believe that it is possible for a brigade to outlive the
> > transaction that created it, especially not when you look at how brigades
> > are used in the web server.
>
> Sure it can. An output brigade created in a subrequest, for
> example, may not be sent until after the subrequest has gone
> out of scope. Currently, we have to set aside the buckets
> into a new brigade with a longer lifetime. That's error-prone
> (based on historical patterns) and adds overhead, but today it
> works. It won't work if the handler and the output filter are
> running in different threads in the future, though.
Please re-read my description. The example of a sub-request does not
display the behavior that you are describing. In all sub-request cases,
before the sub-request is destroyed, the bucket brigade is passed to a
filter that is registered with the original request. That is a
requirement in order for the request to succeed, because otherwise the
brigade would never be sent (it would be sitting in the filter attached to
the sub-request and it would never see the EOS). As all filters have to
assume that the brigade that they were passed will be destroyed before
they are called again, all filters must copy any data they want to save
into their own set-aside brigade. That brigade must be allocated out of a
pool that is defined to still be alive the next time the filter is called.
Does this create some overhead? Yes, we have to move a list of pointers
from one brigade to the next. However, that must happen regardless of how
the brigade is allocated (pool or bucket_allocator). In fact, with Bill's
patch, you have bought nothing at all with your original patch, because
all filters must assume that the bucket_brigade was allocated out of a
pool.
As for this not working when the filter and handler are in different
threads, of course it will. As long as the buckets are copied into a
brigade that was allocated out of a pool that will still be alive when the
filter is called, this will work just fine.
> > Basically, a handler creates a brigade and
> > passes it down the filter stack. Each filter in the line is then allowed
> > to either concatenate that brigade to it's own brigade or keep sending it
> > down the stack. At the bottom of the stack, the brigade must have been
> > generated out of the connection pool, which by definition lives as long as
> > the transaction that created it.
> >
> > Basically, if you received a brigade from a higher filter, you can only
> > assume that it will survive a single trip down the filter stack. That is
> > why all filters must create their own brigade to save data between calls.
> >
> > > > By allocating bucket_brigades out of the bucket_allocator, we have removed
> > > > the protection that the pool cleanups used to give us.
> > >
> > > It's important to keep in mind just what sort of protection
> > > the pool-based brigade allocation offered. It allowed code
> > > that did this:
> > >
> > > bb = apr_brigade_create(...);
> > > apr_brigade_destroy(bb);
> > > APR_BRIGADE_INSERT_HEAD(bb, bucket);
> > >
> > > to work accidentally. I'm hesitant to call that "protection."
> >
> > Give it up, that is a side-effect of pools, and it isn't the protection
> > that we discussed when designing buckets and brigades. The protection
> > that we are talking about is against memory leaks, which this change
> > almost definately opened up in the web server. There is at least one
> > place in the code where we drop a bucket brigade on the floor without
> > cleaning it up, because we rely on the pool_cleanup to do it for us. This
> > change removes that ability.
>
> The change doesn't remove that ability at all. Both my patch
> and Bill's variant allow for a pool cleanup to destroy a brigade.
Yes, but before it was mandatory, not it is optional. It can't be
optional, it will cause a memory leak.
The change isn't buying you anything at all, especially with Bill's
proposed patch. All filters must assume that the bucket_brigade was
allocated out of a pool, because otherwise they will be broken if it
wasn't.
Please post details about why allocating bucket brigades out of a pool
isn't working for you.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 11:24, rbb@apache.org wrote:
>
> > Ummmm, I don't believe that it is possible for a brigade to outlive the
> > transaction that created it, especially not when you look at how brigades
> > are used in the web server.
>
> Sure it can. An output brigade created in a subrequest, for
> example, may not be sent until after the subrequest has gone
> out of scope. Currently, we have to set aside the buckets
> into a new brigade with a longer lifetime. That's error-prone
> (based on historical patterns) and adds overhead, but today it
> works. It won't work if the handler and the output filter are
> running in different threads in the future, though.
Please re-read my description. The example of a sub-request does not
display the behavior that you are describing. In all sub-request cases,
before the sub-request is destroyed, the bucket brigade is passed to a
filter that is registered with the original request. That is a
requirement in order for the request to succeed, because otherwise the
brigade would never be sent (it would be sitting in the filter attached to
the sub-request and it would never see the EOS). As all filters have to
assume that the brigade that they were passed will be destroyed before
they are called again, all filters must copy any data they want to save
into their own set-aside brigade. That brigade must be allocated out of a
pool that is defined to still be alive the next time the filter is called.
Does this create some overhead? Yes, we have to move a list of pointers
from one brigade to the next. However, that must happen regardless of how
the brigade is allocated (pool or bucket_allocator). In fact, with Bill's
patch, you have bought nothing at all with your original patch, because
all filters must assume that the bucket_brigade was allocated out of a
pool.
As for this not working when the filter and handler are in different
threads, of course it will. As long as the buckets are copied into a
brigade that was allocated out of a pool that will still be alive when the
filter is called, this will work just fine.
> > Basically, a handler creates a brigade and
> > passes it down the filter stack. Each filter in the line is then allowed
> > to either concatenate that brigade to it's own brigade or keep sending it
> > down the stack. At the bottom of the stack, the brigade must have been
> > generated out of the connection pool, which by definition lives as long as
> > the transaction that created it.
> >
> > Basically, if you received a brigade from a higher filter, you can only
> > assume that it will survive a single trip down the filter stack. That is
> > why all filters must create their own brigade to save data between calls.
> >
> > > > By allocating bucket_brigades out of the bucket_allocator, we have removed
> > > > the protection that the pool cleanups used to give us.
> > >
> > > It's important to keep in mind just what sort of protection
> > > the pool-based brigade allocation offered. It allowed code
> > > that did this:
> > >
> > > bb = apr_brigade_create(...);
> > > apr_brigade_destroy(bb);
> > > APR_BRIGADE_INSERT_HEAD(bb, bucket);
> > >
> > > to work accidentally. I'm hesitant to call that "protection."
> >
> > Give it up, that is a side-effect of pools, and it isn't the protection
> > that we discussed when designing buckets and brigades. The protection
> > that we are talking about is against memory leaks, which this change
> > almost definately opened up in the web server. There is at least one
> > place in the code where we drop a bucket brigade on the floor without
> > cleaning it up, because we rely on the pool_cleanup to do it for us. This
> > change removes that ability.
>
> The change doesn't remove that ability at all. Both my patch
> and Bill's variant allow for a pool cleanup to destroy a brigade.
Yes, but before it was mandatory, not it is optional. It can't be
optional, it will cause a memory leak.
The change isn't buying you anything at all, especially with Bill's
proposed patch. All filters must assume that the bucket_brigade was
allocated out of a pool, because otherwise they will be broken if it
wasn't.
Please post details about why allocating bucket brigades out of a pool
isn't working for you.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 11:24, rbb@apache.org wrote:
> Ummmm, I don't believe that it is possible for a brigade to outlive the
> transaction that created it, especially not when you look at how brigades
> are used in the web server.
Sure it can. An output brigade created in a subrequest, for
example, may not be sent until after the subrequest has gone
out of scope. Currently, we have to set aside the buckets
into a new brigade with a longer lifetime. That's error-prone
(based on historical patterns) and adds overhead, but today it
works. It won't work if the handler and the output filter are
running in different threads in the future, though.
> Basically, a handler creates a brigade and
> passes it down the filter stack. Each filter in the line is then allowed
> to either concatenate that brigade to it's own brigade or keep sending it
> down the stack. At the bottom of the stack, the brigade must have been
> generated out of the connection pool, which by definition lives as long as
> the transaction that created it.
>
> Basically, if you received a brigade from a higher filter, you can only
> assume that it will survive a single trip down the filter stack. That is
> why all filters must create their own brigade to save data between calls.
>
> > > By allocating bucket_brigades out of the bucket_allocator, we have removed
> > > the protection that the pool cleanups used to give us.
> >
> > It's important to keep in mind just what sort of protection
> > the pool-based brigade allocation offered. It allowed code
> > that did this:
> >
> > bb = apr_brigade_create(...);
> > apr_brigade_destroy(bb);
> > APR_BRIGADE_INSERT_HEAD(bb, bucket);
> >
> > to work accidentally. I'm hesitant to call that "protection."
>
> Give it up, that is a side-effect of pools, and it isn't the protection
> that we discussed when designing buckets and brigades. The protection
> that we are talking about is against memory leaks, which this change
> almost definately opened up in the web server. There is at least one
> place in the code where we drop a bucket brigade on the floor without
> cleaning it up, because we rely on the pool_cleanup to do it for us. This
> change removes that ability.
The change doesn't remove that ability at all. Both my patch
and Bill's variant allow for a pool cleanup to destroy a brigade.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 11:24, rbb@apache.org wrote:
> Ummmm, I don't believe that it is possible for a brigade to outlive the
> transaction that created it, especially not when you look at how brigades
> are used in the web server.
Sure it can. An output brigade created in a subrequest, for
example, may not be sent until after the subrequest has gone
out of scope. Currently, we have to set aside the buckets
into a new brigade with a longer lifetime. That's error-prone
(based on historical patterns) and adds overhead, but today it
works. It won't work if the handler and the output filter are
running in different threads in the future, though.
> Basically, a handler creates a brigade and
> passes it down the filter stack. Each filter in the line is then allowed
> to either concatenate that brigade to it's own brigade or keep sending it
> down the stack. At the bottom of the stack, the brigade must have been
> generated out of the connection pool, which by definition lives as long as
> the transaction that created it.
>
> Basically, if you received a brigade from a higher filter, you can only
> assume that it will survive a single trip down the filter stack. That is
> why all filters must create their own brigade to save data between calls.
>
> > > By allocating bucket_brigades out of the bucket_allocator, we have removed
> > > the protection that the pool cleanups used to give us.
> >
> > It's important to keep in mind just what sort of protection
> > the pool-based brigade allocation offered. It allowed code
> > that did this:
> >
> > bb = apr_brigade_create(...);
> > apr_brigade_destroy(bb);
> > APR_BRIGADE_INSERT_HEAD(bb, bucket);
> >
> > to work accidentally. I'm hesitant to call that "protection."
>
> Give it up, that is a side-effect of pools, and it isn't the protection
> that we discussed when designing buckets and brigades. The protection
> that we are talking about is against memory leaks, which this change
> almost definately opened up in the web server. There is at least one
> place in the code where we drop a bucket brigade on the floor without
> cleaning it up, because we rely on the pool_cleanup to do it for us. This
> change removes that ability.
The change doesn't remove that ability at all. Both my patch
and Bill's variant allow for a pool cleanup to destroy a brigade.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out
of the bucket allocator
Posted by Justin Erenkrantz <je...@apache.org>.
--On Friday, December 20, 2002 9:09 PM -0600 "William A. Rowe, Jr."
<wr...@rowe-clan.net> wrote:
> I suggest we consider the benefits of pulling buckets/brigades from
> apr-util with the 1.0 release of the apr library, and move that
> code back into httpd if we plan to keep abusing it this way.
Serf uses buckets and brigades.
While I know that Cliff, Aaron, and Greg keep mumbling about
rewriting buckets/brigades solely for serf, they've done nothing so
far. I don't think we even came to an agreement on how to model it
correctly.
So, if you yank buckets from apr-util right now, serf is in a world
of hurt. I'd probably be forced to duplicate the code to get it to
build again. Ick. -- justin
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by rb...@apache.org.
On Fri, 20 Dec 2002, William A. Rowe, Jr. wrote:
> At 08:33 PM 12/20/2002, Brian Pane wrote:
> >On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> >
> >> > Basically, if you received a brigade from a higher filter, you can only
> >> > assume that it will survive a single trip down the filter stack.
> >
> >Right. And you also can't assume that the context that passed
> >the brigade down the filter stack still exists by the time the
> >brigade reaches the last filter. It's a safe assumption in
> >httpd-2.0, where the brigade is passed as a synchronous function
> >call, but it's not valid for apps in general (potentially including
> >future Apache versions) where the brigade may be queued up for
> >asynchronous processing by an I/O completion thread.
>
> No, it simply will have to be valid. Not that the context will be
> bound to a specific thread (it won't), and not that the content will
> be living in a given context.
Please read my response to Brian, it isn't a valid assumption in
httpd-2.0.
> Bottom line, we can't break it till we push out APR 1.0. This leaves
> a very useful question. Do buckets/brigades remain in apr-util with
> the release of 1.0? We've just proven that the concept is both very
> apr and very httpd centric. There is no reason we should burden
> apr with the 'learning curve' of redesigning this code.
>
> I suggest we consider the benefits of pulling buckets/brigades from
> apr-util with the 1.0 release of the apr library, and move that code
> back into httpd if we plan to keep abusing it this way.
>
> apr and apr-util should be repositories for proven concepts, if not
> always proven code. If we are still redesigning, something is wrong.
I consider this an incorrect statement. New concepts should be welcome
anywhere. My question is more basic. Is the bucket/brigade concept
useful outside of the web server/web client libraries that are being
discussed? If so, then they should stay where they are. If not, then
move 'em back into the httpd project.
Of course, I don't believe that apr-util is a useful library currently, so
that could be why I am against keeping buckets and brigades in it. That
library won't be useful until it has more holding it together than "These
are concepts that were useful in Apache, so we exported them".
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by rb...@apache.org.
On Fri, 20 Dec 2002, William A. Rowe, Jr. wrote:
> At 08:33 PM 12/20/2002, Brian Pane wrote:
> >On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> >
> >> > Basically, if you received a brigade from a higher filter, you can only
> >> > assume that it will survive a single trip down the filter stack.
> >
> >Right. And you also can't assume that the context that passed
> >the brigade down the filter stack still exists by the time the
> >brigade reaches the last filter. It's a safe assumption in
> >httpd-2.0, where the brigade is passed as a synchronous function
> >call, but it's not valid for apps in general (potentially including
> >future Apache versions) where the brigade may be queued up for
> >asynchronous processing by an I/O completion thread.
>
> No, it simply will have to be valid. Not that the context will be
> bound to a specific thread (it won't), and not that the content will
> be living in a given context.
Please read my response to Brian, it isn't a valid assumption in
httpd-2.0.
> Bottom line, we can't break it till we push out APR 1.0. This leaves
> a very useful question. Do buckets/brigades remain in apr-util with
> the release of 1.0? We've just proven that the concept is both very
> apr and very httpd centric. There is no reason we should burden
> apr with the 'learning curve' of redesigning this code.
>
> I suggest we consider the benefits of pulling buckets/brigades from
> apr-util with the 1.0 release of the apr library, and move that code
> back into httpd if we plan to keep abusing it this way.
>
> apr and apr-util should be repositories for proven concepts, if not
> always proven code. If we are still redesigning, something is wrong.
I consider this an incorrect statement. New concepts should be welcome
anywhere. My question is more basic. Is the bucket/brigade concept
useful outside of the web server/web client libraries that are being
discussed? If so, then they should stay where they are. If not, then
move 'em back into the httpd project.
Of course, I don't believe that apr-util is a useful library currently, so
that could be why I am against keeping buckets and brigades in it. That
library won't be useful until it has more holding it together than "These
are concepts that were useful in Apache, so we exported them".
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out
of the bucket allocator
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 08:33 PM 12/20/2002, Brian Pane wrote:
>On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
>
>> > Basically, if you received a brigade from a higher filter, you can only
>> > assume that it will survive a single trip down the filter stack.
>
>Right. And you also can't assume that the context that passed
>the brigade down the filter stack still exists by the time the
>brigade reaches the last filter. It's a safe assumption in
>httpd-2.0, where the brigade is passed as a synchronous function
>call, but it's not valid for apps in general (potentially including
>future Apache versions) where the brigade may be queued up for
>asynchronous processing by an I/O completion thread.
No, it simply will have to be valid. Not that the context will be
bound to a specific thread (it won't), and not that the content will
be living in a given context.
However, if the core output filter *chooses* to set aside the brigade
list (not the data, remember, but the brigade list itself), then it will
have to have some context to stash that in. So we will end up with
n connections and no less than n pools. But we may only service
those pools with n/4 threads, depending on the economies.
If we are changing ownership assumptions, that breaks existing
modules. If we are breaking 3rd party modules, we are back to the
2.x debates. However, the scope of this change requires that the
old code is broken.
So that leaves us with the obvious course, if we are going to start
rethinking this from scratch (I don't see a reason too, but...) it better
be with the newer version only (way too many segfault reports this
week from all corners.)
Bottom line, we can't break it till we push out APR 1.0. This leaves
a very useful question. Do buckets/brigades remain in apr-util with
the release of 1.0? We've just proven that the concept is both very
apr and very httpd centric. There is no reason we should burden
apr with the 'learning curve' of redesigning this code.
I suggest we consider the benefits of pulling buckets/brigades from
apr-util with the 1.0 release of the apr library, and move that code
back into httpd if we plan to keep abusing it this way.
apr and apr-util should be repositories for proven concepts, if not
always proven code. If we are still redesigning, something is wrong.
Bill
Re: [PATCH] Update to Brian's patch to allocate brigades out
of the bucket allocator
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 08:33 PM 12/20/2002, Brian Pane wrote:
>On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
>
>> > Basically, if you received a brigade from a higher filter, you can only
>> > assume that it will survive a single trip down the filter stack.
>
>Right. And you also can't assume that the context that passed
>the brigade down the filter stack still exists by the time the
>brigade reaches the last filter. It's a safe assumption in
>httpd-2.0, where the brigade is passed as a synchronous function
>call, but it's not valid for apps in general (potentially including
>future Apache versions) where the brigade may be queued up for
>asynchronous processing by an I/O completion thread.
No, it simply will have to be valid. Not that the context will be
bound to a specific thread (it won't), and not that the content will
be living in a given context.
However, if the core output filter *chooses* to set aside the brigade
list (not the data, remember, but the brigade list itself), then it will
have to have some context to stash that in. So we will end up with
n connections and no less than n pools. But we may only service
those pools with n/4 threads, depending on the economies.
If we are changing ownership assumptions, that breaks existing
modules. If we are breaking 3rd party modules, we are back to the
2.x debates. However, the scope of this change requires that the
old code is broken.
So that leaves us with the obvious course, if we are going to start
rethinking this from scratch (I don't see a reason too, but...) it better
be with the newer version only (way too many segfault reports this
week from all corners.)
Bottom line, we can't break it till we push out APR 1.0. This leaves
a very useful question. Do buckets/brigades remain in apr-util with
the release of 1.0? We've just proven that the concept is both very
apr and very httpd centric. There is no reason we should burden
apr with the 'learning curve' of redesigning this code.
I suggest we consider the benefits of pulling buckets/brigades from
apr-util with the 1.0 release of the apr library, and move that code
back into httpd if we plan to keep abusing it this way.
apr and apr-util should be repositories for proven concepts, if not
always proven code. If we are still redesigning, something is wrong.
Bill
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
> > What
> > you've done is said "The brigade may not live long enough", but you
> > haven't explained how that is the case.
>
> This, too, is something that we've discussed already. Request
> cleanups can happen asynchronously relative to transmission of
> brigades created from the request. Having a pool cleanup
> trigger the destruction of a brigade in another thread is
> dangerous. The canonical solution is to migrate the data to
> a new brigade outside the pool before handing it off to another
> thread, which is fine as long as that doesn't create a need for
> another pool in the application (for the reasons noted above).
You are talking about an application that is built around pools. If you
want an async MPM that uses a single thread to write data to the network
and multiple threads to generate the data, then your network I/O thread
will need to have a pool. This pool will be very long lived (as long as
the process is alive). You can allocate the brigade when the server
starts, and just re-use it forever. You can have a pool of brigades that
are continuosly re-used, or you can create a sub-pool for each call into
your network write function. But, moving brigade allocation outside of
pools isn't solving your problem and it is opening huge holes in the
abstraction that was created.
Screw it. Buckets and bucket brigades are being moved into a project that
I no longer have any desire to work on. My veto stands for as long as the
code is in an APR project. The change is wrong and it is dangerous. I
have explained why in detail.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
> > What
> > you've done is said "The brigade may not live long enough", but you
> > haven't explained how that is the case.
>
> This, too, is something that we've discussed already. Request
> cleanups can happen asynchronously relative to transmission of
> brigades created from the request. Having a pool cleanup
> trigger the destruction of a brigade in another thread is
> dangerous. The canonical solution is to migrate the data to
> a new brigade outside the pool before handing it off to another
> thread, which is fine as long as that doesn't create a need for
> another pool in the application (for the reasons noted above).
You are talking about an application that is built around pools. If you
want an async MPM that uses a single thread to write data to the network
and multiple threads to generate the data, then your network I/O thread
will need to have a pool. This pool will be very long lived (as long as
the process is alive). You can allocate the brigade when the server
starts, and just re-use it forever. You can have a pool of brigades that
are continuosly re-used, or you can create a sub-pool for each call into
your network write function. But, moving brigade allocation outside of
pools isn't solving your problem and it is opening huge holes in the
abstraction that was created.
Screw it. Buckets and bucket brigades are being moved into a project that
I no longer have any desire to work on. My veto stands for as long as the
code is in an APR project. The change is wrong and it is dangerous. I
have explained why in detail.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Sat, 2002-12-21 at 09:52, rbb@apache.org wrote:
> No, that is a solution, in fact, it was part of the original design.
And we've discussed already, it's a flaw in the original design,
because it requires that the app either maintain a long-lived
connection-level pool (with unbounded memory usage) or create and
clean up per-response pools (with performance and memory overhead).
> What
> you've done is said "The brigade may not live long enough", but you
> haven't explained how that is the case.
This, too, is something that we've discussed already. Request
cleanups can happen asynchronously relative to transmission of
brigades created from the request. Having a pool cleanup
trigger the destruction of a brigade in another thread is
dangerous. The canonical solution is to migrate the data to
a new brigade outside the pool before handing it off to another
thread, which is fine as long as that doesn't create a need for
another pool in the application (for the reasons noted above).
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Sat, 2002-12-21 at 09:52, rbb@apache.org wrote:
> No, that is a solution, in fact, it was part of the original design.
And we've discussed already, it's a flaw in the original design,
because it requires that the app either maintain a long-lived
connection-level pool (with unbounded memory usage) or create and
clean up per-response pools (with performance and memory overhead).
> What
> you've done is said "The brigade may not live long enough", but you
> haven't explained how that is the case.
This, too, is something that we've discussed already. Request
cleanups can happen asynchronously relative to transmission of
brigades created from the request. Having a pool cleanup
trigger the destruction of a brigade in another thread is
dangerous. The canonical solution is to migrate the data to
a new brigade outside the pool before handing it off to another
thread, which is fine as long as that doesn't create a need for
another pool in the application (for the reasons noted above).
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 21 Dec 2002, Brian Pane wrote:
> On Sat, 2002-12-21 at 07:58, rbb@apache.org wrote:
>
> > I'm really sorry, but I am sick of hearing this isn't a solution for apps
> > in general. This is now the fourth time that I have asked for a clear
> > description of _why_. I helped to design and write buckets and
> > bucket_brigades. I understand them incredibly well. You seem to be
> > unable or unwilling to give a clear description of the problem you are
> > having. All you are doing is hand-waving saying that this might be a
> > problem for some apps. Give a clear description please.
>
> Sorry, but that's just bogus. I've described repeatedly, and in
> detail, the reasons why we can't make bucket allocation dependent
> on pools. You keep posting objections along the lines of "but we
> can just move a brigade into a pool of appropriate scope," when
> you know that's not a solution.
No, that is a solution, in fact, it was part of the original design. What
you've done is said "The brigade may not live long enough", but you
haven't explained how that is the case. You also haven't explained why
moving the buckets to another brigade won't work. Which I have a very
hard time believing, because that is how Apache works today. You also
haven't answered the problem that even _WITH_ this change, you still need
to move the buckets to a new brigade. At the end of the day, this change
doesn't get you anything, because you _still_ have to do the brigade
migration that you say you can't do.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 21 Dec 2002, Brian Pane wrote:
> On Sat, 2002-12-21 at 07:58, rbb@apache.org wrote:
>
> > I'm really sorry, but I am sick of hearing this isn't a solution for apps
> > in general. This is now the fourth time that I have asked for a clear
> > description of _why_. I helped to design and write buckets and
> > bucket_brigades. I understand them incredibly well. You seem to be
> > unable or unwilling to give a clear description of the problem you are
> > having. All you are doing is hand-waving saying that this might be a
> > problem for some apps. Give a clear description please.
>
> Sorry, but that's just bogus. I've described repeatedly, and in
> detail, the reasons why we can't make bucket allocation dependent
> on pools. You keep posting objections along the lines of "but we
> can just move a brigade into a pool of appropriate scope," when
> you know that's not a solution.
No, that is a solution, in fact, it was part of the original design. What
you've done is said "The brigade may not live long enough", but you
haven't explained how that is the case. You also haven't explained why
moving the buckets to another brigade won't work. Which I have a very
hard time believing, because that is how Apache works today. You also
haven't answered the problem that even _WITH_ this change, you still need
to move the buckets to a new brigade. At the end of the day, this change
doesn't get you anything, because you _still_ have to do the brigade
migration that you say you can't do.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Sat, 2002-12-21 at 07:58, rbb@apache.org wrote:
> I'm really sorry, but I am sick of hearing this isn't a solution for apps
> in general. This is now the fourth time that I have asked for a clear
> description of _why_. I helped to design and write buckets and
> bucket_brigades. I understand them incredibly well. You seem to be
> unable or unwilling to give a clear description of the problem you are
> having. All you are doing is hand-waving saying that this might be a
> problem for some apps. Give a clear description please.
Sorry, but that's just bogus. I've described repeatedly, and in
detail, the reasons why we can't make bucket allocation dependent
on pools. You keep posting objections along the lines of "but we
can just move a brigade into a pool of appropriate scope," when
you know that's not a solution.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Sat, 2002-12-21 at 07:58, rbb@apache.org wrote:
> I'm really sorry, but I am sick of hearing this isn't a solution for apps
> in general. This is now the fourth time that I have asked for a clear
> description of _why_. I helped to design and write buckets and
> bucket_brigades. I understand them incredibly well. You seem to be
> unable or unwilling to give a clear description of the problem you are
> having. All you are doing is hand-waving saying that this might be a
> problem for some apps. Give a clear description please.
Sorry, but that's just bogus. I've described repeatedly, and in
detail, the reasons why we can't make bucket allocation dependent
on pools. You keep posting objections along the lines of "but we
can just move a brigade into a pool of appropriate scope," when
you know that's not a solution.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 21:37, rbb@apache.org wrote:
> > On 20 Dec 2002, Brian Pane wrote:
> >
> > > On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> > >
> > > > > Basically, if you received a brigade from a higher filter, you can only
> > > > > assume that it will survive a single trip down the filter stack.
> > >
> > > Right. And you also can't assume that the context that passed
> > > the brigade down the filter stack still exists by the time the
> > > brigade reaches the last filter. It's a safe assumption in
> > > httpd-2.0, where the brigade is passed as a synchronous function
> > > call, but it's not valid for apps in general (potentially including
> > > future Apache versions) where the brigade may be queued up for
> > > asynchronous processing by an I/O completion thread.
> >
> > Actually, it's not even a safe assumption in httpd-2.0. You have data
> > from requests that originally lived in brigades allocated out of the
> > request pool moved to brigades allocated out of the connection pool
> > because the request pool is going away. But that just proves the point,
> > the data must be moved from one brigade to another to change it's
> > lifetime. That is part of the design of buckets and brigades. It is why
> > the concat and split operations were written for maximum performance.
> >
> > We are talking about a couple of pointer assignments for the concatenation
> > and a malloc and some pointer assignments for the split.
>
> And you're also talking about needing a pool from which you
> can do the alloc. With pool-based brigade allocation, there
> are two basic choices when allocating a new brigade during
> the split, and they both have problems:
Um, you need a pool that lives at least as long as your filter. But that
should make sense, because if you are going to do any memory allocation,
you will need a pool in your filter.
> - Allocate from a connection pool. This is what httpd-2.0
> does, but it's not a robust solution, given that connections
> can be very long-lived. And for non-http connections, like
> an app server connector, connections may live forever, so
> doing per-request allocations from a connection-level pool
> guarantees a memory leak.
No, httpd-2.0 only uses the connection pool in filters that live as long
as the connection. If that scope doesn't make sense, then you have to
clear the brigade when you are done with it. Remember, we are talking
about a couple of bytes for the size of the brigade. Even if it does leak
until the end of the connection, it isn't a very big leak. It is a huge
leak if you don't clear the brigade, but that is why we have
brigade_clear.
> Pools just aren't a good match for the allocation needs of
> brigades. We happen to have been able to hack around many
> of the problems through liberal use of the "copy the brigade
> into another pool with a more appropriate scope" technique
> in httpd-2.0, but that's not a solution for apps in general.
I'm really sorry, but I am sick of hearing this isn't a solution for apps
in general. This is now the fourth time that I have asked for a clear
description of _why_. I helped to design and write buckets and
bucket_brigades. I understand them incredibly well. You seem to be
unable or unwilling to give a clear description of the problem you are
having. All you are doing is hand-waving saying that this might be a
problem for some apps. Give a clear description please.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 21:37, rbb@apache.org wrote:
> > On 20 Dec 2002, Brian Pane wrote:
> >
> > > On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> > >
> > > > > Basically, if you received a brigade from a higher filter, you can only
> > > > > assume that it will survive a single trip down the filter stack.
> > >
> > > Right. And you also can't assume that the context that passed
> > > the brigade down the filter stack still exists by the time the
> > > brigade reaches the last filter. It's a safe assumption in
> > > httpd-2.0, where the brigade is passed as a synchronous function
> > > call, but it's not valid for apps in general (potentially including
> > > future Apache versions) where the brigade may be queued up for
> > > asynchronous processing by an I/O completion thread.
> >
> > Actually, it's not even a safe assumption in httpd-2.0. You have data
> > from requests that originally lived in brigades allocated out of the
> > request pool moved to brigades allocated out of the connection pool
> > because the request pool is going away. But that just proves the point,
> > the data must be moved from one brigade to another to change it's
> > lifetime. That is part of the design of buckets and brigades. It is why
> > the concat and split operations were written for maximum performance.
> >
> > We are talking about a couple of pointer assignments for the concatenation
> > and a malloc and some pointer assignments for the split.
>
> And you're also talking about needing a pool from which you
> can do the alloc. With pool-based brigade allocation, there
> are two basic choices when allocating a new brigade during
> the split, and they both have problems:
Um, you need a pool that lives at least as long as your filter. But that
should make sense, because if you are going to do any memory allocation,
you will need a pool in your filter.
> - Allocate from a connection pool. This is what httpd-2.0
> does, but it's not a robust solution, given that connections
> can be very long-lived. And for non-http connections, like
> an app server connector, connections may live forever, so
> doing per-request allocations from a connection-level pool
> guarantees a memory leak.
No, httpd-2.0 only uses the connection pool in filters that live as long
as the connection. If that scope doesn't make sense, then you have to
clear the brigade when you are done with it. Remember, we are talking
about a couple of bytes for the size of the brigade. Even if it does leak
until the end of the connection, it isn't a very big leak. It is a huge
leak if you don't clear the brigade, but that is why we have
brigade_clear.
> Pools just aren't a good match for the allocation needs of
> brigades. We happen to have been able to hack around many
> of the problems through liberal use of the "copy the brigade
> into another pool with a more appropriate scope" technique
> in httpd-2.0, but that's not a solution for apps in general.
I'm really sorry, but I am sick of hearing this isn't a solution for apps
in general. This is now the fourth time that I have asked for a clear
description of _why_. I helped to design and write buckets and
bucket_brigades. I understand them incredibly well. You seem to be
unable or unwilling to give a clear description of the problem you are
having. All you are doing is hand-waving saying that this might be a
problem for some apps. Give a clear description please.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 21:37, rbb@apache.org wrote:
> On 20 Dec 2002, Brian Pane wrote:
>
> > On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> >
> > > > Basically, if you received a brigade from a higher filter, you can only
> > > > assume that it will survive a single trip down the filter stack.
> >
> > Right. And you also can't assume that the context that passed
> > the brigade down the filter stack still exists by the time the
> > brigade reaches the last filter. It's a safe assumption in
> > httpd-2.0, where the brigade is passed as a synchronous function
> > call, but it's not valid for apps in general (potentially including
> > future Apache versions) where the brigade may be queued up for
> > asynchronous processing by an I/O completion thread.
>
> Actually, it's not even a safe assumption in httpd-2.0. You have data
> from requests that originally lived in brigades allocated out of the
> request pool moved to brigades allocated out of the connection pool
> because the request pool is going away. But that just proves the point,
> the data must be moved from one brigade to another to change it's
> lifetime. That is part of the design of buckets and brigades. It is why
> the concat and split operations were written for maximum performance.
>
> We are talking about a couple of pointer assignments for the concatenation
> and a malloc and some pointer assignments for the split.
And you're also talking about needing a pool from which you
can do the alloc. With pool-based brigade allocation, there
are two basic choices when allocating a new brigade during
the split, and they both have problems:
- Allocate from a connection pool. This is what httpd-2.0
does, but it's not a robust solution, given that connections
can be very long-lived. And for non-http connections, like
an app server connector, connections may live forever, so
doing per-request allocations from a connection-level pool
guarantees a memory leak.
- Create some other pool that's guaranteed to have a sufficiently
long lifetime to accommodate the response. This isn't a
viable option either, because creating pools is expensive
in terms of both time and memory usage. In addition, the
app must then ensure that the pool gets cleaned up in all
error conditions; the cleanup burden simply gets shifted
from the brigade to the pool.
Pools just aren't a good match for the allocation needs of
brigades. We happen to have been able to hack around many
of the problems through liberal use of the "copy the brigade
into another pool with a more appropriate scope" technique
in httpd-2.0, but that's not a solution for apps in general.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 21:37, rbb@apache.org wrote:
> On 20 Dec 2002, Brian Pane wrote:
>
> > On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> >
> > > > Basically, if you received a brigade from a higher filter, you can only
> > > > assume that it will survive a single trip down the filter stack.
> >
> > Right. And you also can't assume that the context that passed
> > the brigade down the filter stack still exists by the time the
> > brigade reaches the last filter. It's a safe assumption in
> > httpd-2.0, where the brigade is passed as a synchronous function
> > call, but it's not valid for apps in general (potentially including
> > future Apache versions) where the brigade may be queued up for
> > asynchronous processing by an I/O completion thread.
>
> Actually, it's not even a safe assumption in httpd-2.0. You have data
> from requests that originally lived in brigades allocated out of the
> request pool moved to brigades allocated out of the connection pool
> because the request pool is going away. But that just proves the point,
> the data must be moved from one brigade to another to change it's
> lifetime. That is part of the design of buckets and brigades. It is why
> the concat and split operations were written for maximum performance.
>
> We are talking about a couple of pointer assignments for the concatenation
> and a malloc and some pointer assignments for the split.
And you're also talking about needing a pool from which you
can do the alloc. With pool-based brigade allocation, there
are two basic choices when allocating a new brigade during
the split, and they both have problems:
- Allocate from a connection pool. This is what httpd-2.0
does, but it's not a robust solution, given that connections
can be very long-lived. And for non-http connections, like
an app server connector, connections may live forever, so
doing per-request allocations from a connection-level pool
guarantees a memory leak.
- Create some other pool that's guaranteed to have a sufficiently
long lifetime to accommodate the response. This isn't a
viable option either, because creating pools is expensive
in terms of both time and memory usage. In addition, the
app must then ensure that the pool gets cleaned up in all
error conditions; the cleanup burden simply gets shifted
from the brigade to the pool.
Pools just aren't a good match for the allocation needs of
brigades. We happen to have been able to hack around many
of the problems through liberal use of the "copy the brigade
into another pool with a more appropriate scope" technique
in httpd-2.0, but that's not a solution for apps in general.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
>
> > > Basically, if you received a brigade from a higher filter, you can only
> > > assume that it will survive a single trip down the filter stack.
>
> Right. And you also can't assume that the context that passed
> the brigade down the filter stack still exists by the time the
> brigade reaches the last filter. It's a safe assumption in
> httpd-2.0, where the brigade is passed as a synchronous function
> call, but it's not valid for apps in general (potentially including
> future Apache versions) where the brigade may be queued up for
> asynchronous processing by an I/O completion thread.
Actually, it's not even a safe assumption in httpd-2.0. You have data
from requests that originally lived in brigades allocated out of the
request pool moved to brigades allocated out of the connection pool
because the request pool is going away. But that just proves the point,
the data must be moved from one brigade to another to change it's
lifetime. That is part of the design of buckets and brigades. It is why
the concat and split operations were written for maximum performance.
We are talking about a couple of pointer assignments for the concatenation
and a malloc and some pointer assignments for the split.
The change to stop allocating the brigade out of a pool is a mistake and I
am asking for it to be reverted, or for a clear description of the problem
that it is solving. I have asked for this description three times now,
and I still don't understand what exactly you are trying to achieve and
why it can't be achieved with the old code.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
>
> > > Basically, if you received a brigade from a higher filter, you can only
> > > assume that it will survive a single trip down the filter stack.
>
> Right. And you also can't assume that the context that passed
> the brigade down the filter stack still exists by the time the
> brigade reaches the last filter. It's a safe assumption in
> httpd-2.0, where the brigade is passed as a synchronous function
> call, but it's not valid for apps in general (potentially including
> future Apache versions) where the brigade may be queued up for
> asynchronous processing by an I/O completion thread.
Actually, it's not even a safe assumption in httpd-2.0. You have data
from requests that originally lived in brigades allocated out of the
request pool moved to brigades allocated out of the connection pool
because the request pool is going away. But that just proves the point,
the data must be moved from one brigade to another to change it's
lifetime. That is part of the design of buckets and brigades. It is why
the concat and split operations were written for maximum performance.
We are talking about a couple of pointer assignments for the concatenation
and a malloc and some pointer assignments for the split.
The change to stop allocating the brigade out of a pool is a mistake and I
am asking for it to be reverted, or for a clear description of the problem
that it is solving. I have asked for this description three times now,
and I still don't understand what exactly you are trying to achieve and
why it can't be achieved with the old code.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> > Basically, if you received a brigade from a higher filter, you can only
> > assume that it will survive a single trip down the filter stack.
Right. And you also can't assume that the context that passed
the brigade down the filter stack still exists by the time the
brigade reaches the last filter. It's a safe assumption in
httpd-2.0, where the brigade is passed as a synchronous function
call, but it's not valid for apps in general (potentially including
future Apache versions) where the brigade may be queued up for
asynchronous processing by an I/O completion thread.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 13:50, rbb@apache.org wrote:
> > Basically, if you received a brigade from a higher filter, you can only
> > assume that it will survive a single trip down the filter stack.
Right. And you also can't assume that the context that passed
the brigade down the filter stack still exists by the time the
brigade reaches the last filter. It's a safe assumption in
httpd-2.0, where the brigade is passed as a synchronous function
call, but it's not valid for apps in general (potentially including
future Apache versions) where the brigade may be queued up for
asynchronous processing by an I/O completion thread.
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 22:03, Cliff Woolley wrote:
>
> > Can we at least agree to do one thing:
> > release 2.0.44 *without* Brian's change.
>
> Okay with me.
I am vetoing this change until I get a clear description of the problem it
is solving. I have explained multiple times why it is a bad change, but I
can't offer any other solutions until I know why it is needed.
> > Then we can sort out the best
> > course of action without delaying the entire release process on the stable
> > branch. It might well be that the best course of action is to just fix
> > the places in httpd that abuse brigades. But as Bill pointed out, what
> > about third party modules? Ay caramba.
>
> FirstBill's patch should allow third party 2.0.x modules to work.
It will not. If you _ever_ create a brigade with a NULL pool, you will
have memory leaks. I dare say that every filter ever written will leak
if the feature implemented with this patch is ever used. I have already
posted one code segment in the core_output_filter that proves the memory
leak exists. Trust me when I tell you that there are hundreds more, and
some of them are in 3rd party modules that you don't control.
> > +1 to moving brigades back into httpd, fwiw.
>
> +1
++1, but they should go into an external library so that projects like
serf have access to them. Happily, I don't have to worry about that.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 22:03, Cliff Woolley wrote:
>
> > Can we at least agree to do one thing:
> > release 2.0.44 *without* Brian's change.
>
> Okay with me.
I am vetoing this change until I get a clear description of the problem it
is solving. I have explained multiple times why it is a bad change, but I
can't offer any other solutions until I know why it is needed.
> > Then we can sort out the best
> > course of action without delaying the entire release process on the stable
> > branch. It might well be that the best course of action is to just fix
> > the places in httpd that abuse brigades. But as Bill pointed out, what
> > about third party modules? Ay caramba.
>
> FirstBill's patch should allow third party 2.0.x modules to work.
It will not. If you _ever_ create a brigade with a NULL pool, you will
have memory leaks. I dare say that every filter ever written will leak
if the feature implemented with this patch is ever used. I have already
posted one code segment in the core_output_filter that proves the memory
leak exists. Trust me when I tell you that there are hundreds more, and
some of them are in 3rd party modules that you don't control.
> > +1 to moving brigades back into httpd, fwiw.
>
> +1
++1, but they should go into an external library so that projects like
serf have access to them. Happily, I don't have to worry about that.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 22:03, Cliff Woolley wrote:
> Can we at least agree to do one thing:
> release 2.0.44 *without* Brian's change.
Okay with me.
> Then we can sort out the best
> course of action without delaying the entire release process on the stable
> branch. It might well be that the best course of action is to just fix
> the places in httpd that abuse brigades. But as Bill pointed out, what
> about third party modules? Ay caramba.
FirstBill's patch should allow third party 2.0.x modules to work.
> +1 to moving brigades back into httpd, fwiw.
+1
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of
the bucket allocator
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-12-20 at 22:03, Cliff Woolley wrote:
> Can we at least agree to do one thing:
> release 2.0.44 *without* Brian's change.
Okay with me.
> Then we can sort out the best
> course of action without delaying the entire release process on the stable
> branch. It might well be that the best course of action is to just fix
> the places in httpd that abuse brigades. But as Bill pointed out, what
> about third party modules? Ay caramba.
FirstBill's patch should allow third party 2.0.x modules to work.
> +1 to moving brigades back into httpd, fwiw.
+1
Brian
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On Sat, 21 Dec 2002, Cliff Woolley wrote:
> On Fri, 20 Dec 2002 rbb@apache.org wrote:
>
> > apr_brigade_create(bb, ...);
> > fill_brigade_with_data(bb, ...);
> > ap_pass_brigade(bb, ...)
> >
> > ap_clean_brigade(bb, ...);
> > fill_brigade_with_more_data(bb, ...);
> > ap_pass_brigade(bb, ...);
> >
> > This is perfectly legal for a handler to do.
>
> No it isn't, because if ap_pass_brigade() makes it all the way down to the
> core_output_filter, then after ap_pass_brigade_() returns, bb will have
> been destroyed by the core_output_filter. I'm not necessarily saying
> that's *right*, just that that's the case today.
That would be a pretty massive bug. I have both handlers and filters that
follow this pattern because it is supposed to work. The reason it works
in my testing, is that _most_ of the time the core_output_filter doesn't
destroy that brigade.
if (ctx->b) {
APR_BRIGADE_CONCAT(ctx->b, b);
b = ctx->b;
ctx->b = NULL;
}
This is where the magic happens. If the filter has already saved aside a
brigade for this request (or a previous request in this connection), we
merge the two and drop the new one (a potential memory leak if not for
pools).
As I have said all day, a filter cannot keep or destroy a brigade that it
didn't create. The creator of the brigade owns the brigade, and no other
filter can modify the brigade. They can modify the buckets in the
brigade, but they can't do anything to the brigade itself.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On Sat, 21 Dec 2002, Cliff Woolley wrote:
> On Fri, 20 Dec 2002 rbb@apache.org wrote:
>
> > apr_brigade_create(bb, ...);
> > fill_brigade_with_data(bb, ...);
> > ap_pass_brigade(bb, ...)
> >
> > ap_clean_brigade(bb, ...);
> > fill_brigade_with_more_data(bb, ...);
> > ap_pass_brigade(bb, ...);
> >
> > This is perfectly legal for a handler to do.
>
> No it isn't, because if ap_pass_brigade() makes it all the way down to the
> core_output_filter, then after ap_pass_brigade_() returns, bb will have
> been destroyed by the core_output_filter. I'm not necessarily saying
> that's *right*, just that that's the case today.
That would be a pretty massive bug. I have both handlers and filters that
follow this pattern because it is supposed to work. The reason it works
in my testing, is that _most_ of the time the core_output_filter doesn't
destroy that brigade.
if (ctx->b) {
APR_BRIGADE_CONCAT(ctx->b, b);
b = ctx->b;
ctx->b = NULL;
}
This is where the magic happens. If the filter has already saved aside a
brigade for this request (or a previous request in this connection), we
merge the two and drop the new one (a potential memory leak if not for
pools).
As I have said all day, a filter cannot keep or destroy a brigade that it
didn't create. The creator of the brigade owns the brigade, and no other
filter can modify the brigade. They can modify the buckets in the
brigade, but they can't do anything to the brigade itself.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 20 Dec 2002 rbb@apache.org wrote:
> apr_brigade_create(bb, ...);
> fill_brigade_with_data(bb, ...);
> ap_pass_brigade(bb, ...)
>
> ap_clean_brigade(bb, ...);
> fill_brigade_with_more_data(bb, ...);
> ap_pass_brigade(bb, ...);
>
> This is perfectly legal for a handler to do.
No it isn't, because if ap_pass_brigade() makes it all the way down to the
core_output_filter, then after ap_pass_brigade_() returns, bb will have
been destroyed by the core_output_filter. I'm not necessarily saying
that's *right*, just that that's the case today.
I still can't decide whom to agree with in this debate. I'll have to
ponder it overnight before I throw in my opinion. One thing is certain...
the current level of segfaultyness and memory leakage is bad news for
httpd 2.0.44. Note that there are quite a few places in the httpd (last I
checked) where we abandon bucket brigades because it's always been the
case that they were cleaned up for us. The fact that that's changing
doesn't sit well with me, because it makes me nervous about the stability
of the next httpd 2.0 release. Can we at least agree to do one thing:
release 2.0.44 *without* Brian's change. Then we can sort out the best
course of action without delaying the entire release process on the stable
branch. It might well be that the best course of action is to just fix
the places in httpd that abuse brigades. But as Bill pointed out, what
about third party modules? Ay caramba.
+1 to moving brigades back into httpd, fwiw.
Thanks,
Cliff
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 20 Dec 2002 rbb@apache.org wrote:
> apr_brigade_create(bb, ...);
> fill_brigade_with_data(bb, ...);
> ap_pass_brigade(bb, ...)
>
> ap_clean_brigade(bb, ...);
> fill_brigade_with_more_data(bb, ...);
> ap_pass_brigade(bb, ...);
>
> This is perfectly legal for a handler to do.
No it isn't, because if ap_pass_brigade() makes it all the way down to the
core_output_filter, then after ap_pass_brigade_() returns, bb will have
been destroyed by the core_output_filter. I'm not necessarily saying
that's *right*, just that that's the case today.
I still can't decide whom to agree with in this debate. I'll have to
ponder it overnight before I throw in my opinion. One thing is certain...
the current level of segfaultyness and memory leakage is bad news for
httpd 2.0.44. Note that there are quite a few places in the httpd (last I
checked) where we abandon bucket brigades because it's always been the
case that they were cleaned up for us. The fact that that's changing
doesn't sit well with me, because it makes me nervous about the stability
of the next httpd 2.0 release. Can we at least agree to do one thing:
release 2.0.44 *without* Brian's change. Then we can sort out the best
course of action without delaying the entire release process on the stable
branch. It might well be that the best course of action is to just fix
the places in httpd that abuse brigades. But as Bill pointed out, what
about third party modules? Ay caramba.
+1 to moving brigades back into httpd, fwiw.
Thanks,
Cliff
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On Fri, 20 Dec 2002 rbb@apache.org wrote:
> On 20 Dec 2002, Brian Pane wrote:
>
> > On Fri, 2002-12-20 at 08:57, rbb@apache.org wrote:
> > > I am actually pretty sure that allocating brigades out of the
> > > bucket_allocator is a VERY big mistake. In fact, I am close to asking
> > > that change to be backed out because it is too dangerous.
> > >
> > > When we first designed buckets and bucket brigades, we made one VERY clear
> > > distinction. Bucket_brigades are allocated out of a pool, because that
> > > stops us from leaking memory.
> >
> > Allocating brigades from a pool is often a design mistake.
> > Brigades tend to outlive the transactions that produce
> > them (especially in apps like http servers), and having a
> > brigade disappear as a side-effect of a transaction pool's
> > cleanup will cause problems elsewhere in the code.
>
> Ummmm, I don't believe that it is possible for a brigade to outlive the
> transaction that created it, especially not when you look at how brigades
> are used in the web server. Basically, a handler creates a brigade and
> passes it down the filter stack. Each filter in the line is then allowed
> to either concatenate that brigade to it's own brigade or keep sending it
> down the stack. At the bottom of the stack, the brigade must have been
> generated out of the connection pool, which by definition lives as long as
> the transaction that created it.
>
> Basically, if you received a brigade from a higher filter, you can only
> assume that it will survive a single trip down the filter stack. That is
> why all filters must create their own brigade to save data between calls.
Just to be very clear about why this true. Imagine a handler that does
this:
apr_brigade_create(bb, ...);
fill_brigade_with_data(bb, ...);
ap_pass_brigade(bb, ...)
ap_clean_brigade(bb, ...);
fill_brigade_with_more_data(bb, ...);
ap_pass_brigade(bb, ...);
This is perfectly legal for a handler to do. However, this means that any
filter that wants to save the data in this brigade for use later, must
copy the buckets to their own brigade. If they try to just save a pointer
to the brigade, they won't have the data they expect on the second call.
The brigade will still exist, but the data will be wrong.
For this reason, all filters must move the buckets to their own brigade,
which means that a brigade cannot outlive the transaction that it was
created for. Essentially, the filters mitigate the brigade lifetimes.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
> and you will have a hell of a time tracking it down. I can gaurantee that
> this change caused a memory leak. I know it did because Greg Stein and I
> argued over leaving a comment in the code that said essentially "This
> would be a memory leak if the brigade were allocated out of a pool, but it
oops: s/were/weren't/
> is, so it is safe". We removed the comment, because the bucket_brigade
> design requires that all brigades are allocated out of pools.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On Fri, 20 Dec 2002 rbb@apache.org wrote:
> On 20 Dec 2002, Brian Pane wrote:
>
> > On Fri, 2002-12-20 at 08:57, rbb@apache.org wrote:
> > > I am actually pretty sure that allocating brigades out of the
> > > bucket_allocator is a VERY big mistake. In fact, I am close to asking
> > > that change to be backed out because it is too dangerous.
> > >
> > > When we first designed buckets and bucket brigades, we made one VERY clear
> > > distinction. Bucket_brigades are allocated out of a pool, because that
> > > stops us from leaking memory.
> >
> > Allocating brigades from a pool is often a design mistake.
> > Brigades tend to outlive the transactions that produce
> > them (especially in apps like http servers), and having a
> > brigade disappear as a side-effect of a transaction pool's
> > cleanup will cause problems elsewhere in the code.
>
> Ummmm, I don't believe that it is possible for a brigade to outlive the
> transaction that created it, especially not when you look at how brigades
> are used in the web server. Basically, a handler creates a brigade and
> passes it down the filter stack. Each filter in the line is then allowed
> to either concatenate that brigade to it's own brigade or keep sending it
> down the stack. At the bottom of the stack, the brigade must have been
> generated out of the connection pool, which by definition lives as long as
> the transaction that created it.
>
> Basically, if you received a brigade from a higher filter, you can only
> assume that it will survive a single trip down the filter stack. That is
> why all filters must create their own brigade to save data between calls.
Just to be very clear about why this true. Imagine a handler that does
this:
apr_brigade_create(bb, ...);
fill_brigade_with_data(bb, ...);
ap_pass_brigade(bb, ...)
ap_clean_brigade(bb, ...);
fill_brigade_with_more_data(bb, ...);
ap_pass_brigade(bb, ...);
This is perfectly legal for a handler to do. However, this means that any
filter that wants to save the data in this brigade for use later, must
copy the buckets to their own brigade. If they try to just save a pointer
to the brigade, they won't have the data they expect on the second call.
The brigade will still exist, but the data will be wrong.
For this reason, all filters must move the buckets to their own brigade,
which means that a brigade cannot outlive the transaction that it was
created for. Essentially, the filters mitigate the brigade lifetimes.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
> and you will have a hell of a time tracking it down. I can gaurantee that
> this change caused a memory leak. I know it did because Greg Stein and I
> argued over leaving a comment in the code that said essentially "This
> would be a memory leak if the brigade were allocated out of a pool, but it
oops: s/were/weren't/
> is, so it is safe". We removed the comment, because the bucket_brigade
> design requires that all brigades are allocated out of pools.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 08:57, rbb@apache.org wrote:
> > I am actually pretty sure that allocating brigades out of the
> > bucket_allocator is a VERY big mistake. In fact, I am close to asking
> > that change to be backed out because it is too dangerous.
> >
> > When we first designed buckets and bucket brigades, we made one VERY clear
> > distinction. Bucket_brigades are allocated out of a pool, because that
> > stops us from leaking memory.
>
> Allocating brigades from a pool is often a design mistake.
> Brigades tend to outlive the transactions that produce
> them (especially in apps like http servers), and having a
> brigade disappear as a side-effect of a transaction pool's
> cleanup will cause problems elsewhere in the code.
Ummmm, I don't believe that it is possible for a brigade to outlive the
transaction that created it, especially not when you look at how brigades
are used in the web server. Basically, a handler creates a brigade and
passes it down the filter stack. Each filter in the line is then allowed
to either concatenate that brigade to it's own brigade or keep sending it
down the stack. At the bottom of the stack, the brigade must have been
generated out of the connection pool, which by definition lives as long as
the transaction that created it.
Basically, if you received a brigade from a higher filter, you can only
assume that it will survive a single trip down the filter stack. That is
why all filters must create their own brigade to save data between calls.
> > By allocating bucket_brigades out of the bucket_allocator, we have removed
> > the protection that the pool cleanups used to give us.
>
> It's important to keep in mind just what sort of protection
> the pool-based brigade allocation offered. It allowed code
> that did this:
>
> bb = apr_brigade_create(...);
> apr_brigade_destroy(bb);
> APR_BRIGADE_INSERT_HEAD(bb, bucket);
>
> to work accidentally. I'm hesitant to call that "protection."
Give it up, that is a side-effect of pools, and it isn't the protection
that we discussed when designing buckets and brigades. The protection
that we are talking about is against memory leaks, which this change
almost definately opened up in the web server. There is at least one
place in the code where we drop a bucket brigade on the floor without
cleaning it up, because we rely on the pool_cleanup to do it for us. This
change removes that ability. Of course, you won't see that memory leak
unless you make the correct request, but at some point, it will show up
and you will have a hell of a time tracking it down. I can gaurantee that
this change caused a memory leak. I know it did because Greg Stein and I
argued over leaving a comment in the code that said essentially "This
would be a memory leak if the brigade were allocated out of a pool, but it
is, so it is safe". We removed the comment, because the bucket_brigade
design requires that all brigades are allocated out of pools.
As for the bogus psuedo-code that you have above, that is pools for you.
The same thing could be said for _ANY_ apr type. Try this sometime:
apr_file_open(&f, .....);
apr_file_close(f);
apr_file_name_get(&fname, f);
You will get the correct value for fname without a segfault, and this will
work on all platforms. The only way to fix that type of problem would be
to make all destroy/close functions accept a pointer to the object that
they are destroying/closing. Then, we could set the pointer to NULL, and
the code above wouldn't work.
We don't try to protect people from doing bone-headed things like using a
structure after it has been destroyed, that is just a programmer not
paying attention. However, the code that you have added removes the
memory leak protection that brigades were supposed to provide for buckets.
That is wrong, and thus the change should be removed.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of the
bucket allocator
Posted by rb...@apache.org.
On 20 Dec 2002, Brian Pane wrote:
> On Fri, 2002-12-20 at 08:57, rbb@apache.org wrote:
> > I am actually pretty sure that allocating brigades out of the
> > bucket_allocator is a VERY big mistake. In fact, I am close to asking
> > that change to be backed out because it is too dangerous.
> >
> > When we first designed buckets and bucket brigades, we made one VERY clear
> > distinction. Bucket_brigades are allocated out of a pool, because that
> > stops us from leaking memory.
>
> Allocating brigades from a pool is often a design mistake.
> Brigades tend to outlive the transactions that produce
> them (especially in apps like http servers), and having a
> brigade disappear as a side-effect of a transaction pool's
> cleanup will cause problems elsewhere in the code.
Ummmm, I don't believe that it is possible for a brigade to outlive the
transaction that created it, especially not when you look at how brigades
are used in the web server. Basically, a handler creates a brigade and
passes it down the filter stack. Each filter in the line is then allowed
to either concatenate that brigade to it's own brigade or keep sending it
down the stack. At the bottom of the stack, the brigade must have been
generated out of the connection pool, which by definition lives as long as
the transaction that created it.
Basically, if you received a brigade from a higher filter, you can only
assume that it will survive a single trip down the filter stack. That is
why all filters must create their own brigade to save data between calls.
> > By allocating bucket_brigades out of the bucket_allocator, we have removed
> > the protection that the pool cleanups used to give us.
>
> It's important to keep in mind just what sort of protection
> the pool-based brigade allocation offered. It allowed code
> that did this:
>
> bb = apr_brigade_create(...);
> apr_brigade_destroy(bb);
> APR_BRIGADE_INSERT_HEAD(bb, bucket);
>
> to work accidentally. I'm hesitant to call that "protection."
Give it up, that is a side-effect of pools, and it isn't the protection
that we discussed when designing buckets and brigades. The protection
that we are talking about is against memory leaks, which this change
almost definately opened up in the web server. There is at least one
place in the code where we drop a bucket brigade on the floor without
cleaning it up, because we rely on the pool_cleanup to do it for us. This
change removes that ability. Of course, you won't see that memory leak
unless you make the correct request, but at some point, it will show up
and you will have a hell of a time tracking it down. I can gaurantee that
this change caused a memory leak. I know it did because Greg Stein and I
argued over leaving a comment in the code that said essentially "This
would be a memory leak if the brigade were allocated out of a pool, but it
is, so it is safe". We removed the comment, because the bucket_brigade
design requires that all brigades are allocated out of pools.
As for the bogus psuedo-code that you have above, that is pools for you.
The same thing could be said for _ANY_ apr type. Try this sometime:
apr_file_open(&f, .....);
apr_file_close(f);
apr_file_name_get(&fname, f);
You will get the correct value for fname without a segfault, and this will
work on all platforms. The only way to fix that type of problem would be
to make all destroy/close functions accept a pointer to the object that
they are destroying/closing. Then, we could set the pointer to NULL, and
the code above wouldn't work.
We don't try to protect people from doing bone-headed things like using a
structure after it has been destroyed, that is just a programmer not
paying attention. However, the code that you have added removes the
memory leak protection that brigades were supposed to provide for buckets.
That is wrong, and thus the change should be removed.
Ryan