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