On 25-Apr-05, at 4:26 PM, Joe Orton wrote:
On Mon, Apr 25, 2005 at 03:58:59PM -0500, Rici Lake wrote:If we accept that the contents of a brigade are "undefined" when
ap_pass_brigade returns, the caller has three options:
-- call cleanup and reuse the brigade
-- call destroy (which will first call cleanup)
-- drop it on the floor and let destroy be called by the cleanup function
In no case can it make any use of the brigade's contents (they're "undefined"), so cleanup must be called just after ap_pass_brigade returns.
But in the latter two cases, adding a apr_brigade_cleanup() call to ap_pass_brigade() is completely redundant. To me, you need to make the argument that the former case is so prevalent that it's worth an additional API guarantee, and the additional (small) overhead in in ap_pass_brigade. I don't see it really...
I guess that depends on whether most filters use and recycle pass brigades. I suspect that most internal filters don't (and possibly have the model which Joe Schaefer describes, where the brigade is simply passed through), whereas most third-party filters probably either do or should. Having now looked at quite a few output filters, I've come to the conclusion that the normal model is to create and destroy brigades on every invocation, which I can't help but think is a lot less efficient than it could be.
But I actually disagree with the premise: the issue is not how prevalent the case is, it is how likely it is to create a bug which is triggered by an interaction between unrelated modules. Mitigating the creation of such bugs is, in my mind, an important part of component API design; particularly if the mitigation is low-cost. Of course, everyone has their own concept of beauty, and mine is probably the least important of anyone participating in this debate.
In any event, a draft docstring. This does not reflect my view, and I don't know if there is actually a consensus view, but it may reflect the current state of affairs:
Index: util_filter.h
===================================================================
--- util_filter.h (revision 158730)
+++ util_filter.h (working copy)
@@ -103,8 +103,8 @@
* @name Filter callbacks
*
* This function type is used for filter callbacks. It will be passed a
- * pointer to "this" filter, and a "bucket" containing the content to be
- * filtered.
+ * pointer to "this" filter, and a "bucket brigade" containing the content
+ * to be filtered.
*
* In filter->ctx, the callback will find its context. This context is
* provided here, so that a filter may be installed multiple times, each
@@ -120,10 +120,15 @@
* or output filter chains and before any data is generated to allow the
* filter to prepare for processing.
*
- * The *bucket structure (and all those referenced by ->next and ->prev)
- * should be considered "const". The filter is allowed to modify the
- * next/prev to insert/remove/replace elements in the bucket list, but
- * the types and values of the individual buckets should not be altered.
+ * The bucket brigade always belongs to the caller, but the filter
+ * is free to use the buckets within it as it sees fit. Normally,
+ * the brigade will be returned empty. Buckets *may not* be retained
+ * between successive calls to the filter unless they have been
+ * "set aside" with a call apr_bucket_setaside. Typically this will
+ * be done with ap_save_brigade(). Buckets removed from the brigade
+ * become the responsibility of the filter, which must arrange for
+ * them to be deleted, either by doing so directly or by inserting
+ * them in a brigade which will subsequently be destroyed.
*
* For the input and output filters, the return value of a filter should be
* an APR status value. For the init function, the return value should
@@ -299,9 +304,13 @@
* Pass the current bucket brigade down to the next filter on the filter
* stack. The filter returns an apr_status_t value. If the bottom-most
* filter doesn't write to the network, then ::AP_NOBODY_WROTE is returned.
- * The caller relinquishes ownership of the brigade.
* @param filter The next filter in the chain
* @param bucket The current bucket brigade
+ *
+ * @remark Ownership of the brigade is retained by the caller. On return,
+ * the contents of the brigade are UNDEFINED, and the caller must
+ * either call apr_brigade_cleanup or apr_brigade_destroy on
+ * the brigade.
*/
AP_DECLARE(apr_status_t) ap_pass_brigade(ap_filter_t *filter,
apr_bucket_brigade *bucket);
