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 20:19:43 GMT

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


Mime
View raw message