Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On Sat, 21 Dec 2002, Cliff Woolley wrote:
On Fri, 20 Dec 2002 [EMAIL PROTECTED] wrote:
apr_brigade_create(bb, ...);
fill_brigade_with_data(bb, ...);
ap_pass_brigade(bb, ...)
ap_clean_brigade(bb, ...);
fill_brigade_with_more_data(bb, ...);
ap_pass_brigade(bb, ...);
This is perfectly legal for a handler to do.
No it isn't, because if ap_pass_brigade() makes it all the way down to the
core_output_filter, then after ap_pass_brigade_() returns, bb will have
been destroyed by the core_output_filter. I'm not necessarily saying
that's *right*, just that that's the case today.
That would be a pretty massive bug. I have both handlers and filters that
follow this pattern because it is supposed to work. The reason it works
in my testing, is that _most_ of the time the core_output_filter doesn't
destroy that brigade.
if (ctx-b) {
APR_BRIGADE_CONCAT(ctx-b, b);
b = ctx-b;
ctx-b = NULL;
}
This is where the magic happens. If the filter has already saved aside a
brigade for this request (or a previous request in this connection), we
merge the two and drop the new one (a potential memory leak if not for
pools).
As I have said all day, a filter cannot keep or destroy a brigade that it
didn't create. The creator of the brigade owns the brigade, and no other
filter can modify the brigade. They can modify the buckets in the
brigade, but they can't do anything to the brigade itself.
Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On 20 Dec 2002, Brian Pane wrote: On Fri, 2002-12-20 at 21:37, [EMAIL PROTECTED] wrote: On 20 Dec 2002, Brian Pane wrote: On Fri, 2002-12-20 at 13:50, [EMAIL PROTECTED] wrote: 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. Right. And you also can't assume that the context that passed the brigade down the filter stack still exists by the time the brigade reaches the last filter. It's a safe assumption in httpd-2.0, where the brigade is passed as a synchronous function call, but it's not valid for apps in general (potentially including future Apache versions) where the brigade may be queued up for asynchronous processing by an I/O completion thread. Actually, it's not even a safe assumption in httpd-2.0. You have data from requests that originally lived in brigades allocated out of the request pool moved to brigades allocated out of the connection pool because the request pool is going away. But that just proves the point, the data must be moved from one brigade to another to change it's lifetime. That is part of the design of buckets and brigades. It is why the concat and split operations were written for maximum performance. We are talking about a couple of pointer assignments for the concatenation and a malloc and some pointer assignments for the split. And you're also talking about needing a pool from which you can do the alloc. With pool-based brigade allocation, there are two basic choices when allocating a new brigade during the split, and they both have problems: Um, you need a pool that lives at least as long as your filter. But that should make sense, because if you are going to do any memory allocation, you will need a pool in your filter. - Allocate from a connection pool. This is what httpd-2.0 does, but it's not a robust solution, given that connections can be very long-lived. And for non-http connections, like an app server connector, connections may live forever, so doing per-request allocations from a connection-level pool guarantees a memory leak. No, httpd-2.0 only uses the connection pool in filters that live as long as the connection. If that scope doesn't make sense, then you have to clear the brigade when you are done with it. Remember, we are talking about a couple of bytes for the size of the brigade. Even if it does leak until the end of the connection, it isn't a very big leak. It is a huge leak if you don't clear the brigade, but that is why we have brigade_clear. Pools just aren't a good match for the allocation needs of brigades. We happen to have been able to hack around many of the problems through liberal use of the copy the brigade into another pool with a more appropriate scope technique in httpd-2.0, but that's not a solution for apps in general. I'm really sorry, but I am sick of hearing this isn't a solution for apps in general. This is now the fourth time that I have asked for a clear description of _why_. I helped to design and write buckets and bucket_brigades. I understand them incredibly well. You seem to be unable or unwilling to give a clear description of the problem you are having. All you are doing is hand-waving saying that this might be a problem for some apps. Give a clear description please. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On 20 Dec 2002, Brian Pane wrote: On Fri, 2002-12-20 at 22:03, Cliff Woolley wrote: Can we at least agree to do one thing: release 2.0.44 *without* Brian's change. Okay with me. I am vetoing this change until I get a clear description of the problem it is solving. I have explained multiple times why it is a bad change, but I can't offer any other solutions until I know why it is needed. Then we can sort out the best course of action without delaying the entire release process on the stable branch. It might well be that the best course of action is to just fix the places in httpd that abuse brigades. But as Bill pointed out, what about third party modules? Ay caramba. FirstBill's patch should allow third party 2.0.x modules to work. It will not. If you _ever_ create a brigade with a NULL pool, you will have memory leaks. I dare say that every filter ever written will leak if the feature implemented with this patch is ever used. I have already posted one code segment in the core_output_filter that proves the memory leak exists. Trust me when I tell you that there are hundreds more, and some of them are in 3rd party modules that you don't control. +1 to moving brigades back into httpd, fwiw. +1 ++1, but they should go into an external library so that projects like serf have access to them. Happily, I don't have to worry about that. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On 21 Dec 2002, Brian Pane wrote: On Sat, 2002-12-21 at 07:58, [EMAIL PROTECTED] wrote: I'm really sorry, but I am sick of hearing this isn't a solution for apps in general. This is now the fourth time that I have asked for a clear description of _why_. I helped to design and write buckets and bucket_brigades. I understand them incredibly well. You seem to be unable or unwilling to give a clear description of the problem you are having. All you are doing is hand-waving saying that this might be a problem for some apps. Give a clear description please. Sorry, but that's just bogus. I've described repeatedly, and in detail, the reasons why we can't make bucket allocation dependent on pools. You keep posting objections along the lines of but we can just move a brigade into a pool of appropriate scope, when you know that's not a solution. No, that is a solution, in fact, it was part of the original design. What you've done is said The brigade may not live long enough, but you haven't explained how that is the case. You also haven't explained why moving the buckets to another brigade won't work. Which I have a very hard time believing, because that is how Apache works today. You also haven't answered the problem that even _WITH_ this change, you still need to move the buckets to a new brigade. At the end of the day, this change doesn't get you anything, because you _still_ have to do the brigade migration that you say you can't do. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
What you've done is said The brigade may not live long enough, but you haven't explained how that is the case. This, too, is something that we've discussed already. Request cleanups can happen asynchronously relative to transmission of brigades created from the request. Having a pool cleanup trigger the destruction of a brigade in another thread is dangerous. The canonical solution is to migrate the data to a new brigade outside the pool before handing it off to another thread, which is fine as long as that doesn't create a need for another pool in the application (for the reasons noted above). You are talking about an application that is built around pools. If you want an async MPM that uses a single thread to write data to the network and multiple threads to generate the data, then your network I/O thread will need to have a pool. This pool will be very long lived (as long as the process is alive). You can allocate the brigade when the server starts, and just re-use it forever. You can have a pool of brigades that are continuosly re-used, or you can create a sub-pool for each call into your network write function. But, moving brigade allocation outside of pools isn't solving your problem and it is opening huge holes in the abstraction that was created. Screw it. Buckets and bucket brigades are being moved into a project that I no longer have any desire to work on. My veto stands for as long as the code is in an APR project. The change is wrong and it is dangerous. I have explained why in detail. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
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. Buckets shouldn't be allocated out of a
pool, because nobody knows which pool to allocate them from. Basically,
we said that in Apache, it is safe to just drop a bucket briade because
the pool cleanups will free the memory. It is not safe to just drop a
bucket, it must either be left on the brigade to be cleaned up by the
brigade_cleanup function, or it must be destroyed when it is removed from
the brigade.
By allocating bucket_brigades out of the bucket_allocator, we have removed
the protection that the pool cleanups used to give us. I may be wrong
about how the bucket_allocator works, but I don't believe I am.
Basically, this change will make Apache leak memory on some requests, and
it goes against the original design of buckets and bucket brigades. In
fact, allocating the brigades out of a pool was a requirement, because
some people at the meeting were afraid of memory leaks with bucket
brigades.
Ryan
On Fri, 20 Dec 2002, Bill Stoddard wrote:
If a pool is passed to apr_brigade_create, the brigade is allocated out of the
pool. If the pool is NULL, the brigade is allocated out of the bucket allocator.
We don't want a pool pointer hanging around in a brigade allocated out of the
bucket allocator. That's just asking for trouble. This patch makes how the
brigade is allocated, either out of the pool or out of the allocator, explicit.
Index: apr_brigade.c
===
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.55
diff -u -r1.55 apr_brigade.c
--- apr_brigade.c 17 Dec 2002 19:16:39 - 1.55
+++ apr_brigade.c 20 Dec 2002 15:17:58 -
@@ -95,7 +95,9 @@
apr_pool_cleanup_kill(b-p, b, brigade_cleanup);
}
rv = apr_brigade_cleanup(b);
-apr_bucket_free(b);
+if (!b-p) {
+apr_bucket_free(b);
+}
return rv;
}
@@ -104,7 +106,12 @@
{
apr_bucket_brigade *b;
-b = apr_bucket_alloc(sizeof(*b), list);
+if (p) {
+b = apr_palloc(p, sizeof(*b));
+}
+else {
+b = apr_bucket_alloc(sizeof(*b), list);
+}
b-p = p;
b-bucket_alloc = list;
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
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. U, 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
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
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 oops: s/were/weren't/ is, so it is safe. We removed the comment, because the bucket_brigade design requires that all brigades are allocated out of pools. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
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
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On Fri, 20 Dec 2002 [EMAIL PROTECTED] wrote: 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. U, 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. Just to be very clear about why this true. Imagine a handler that does this: apr_brigade_create(bb, ...); fill_brigade_with_data(bb, ...); ap_pass_brigade(bb, ...) ap_clean_brigade(bb, ...); fill_brigade_with_more_data(bb, ...); ap_pass_brigade(bb, ...); This is perfectly legal for a handler to do. However, this means that any filter that wants to save the data in this brigade for use later, must copy the buckets to their own brigade. If they try to just save a pointer to the brigade, they won't have the data they expect on the second call. The brigade will still exist, but the data will be wrong. For this reason, all filters must move the buckets to their own brigade, which means that a brigade cannot outlive the transaction that it was created for. Essentially, the filters mitigate the brigade lifetimes. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On Fri, 20 Dec 2002, Aaron Bannert wrote: On Friday, December 20, 2002, at 01:26 PM, [EMAIL PROTECTED] wrote: The pools are used so that we don't have to track resources this closely, that is, in fact, their main benefit. They were also added for performance reasons, but I don't know if this was a side effect or the main reason. Delaying freelist management until after a response gives a huge performance gain from the perspective of an http client. yes, performance was a reason in httpd, but in APR, it really came down to convenience. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On 20 Dec 2002, Brian Pane wrote: On Fri, 2002-12-20 at 13:50, [EMAIL PROTECTED] wrote: 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. Right. And you also can't assume that the context that passed the brigade down the filter stack still exists by the time the brigade reaches the last filter. It's a safe assumption in httpd-2.0, where the brigade is passed as a synchronous function call, but it's not valid for apps in general (potentially including future Apache versions) where the brigade may be queued up for asynchronous processing by an I/O completion thread. Actually, it's not even a safe assumption in httpd-2.0. You have data from requests that originally lived in brigades allocated out of the request pool moved to brigades allocated out of the connection pool because the request pool is going away. But that just proves the point, the data must be moved from one brigade to another to change it's lifetime. That is part of the design of buckets and brigades. It is why the concat and split operations were written for maximum performance. We are talking about a couple of pointer assignments for the concatenation and a malloc and some pointer assignments for the split. The change to stop allocating the brigade out of a pool is a mistake and I am asking for it to be reverted, or for a clear description of the problem that it is solving. I have asked for this description three times now, and I still don't understand what exactly you are trying to achieve and why it can't be achieved with the old code. Ryan
Re: [PATCH] Update to Brian's patch to allocate brigades out of thebucket allocator
On Fri, 20 Dec 2002 [EMAIL PROTECTED] wrote: apr_brigade_create(bb, ...); fill_brigade_with_data(bb, ...); ap_pass_brigade(bb, ...) ap_clean_brigade(bb, ...); fill_brigade_with_more_data(bb, ...); ap_pass_brigade(bb, ...); This is perfectly legal for a handler to do. No it isn't, because if ap_pass_brigade() makes it all the way down to the core_output_filter, then after ap_pass_brigade_() returns, bb will have been destroyed by the core_output_filter. I'm not necessarily saying that's *right*, just that that's the case today. I still can't decide whom to agree with in this debate. I'll have to ponder it overnight before I throw in my opinion. One thing is certain... the current level of segfaultyness and memory leakage is bad news for httpd 2.0.44. Note that there are quite a few places in the httpd (last I checked) where we abandon bucket brigades because it's always been the case that they were cleaned up for us. The fact that that's changing doesn't sit well with me, because it makes me nervous about the stability of the next httpd 2.0 release. Can we at least agree to do one thing: release 2.0.44 *without* Brian's change. Then we can sort out the best course of action without delaying the entire release process on the stable branch. It might well be that the best course of action is to just fix the places in httpd that abuse brigades. But as Bill pointed out, what about third party modules? Ay caramba. +1 to moving brigades back into httpd, fwiw. Thanks, Cliff
