On 20 Dec 2002, Brian Pane wrote: > On Fri, 2002-12-20 at 08:57, [EMAIL PROTECTED] 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
