apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@apache.org>
Subject Re: [PATCH] Update to Brian's patch to allocate brigades out of the bucket allocator
Date Fri, 20 Dec 2002 21:26:23 GMT

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


Mime
View raw message