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 19:24:31 GMT
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


Mime
View raw message